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 06:31:24 -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>
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.
--
H.J.