Re: [PATCH] PR gold/17640

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

On Thu, Mar 5, 2015 at 2:40 AM, Ilya Tocar <tocarip.intel@gmail.com> wrote:
> On 27 Feb 07:01, H.J. Lu wrote:
>> 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.
>>
> Good catch!
> Fixed. I've also added tests for _DYNAMIC, pie, Bsymbolic.
> Ok for trunk?
>
> ---
>  gold/ChangeLog                    |  15 +++++
>  gold/i386.cc                      | 121 ++++++++++++++++++++++++++++----------
>  gold/testsuite/Makefile.am        |  51 ++++++++++++++++
>  gold/testsuite/i386_mov_to_lea.sh |  36 ++++++++++++
>  gold/testsuite/i386_mov_to_lea1.s |  11 ++++
>  gold/testsuite/i386_mov_to_lea2.s |  10 ++++
>  gold/testsuite/i386_mov_to_lea3.s |   4 ++
>  gold/testsuite/i386_mov_to_lea4.s |  12 ++++
>  8 files changed, 228 insertions(+), 32 deletions(-)
>  create mode 100755 gold/testsuite/i386_mov_to_lea.sh
>  create mode 100644 gold/testsuite/i386_mov_to_lea1.s
>  create mode 100644 gold/testsuite/i386_mov_to_lea2.s
>  create mode 100644 gold/testsuite/i386_mov_to_lea3.s
>  create mode 100644 gold/testsuite/i386_mov_to_lea4.s
>
> diff --git a/gold/ChangeLog b/gold/ChangeLog
> index fe6a56b..0abe83e 100644
> --- a/gold/ChangeLog
> +++ b/gold/ChangeLog
> @@ -1,3 +1,18 @@
> +2015-03-05  Ilya Tocar  <ilya.tocar@intel.com>
> +
> +       PR gold/17640
> +       * i386.cc (Target_i386::Scan::local): Don't create GOT entry, when we
> +       can convert GOT to GOTOFF.
> +       (Target_i386::Scan::global): Ditto.
> +       (Target_i386::Relocate::relocate): Convert  mov foo@GOT(%reg), %reg to
> +       lea foo@GOTOFF(%reg), %reg if possible.
> +       * testsuite/Makefile.am (i386_mov_to_lea): New test.
> +       * testsuite/i386_mov_to_lea1.s: New.
> +       * testsuite/i386_mov_to_lea2.s: Ditto.
> +       * testsuite/i386_mov_to_lea3.s: Ditto.
> +       * testsuite/i386_mov_to_lea4.s: Ditto.
> +       * testsuite/i386_mov_to_lea.sh: Ditto.
> +
>  2015-03-04  Cary Coutant  <ccoutant@google.com>
>
>         * parameters.cc (Parameters::set_target_once): Call
> diff --git a/gold/i386.cc b/gold/i386.cc
> index 24f4103..96ba773 100644
> --- a/gold/i386.cc
> +++ b/gold/i386.cc
> @@ -1835,8 +1835,28 @@ Target_i386::Scan::local(Symbol_table* symtab,
>
>      case elfcpp::R_386_GOT32:
>        {
> -       // The symbol requires a GOT entry.
> +
> +       // We need GOT section.
>         Output_data_got<32, false>* got = target->got_section(symtab, layout);
> +
> +       // 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 (reloc.get_r_offset() >= 2
> +           && lsym.get_st_type() != elfcpp::STT_GNU_IFUNC)
> +         {
> +           section_size_type stype;
> +           const unsigned char* view = object->section_contents(data_shndx,
> +                                                                &stype, true);
> +           if (view[reloc.get_r_offset() - 2] == 0x8b)
> +             break;
> +
> +         }
> +
> +       // Otherwise, the symbol requires a GOT entry.
>         unsigned int r_sym = elfcpp::elf_r_sym<32>(reloc.get_r_info());
>
>         // For a STT_GNU_IFUNC symbol we want the PLT offset.  That
> @@ -2229,8 +2249,33 @@ Target_i386::Scan::global(Symbol_table* symtab,
>
>      case elfcpp::R_386_GOT32:
>        {
> +
>         // The symbol requires a GOT entry.
>         Output_data_got<32, false>* got = target->got_section(symtab, layout);
> +
> +       // 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 its address may be used.
> +       if (reloc.get_r_offset() >= 2
> +           && gsym->type() != elfcpp::STT_GNU_IFUNC
> +           && !gsym->is_undefined()
> +           && !gsym->is_from_dynobj()
> +           && !gsym->is_preemptible()
> +           && (!parameters->options().shared()
> +               || gsym->visibility() != elfcpp::STV_DEFAULT
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Protected symbol means that it can't be pre-emptied.  It
doesn't mean its address won't be external.  This is true
for pointer to protected function.  With copy relocation,
address of protected data defined in the shared library may
also be external.  We only know that for sure at run-time.

See:

https://sourceware.org/ml/binutils/2015-03/msg00036.html
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248

> +               || parameters->options().Bsymbolic())
> +           && strcmp(gsym->name(), "_DYNAMIC"))
>



-- 
H.J.