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

Message ID 20210511152304.3905930-1-rearnsha@arm.com
State New
Headers show
Series
  • [committed] arm: correctly decode Tag_THUMB_ISA_use=3 for thumb2 features
Related show

Commit Message

H.J. Lu via Binutils May 11, 2021, 3:23 p.m.
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(-)

-- 
2.25.1

Comments

H.J. Lu via Binutils May 11, 2021, 6:52 p.m. | #1
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

>
Hans-Peter Nilsson May 11, 2021, 8:05 p.m. | #2
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.
Tom Tromey May 11, 2021, 8:27 p.m. | #3
>>>>> "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
H.J. Lu via Binutils May 12, 2021, 9:51 a.m. | #4
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

>>
H.J. Lu via Binutils May 12, 2021, 9:54 a.m. | #5
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.
H.J. Lu via Binutils May 12, 2021, 10:17 a.m. | #6
>>> 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 ;)
H.J. Lu via Binutils May 13, 2021, 2:41 a.m. | #7
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

Patch

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.  */