[committed] arm: correctly decode Tag_THUMB_ISA_use=3 for thumb2 features

[committed] arm: correctly decode Tag_THUMB_ISA_use=3 for thumb2 features

[Richard Earnshaw]

This was detected when a user accidentally tried to build a shared
library using armv8-m.main objects.  The resulting error was "warning:
thumb-1 mode PLT generation not currently supported".  Something was
clearly wrong because v8-m.main is a thumb-2 variant.

It turns out that the code to detect thumb-2 in object files hadn't
been updated for the extended definition of Tag_THUMB_ISA_use to
support the value 3, meaning 'work it out for yourself from the
architecture tag'; something that is now necessary given that the line
between thumb-1 and thumb-2 has become blurred over time.

Another problem with the function doing this calculation was that the
absence of this tag (implying a default value 0) should mean use of
thumb code was NOT permitted.  However, the code went on to look at
the architecture flags and decide that it could ignore this if the
architecture flags said that thumb2 features were available, thus
completely ignoring the intended meaning.

bfd/

	* elf32-arm.c (using_thumb2): Correctly handle Tag_THUMB_ISA_use
	values 0 and 3.
---
 bfd/elf32-arm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index 79b94e836fc..cb567fe82ad 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -3888,9 +3888,11 @@ using_thumb2 (struct elf32_arm_link_hash_table *globals)
   int thumb_isa = bfd_elf_get_obj_attr_int (globals->obfd, OBJ_ATTR_PROC,
 					    Tag_THUMB_ISA_use);
 
-  if (thumb_isa)
+  /* No use of thumb permitted, or a legacy thumb-1/2 definition.  */
+  if (thumb_isa < 3)
     return thumb_isa == 2;
 
+  /* Variant of thumb is described by the architecture tag.  */
   arch = bfd_elf_get_obj_attr_int (globals->obfd, OBJ_ATTR_PROC, Tag_CPU_arch);
 
   /* Force return logic to be reviewed for each new architecture.  */
-- 
2.25.1

Re: [committed] arm: correctly decode Tag_THUMB_ISA_use=3 for thumb2 features

[Christophe Lyon]

Hi Richard,

On Tue, 11 May 2021 at 17:23, Richard Earnshaw via Binutils
<binutils@sourceware.org> wrote:
>
> This was detected when a user accidentally tried to build a shared
> library using armv8-m.main objects.  The resulting error was "warning:
> thumb-1 mode PLT generation not currently supported".  Something was
> clearly wrong because v8-m.main is a thumb-2 variant.
>
> It turns out that the code to detect thumb-2 in object files hadn't
> been updated for the extended definition of Tag_THUMB_ISA_use to
> support the value 3, meaning 'work it out for yourself from the
> architecture tag'; something that is now necessary given that the line
> between thumb-1 and thumb-2 has become blurred over time.
>
> Another problem with the function doing this calculation was that the
> absence of this tag (implying a default value 0) should mean use of
> thumb code was NOT permitted.  However, the code went on to look at
> the architecture flags and decide that it could ignore this if the
> architecture flags said that thumb2 features were available, thus
> completely ignoring the intended meaning.
>
> bfd/
>
>         * elf32-arm.c (using_thumb2): Correctly handle Tag_THUMB_ISA_use
>         values 0 and 3.


This patch causes a regression in the ld tests:
FAIL:Thumb-Thumb farcall v8-M Mainline

Can you check?

Thanks,

Christophe

> ---
>  bfd/elf32-arm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> index 79b94e836fc..cb567fe82ad 100644
> --- a/bfd/elf32-arm.c
> +++ b/bfd/elf32-arm.c
> @@ -3888,9 +3888,11 @@ using_thumb2 (struct elf32_arm_link_hash_table *globals)
>    int thumb_isa = bfd_elf_get_obj_attr_int (globals->obfd, OBJ_ATTR_PROC,
>                                             Tag_THUMB_ISA_use);
>
> -  if (thumb_isa)
> +  /* No use of thumb permitted, or a legacy thumb-1/2 definition.  */
> +  if (thumb_isa < 3)
>      return thumb_isa == 2;
>
> +  /* Variant of thumb is described by the architecture tag.  */
>    arch = bfd_elf_get_obj_attr_int (globals->obfd, OBJ_ATTR_PROC, Tag_CPU_arch);
>
>    /* Force return logic to be reviewed for each new architecture.  */
> --
> 2.25.1
>

Autogenerate ChangeLog commits (was: [committed] arm:...)

[Hans-Peter Nilsson]

On Tue, 11 May 2021, Christophe Lyon via Binutils wrote:
> This patch causes a regression in the ld tests:
> FAIL:Thumb-Thumb farcall v8-M Mainline
>
> Can you check?

Even worse, no ChangeLog entry was committed! ;-)

Everyone,
On the more serious side, how about adopting the wonderful gcc
commit-hook and scripts that takes care of this, already?

I'm sure most other people, including me, will eventually forget
about it committing ChangeLog entries too.

