This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] PR gold/17640
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Ilya Tocar <tocarip dot intel at gmail dot com>
- Cc: Cary Coutant <ccoutant at google dot com>, Ian Lance Taylor <iant at google dot com>, Binutils <binutils at sourceware dot org>
- Date: Fri, 27 Feb 2015 07:01:42 -0800
- Subject: Re: [PATCH] PR gold/17640
- Authentication-results: sourceware.org; auth=none
- References: <20150202134701 dot GA91020 at msticlxl7 dot ims dot intel dot com> <CAMe9rOpzSBoiykoFG+YuAK8QYsguqzZNrvj2sE7EhafUzOjJJw at mail dot gmail dot com> <20150218150011 dot GA40373 at msticlxl7 dot ims dot intel dot com> <CAMe9rOqGecgvSMs937a2b57mGjp-ZF5XuULQoAx3Tr9NG75GcA at mail dot gmail dot com> <20150219142707 dot GA507 at msticlxl7 dot ims dot intel dot com> <CAMe9rOp+6-mKKqE4h1jUvt-wBWVU0YK62yJPgrN0LVuU4JhxRw at mail dot gmail dot com> <CAHACq4poMBVYcg=nS01tPsLuNi=BdtL4gSTz5Q1-auGBX=zA-Q at mail dot gmail dot com> <20150226104626 dot GA16554 at msticlxl7 dot ims dot intel dot com> <CAHACq4ojyujhxYSsMaF_jSVWUg0=YgNawR6=6XYdZie1PMhXYQ at mail dot gmail dot com> <20150227142003 dot GA122934 at msticlxl7 dot ims dot intel dot com> <CAMe9rOrzXOu+CkT_A7XZLJ1MK_nqnMk69LjgXUSQ-FnS1fKsEQ at mail dot 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.