[ARM] Fix ld.so crash when built using Binutils 2.29

[ARM] Fix ld.so crash when built using Binutils 2.29

[Jiong Wang]

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

Hi,

   There is bug report that ld.so in GLIBC 2.24 built by Binutils 2.29 will crash
on arm-linux-gnueabihf.  This is confirmed, and the details is at:

    https://sourceware.org/bugzilla/show_bug.cgi?id=21725.

   And I could also reproduce this crash using GLIBC master.

   As analyzed in the PR, the old code was with the assumption that assembler
won't set bit0 of thumb function address if it comes from PC-relative
instructions and the calculation can be finished during assembling.  This
assumption however does not hold after PR gas/21458.

   I think ARM backend in GLIBC should be fix to be more portable so it could
work with various combinations of GLIBC and Binutils.

   OK for master and backport to all release branches?


2017-07-12  Jiong Wang  <jiong.wang@arm.com>

         * sysdeps/arm/dl-machine.h (elf_machine_load_address):  Also strip bit 0
         of pcrel_address under Thumb mode.


[-- Attachment #2: fix-arm.patch --]
[-- Type: text/x-patch, Size: 1177 bytes --]

diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
index 7053ead16ed0e7dac182660f7d88fa21f2b4799a..5b67e3d004818308d9bf93effb13d23a762e160f 100644
--- a/sysdeps/arm/dl-machine.h
+++ b/sysdeps/arm/dl-machine.h
@@ -56,11 +56,19 @@ elf_machine_load_address (void)
   extern Elf32_Addr internal_function __dl_start (void *) asm ("_dl_start");
   Elf32_Addr got_addr = (Elf32_Addr) &__dl_start;
   Elf32_Addr pcrel_addr;
+  asm ("adr %0, _dl_start" : "=r" (pcrel_addr));
 #ifdef __thumb__
-  /* Clear the low bit of the funciton address.  */
+  /* Clear the low bit of the funciton address.
+
+     NOTE: got_addr is from GOT table whose lsb is always set by linker if it's
+     Thumb function address.  PCREL_ADDR comes from PC-relative calculation
+     which will finish during assembling.  GAS assembler before the fix for
+     PR gas/21458 was not setting the lsb but does after that.  Always do the
+     strip for both, so the code works with various combinations of glibc and
+     Binutils.  */
   got_addr &= ~(Elf32_Addr) 1;
+  pcrel_addr &= ~(Elf32_Addr) 1;
 #endif
-  asm ("adr %0, _dl_start" : "=r" (pcrel_addr));
   return pcrel_addr - got_addr;
 }
 

Re: [ARM] Fix ld.so crash when built using Binutils 2.29

[Ramana Radhakrishnan]

On Wed, Jul 12, 2017 at 5:13 PM, Jiong Wang <jiong.wang@foss.arm.com> wrote:
> Hi,
>
>   There is bug report that ld.so in GLIBC 2.24 built by Binutils 2.29 will
> crash
> on arm-linux-gnueabihf.  This is confirmed, and the details is at:
>
>    https://sourceware.org/bugzilla/show_bug.cgi?id=21725.
>
>   And I could also reproduce this crash using GLIBC master.
>
>   As analyzed in the PR, the old code was with the assumption that assembler
> won't set bit0 of thumb function address if it comes from PC-relative
> instructions and the calculation can be finished during assembling.  This
> assumption however does not hold after PR gas/21458.
>
>   I think ARM backend in GLIBC should be fix to be more portable so it could
> work with various combinations of GLIBC and Binutils.
>
>   OK for master and backport to all release branches?

Has a combination of a binutils that did not have the fix for 21458 +
glibc with this patch been tested ?

Ramana
>
>
> 2017-07-12  Jiong Wang  <jiong.wang@arm.com>
>
>         * sysdeps/arm/dl-machine.h (elf_machine_load_address):  Also strip
> bit 0
>         of pcrel_address under Thumb mode.
>

Re: [ARM] Fix ld.so crash when built using Binutils 2.29

[Jiong Wang]

On 12/07/17 17:23, Ramana Radhakrishnan wrote:
> On Wed, Jul 12, 2017 at 5:13 PM, Jiong Wang <jiong.wang@foss.arm.com> wrote:
>> Hi,
>>
>>    There is bug report that ld.so in GLIBC 2.24 built by Binutils 2.29 will
>> crash
>> on arm-linux-gnueabihf.  This is confirmed, and the details is at:
>>
>>     https://sourceware.org/bugzilla/show_bug.cgi?id=21725.
>>
>>    And I could also reproduce this crash using GLIBC master.
>>
>>    As analyzed in the PR, the old code was with the assumption that assembler
>> won't set bit0 of thumb function address if it comes from PC-relative
>> instructions and the calculation can be finished during assembling.  This
>> assumption however does not hold after PR gas/21458.
>>
>>    I think ARM backend in GLIBC should be fix to be more portable so it could
>> work with various combinations of GLIBC and Binutils.
>>
>>    OK for master and backport to all release branches?
> Has a combination of a binutils that did not have the fix for 21458 +
> glibc with this patch been tested ?

It has been tested.

PR gas/21458 will set lsb of the address, so now the lsb may or may not be set and this
depends on whether you are using old or new Binutils.  Always strip "pcrel_addr" makes
its lsb status synced with "got_addr".

>
> Ramana
>>
>> 2017-07-12  Jiong Wang  <jiong.wang@arm.com>
>>
>>          * sysdeps/arm/dl-machine.h (elf_machine_load_address):  Also strip
>> bit 0
>>          of pcrel_address under Thumb mode.
>>

Re: [ARM] Fix ld.so crash when built using Binutils 2.29

[Phil Blundell]

On Wed, 2017-07-12 at 17:13 +0100, Jiong Wang wrote:
> 

> 2017-07-12  Jiong Wang  <jiong.wang@arm.com>
> 
>          * sysdeps/arm/dl-machine.h (elf_machine_load_address):  Also
> strip bit 0
>          of pcrel_address under Thumb mode.

It seems a bit unfortunate that the semantics of the ADR instruction
have changed in this way (after nearly two decades of the old
behaviour) but having reviewed the GAS bug report again I do agree with
Nick's rationale for doing that.  And this patch seems like a
reasonable way of dealing with it in glibc.  So, assuming you have
tested this with both old and new binutils, it is OK.

Thanks

Phil

Re: [ARM] Fix ld.so crash when built using Binutils 2.29

[Carlos O'Donell]

On 07/13/2017 07:41 AM, Phil Blundell wrote:
> On Wed, 2017-07-12 at 17:13 +0100, Jiong Wang wrote:
>>
> 
>> 2017-07-12  Jiong Wang  <jiong.wang@arm.com>
>>
>>          * sysdeps/arm/dl-machine.h (elf_machine_load_address):  Also
>> strip bit 0
>>          of pcrel_address under Thumb mode.
> 
> It seems a bit unfortunate that the semantics of the ADR instruction
> have changed in this way (after nearly two decades of the old
> behaviour) but having reviewed the GAS bug report again I do agree with
> Nick's rationale for doing that.  And this patch seems like a
> reasonable way of dealing with it in glibc.  So, assuming you have
> tested this with both old and new binutils, it is OK.

$0.02.

I'm already running with this patch in Fedora Rawhide for our Fedora 27
32-bit ARM builds and it fixes the issue with no regressions.

-- 
Cheers,
Carlos.