brgds, H-P
PS. Not a debate about having ChangeLogs at all, please.

Re: Autogenerate ChangeLog commits

[Tom Tromey]

>>>>> "Hans-Peter" == Hans-Peter Nilsson <hp@bitrange.com> writes:

Hans-Peter> On the more serious side, how about adopting the wonderful gcc
Hans-Peter> commit-hook and scripts that takes care of this, already?

I tried this recently and found it even worse than the status quo, so
strong -1 from me, unless it somehow does not apply to gdb.

glibc I think fully automated ChangeLogs and got rid of any need to
format any kind of entry.  Dunno if that would work for gdb but it might
for the C code in tree.

Hans-Peter> PS. Not a debate about having ChangeLogs at all, please.

We've been discussing this on the gdb maintainers list.
I'm still hopeful it is what we'll adopt there.

thanks,
Tom

Re: [committed] arm: correctly decode Tag_THUMB_ISA_use=3 for thumb2 features

[Richard Earnshaw]

On 11/05/2021 19:52, Christophe Lyon via Binutils wrote:
> Hi Richard,
> 
> On Tue, 11 May 2021 at 17:23, Richard Earnshaw via Binutils
> <binutils@sourceware.org> wrote:
>>
>> This was detected when a user accidentally tried to build a shared
>> library using armv8-m.main objects.  The resulting error was "warning:
>> thumb-1 mode PLT generation not currently supported".  Something was
>> clearly wrong because v8-m.main is a thumb-2 variant.
>>
>> It turns out that the code to detect thumb-2 in object files hadn't
>> been updated for the extended definition of Tag_THUMB_ISA_use to
>> support the value 3, meaning 'work it out for yourself from the
>> architecture tag'; something that is now necessary given that the line
>> between thumb-1 and thumb-2 has become blurred over time.
>>
>> Another problem with the function doing this calculation was that the
>> absence of this tag (implying a default value 0) should mean use of
>> thumb code was NOT permitted.  However, the code went on to look at
>> the architecture flags and decide that it could ignore this if the
>> architecture flags said that thumb2 features were available, thus
>> completely ignoring the intended meaning.
>>
>> bfd/
>>
>>          * elf32-arm.c (using_thumb2): Correctly handle Tag_THUMB_ISA_use
>>          values 0 and 3.
> 
> 
> This patch causes a regression in the ld tests:
> FAIL:Thumb-Thumb farcall v8-M Mainline

Sigh, I could have sworn I'd run all the tests...  I see it as well now, 
however.

A quick look suggests the test is wrong and has been wrong since it was 
written.  So wrong, in fact, that if somebody had actually examined it 
properly they would probably have realised the bug that I was fixing was 
present :(.

I think the fix is to change the expected output to 
farcall-thumb2-thumb2.d, but I need to run that change to make sure.

R.

> 
> Can you check?
> 
> Thanks,
> 
> Christophe
> 
>> ---
>>   bfd/elf32-arm.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
>> index 79b94e836fc..cb567fe82ad 100644
>> --- a/bfd/elf32-arm.c
>> +++ b/bfd/elf32-arm.c
>> @@ -3888,9 +3888,11 @@ using_thumb2 (struct elf32_arm_link_hash_table *globals)
>>     int thumb_isa = bfd_elf_get_obj_attr_int (globals->obfd, OBJ_ATTR_PROC,
>>                                              Tag_THUMB_ISA_use);
>>
>> -  if (thumb_isa)
>> +  /* No use of thumb permitted, or a legacy thumb-1/2 definition.  */
>> +  if (thumb_isa < 3)
>>       return thumb_isa == 2;
>>
>> +  /* Variant of thumb is described by the architecture tag.  */
>>     arch = bfd_elf_get_obj_attr_int (globals->obfd, OBJ_ATTR_PROC, Tag_CPU_arch);
>>
>>     /* Force return logic to be reviewed for each new architecture.  */
>> --
>> 2.25.1
>>

Re: Autogenerate ChangeLog commits (was: [committed] arm:...)

[Richard Earnshaw]

On 11/05/2021 21:05, Hans-Peter Nilsson wrote:
> On Tue, 11 May 2021, Christophe Lyon via Binutils wrote:
>> This patch causes a regression in the ld tests:
>> FAIL:Thumb-Thumb farcall v8-M Mainline
>>
>> Can you check?
> 
> Even worse, no ChangeLog entry was committed! ;-)
> 
> Everyone,
> On the more serious side, how about adopting the wonderful gcc
> commit-hook and scripts that takes care of this, already?
> 
> I'm sure most other people, including me, will eventually forget
> about it committing ChangeLog entries too.
> 
> brgds, H-P
> PS. Not a debate about having ChangeLogs at all, please.
> 

Apologies, I thought we /had/ adopted the GCC model, which is why the 
commit log contains

bfd/

	* elf32-arm.c (using_thumb2): Correctly handle Tag_THUMB_ISA_use
	values 0 and 3.

My vote would be for consistency with the other repos; there's nothing 
worse than having subtly different rules for each project.

