Re: [PATCH] PR gold/17640

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Feb 27, 2015 at 6:31 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Feb 27, 2015 at 6:20 AM, Ilya Tocar <tocarip.intel@gmail.com> wrote:
>> On 26 Feb 10:15, Cary Coutant wrote:
>>> Thanks for working on this!
>>>
>>> Please write a ChangeLog entry.
>>>
>> Done.
>>
>>> > +       // If the relocation symbol isn't IFUNC,
>>> > +       // and is local, then we will convert
>>> > +       // mov foo@GOT(%reg), %reg
>>> > +       // to
>>> > +       // lea foo@GOTOFF(%reg), %reg
>>> > +       // in Relocate::relocate
>>> > +       if (view[reloc.get_r_offset() - 2] == 0x8b
>>>
>>> You also need to check that reloc.get_r_offset() >= 2. If that's
>>> false, or if the symbol is an IFUNC symbol, you could avoid getting
>>> the section contents.
>>>
>> Done.
>>
>>> > +       // If we convert this from
>>> > +       // mov foo@GOT(%reg), %reg
>>> > +       // to
>>> > +       // lea foo@GOTOFF(%reg), %reg
>>> > +       // in Relocate::relocate, then there is nothing to do here.
>>> > +       // Avoid converting _DYNAMIC, because it's address may be used.
>>> > +       if (view[reloc.get_r_offset() - 2] == 0x8b
>>> > +           && gsym->type() != elfcpp::STT_GNU_IFUNC
>>> > +           && !gsym->is_undefined()
>>> > +           && !gsym->is_from_dynobj()
>>> > +           && strcmp(gsym->name(), "_DYNAMIC"))
>>> > +         break;
>>>
>>> Same here.
>>>
>>> s/it's/its/
>>>
>> Fixed.
>>
>>> Could you explain more about the exception for _DYNAMIC? If its
>>> address is used by some other relocation, won't we end up creating the
>>> GOT entry anyway when we process that relocation? And if it's not,
>>> isn't it OK to omit the GOT entry?
>>>
>> Comment in bfd/elf32-i386.c (elf_i386_convert_mov_to_lea:2714) says:
>>
>> We also avoid optimizing _DYNAMIC since ld.so may use its link-time
>> address.
>>
>> I've checked mov1 tests in ld/testsuite/ld-i386/
>> and without this check we optimize it (incorrectly) away.
>>
>>> > +      // If the relocation symbol isn't IFUNC,
>>> > +      // and is local, then we convert
>>> > +      // mov foo@GOT(%reg), %reg
>>> > +      // to
>>> > +      // lea foo@GOTOFF(%reg), %reg
>>> > +      if (view[-2] == 0x8b
>>>
>>> Again, you need to check that r_offset is in range. See calls to
>>> tls::check_tls() later in the source file.
>>>
>> Check added.
>>
>>> > +         && ((gsym == NULL && !psymval->is_ifunc_symbol())
>>> > +             || (gsym != NULL
>>> > +                 && gsym->type() != elfcpp::STT_GNU_IFUNC
>>> > +                 && !gsym->is_from_dynobj()
>>> > +                 && !gsym->is_undefined())))
>>>
>>> What about _DYNAMIC? You need to make sure you make the same decision
>>> here that you made in Scan::local or Scan::global.
>> Why? What's wrong with optimizing access into lea, but leaving GOT
>> entry?
>
> You should add a testcase for  _DYNAMIC, as in
>
> ommit 3f65f59941a8cf0895384bc4700f41a2f37e1ff2
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date:   Sat Sep 1 02:50:14 2012 +0000
>
>     Don't optimize relocation against _DYNAMIC
>
>     bfd/
>
>       * elf32-i386.c (elf_i386_convert_mov_to_lea): Don't optimize
>       _DYNAMIC.
>       * elf64-x86-64.c (elf_x86_64_convert_mov_to_lea): Likewise.
>
>     ld/testsuite/
>
>       * ld-i386/i386.exp: Run mov1a, mov1b.
>       * ld-x86-64/x86-64.exp: Run mov1a, mov1b, mov1c, mov1d.
>
>       * ld-i386/mov1.s: New file.
>       * ld-i386/mov1a.d: Likewise.
>       * ld-i386/mov1b.d: Likewise.
>       * ld-x86-64/mov1.s: Likewise.
>       * ld-x86-64/mov1a.d: Likewise.
>       * ld-x86-64/mov1b.d: Likewise.
>

You should also add testcases for PIE, -Bsymbolic with DSO
and normal executable.  You also need testcases with
relocation against global symbol with and without visibility,.

Your patch incorrectly converts mov to lea with global symbol
defined within DSO when building DSO.



-- 
H.J.