I can put up with things not being 100% optimal if they are consistent. 
  But not 100% optimal and inconsistent is just a nightmare.

R.

Re: Autogenerate ChangeLog commits

[Jose E. Marchesi]

>>> This patch causes a regression in the ld tests:
>>> FAIL:Thumb-Thumb farcall v8-M Mainline
>>>
>>> Can you check?
>> Even worse, no ChangeLog entry was committed! ;-)
>> Everyone,
>> On the more serious side, how about adopting the wonderful gcc
>> commit-hook and scripts that takes care of this, already?
>> I'm sure most other people, including me, will eventually forget
>> about it committing ChangeLog entries too.
>> brgds, H-P
>> PS. Not a debate about having ChangeLogs at all, please.
>> 
>
> Apologies, I thought we /had/ adopted the GCC model, which is why the
> commit log contains
>
> bfd/
>
> 	* elf32-arm.c (using_thumb2): Correctly handle Tag_THUMB_ISA_use
> 	values 0 and 3.
>
> My vote would be for consistency with the other repos; there's nothing
> worse than having subtly different rules for each project.
>
> I can put up with things not being 100% optimal if they are
> consistent.   But not 100% optimal and inconsistent is just a
> nightmare.

+1 for adopting the GCC changelog related scripts and workflow in
binutils.

Actually, if these scripts would find their way into gnulib I would
certainly use them as well in my other projects.

They make backporting almost a pleasurable experience ;)

[committed] arm: fix fallout from recent thumb2 detection patch

[Richard Earnshaw]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]


The recent change to correct the detection of thumb2 object files
resulted in a ld test for veneering starting to fail.  The problem was
the test itself, which was incorrectly expecting thumb1 type far-call
veneers instead of the thumb2 flavour.  We already have a dump file of
the expected form, so the fix is to change the expected output
accordingly.

ld/

	* testsuite/ld-arm/arm-elf.exp (farcall test for v8-m.mainline):
	Correct expected output.
---
 ld/ChangeLog                    | 5 +++++
 ld/testsuite/ld-arm/arm-elf.exp | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-arm-fix-fallout-from-recent-thumb2-detection-patch.patch --]
[-- Type: text/x-patch; name="0001-arm-fix-fallout-from-recent-thumb2-detection-patch.patch", Size: 1164 bytes --]

diff --git a/ld/ChangeLog b/ld/ChangeLog
index eec797754ed..77b10eb86df 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,3 +1,8 @@
+2021-05-12  Richard Earnshaw  <rearnsha@arm.com>
+
+	* testsuite/ld-arm/arm-elf.exp (farcall test for v8-m.mainline):
+	Correct expected output.
+
 2021-05-11  Roland McGrath  <mcgrathr@google.com>
 
 	* testsuite/ld-x86-64/rela.d: Fix regexp not to presume a specific
diff --git a/ld/testsuite/ld-arm/arm-elf.exp b/ld/testsuite/ld-arm/arm-elf.exp
index ea1d1e1345a..9bad9b8859b 100644
--- a/ld/testsuite/ld-arm/arm-elf.exp
+++ b/ld/testsuite/ld-arm/arm-elf.exp
@@ -575,7 +575,7 @@ set armeabitests_nonacl {
      {{objdump -d farcall-thumb-thumb-m.d}}
      "farcall-thumb-thumb-v8-m-base"}
     {"Thumb-Thumb farcall v8-M Mainline" "-Ttext 0x1000 --section-start .foo=0x2001014" "" "-march=armv8-m.main" {farcall-thumb-thumb.s}
-     {{objdump -d farcall-thumb-thumb-m.d}}
+     {{objdump -d farcall-thumb2-thumb2-m.d}}
      "farcall-thumb-thumb-v8-m-main"}
     {"Thumb-Thumb farcall v6-M" "-Ttext 0x1000 --section-start .foo=0x2001014" "" "-march=armv6-m" {farcall-thumb-thumb.s}
      {{objdump -d farcall-thumb-thumb-m.d}}

Re: Autogenerate ChangeLog commits (was: [committed] arm:...)

[Alan Modra]

On Wed, May 12, 2021 at 10:54:25AM +0100, Richard Earnshaw via Binutils wrote:
> On 11/05/2021 21:05, Hans-Peter Nilsson wrote:
> > On the more serious side, how about adopting the wonderful gcc
> > commit-hook and scripts that takes care of this, already?
> 
> My vote would be for consistency with the other repos; there's nothing worse
> than having subtly different rules for each project.
> 
> I can put up with things not being 100% optimal if they are consistent.  But
> not 100% optimal and inconsistent is just a nightmare.

I agree, and would be happy to follow the gcc lead here.  I'm sure
there will be things that might be annoying on some occasions.  For
example, the gcc commit hook insistence that all files changed be
mentioned in the changelog (or there be no changelog entry).  But that
sort of checking is often a good thing, and I'm willing to put up with
the times it is annoying.

-- 
Alan Modra
Australia Development Lab, IBM