Re: [PATCH][PING] PR gold/17640

[Ilya Tocar <tocarip dot intel at gmail dot com>]

Ping.

On 23 Mar 18:01, Ilya Tocar wrote:
> Ping.
> 
> On 18 Mar 00:40, Ilya Tocar wrote:
> > On 12 Mar 15:08, Cary Coutant wrote:
> > > + // 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.
> > > 
> > > I'm sorry, but I still don't understand the rationale for treating
> > > _DYNAMIC specially. If you convert all of the references to @GOTOFF,
> > > what's the point of having the GOT entry? If the loader is going to
> > > use it, how does it find it?
> > > 
> > > At any rate, the comment here is wrong, because you *will* convert mov
> > > to lea for this reference -- all you're doing here is keeping the GOT
> > > entry, even though you're not going to use it.
> > >
> > As discussed, updated version leaves lea, because we will use
> > linktime address of _DYNAMIC.
> > 
> > > +    && strcmp(gsym->name(), "_DYNAMIC"))
> > > 
> > > strcmp doesn't return a boolean; I'd prefer to see an explicit "!= 0".
> > >
> > Fixed.
> > > And later, in Relocate::relocate, you cut & pasted the comment from Scan::local:
> > > 
> > > +      // If the relocation symbol isn't IFUNC,
> > > +      // and is local, then we convert
> > > +      // mov foo@GOT(%reg), %reg
> > > +      // to
> > > +      // lea foo@GOTOFF(%reg), %reg
> > > 
> > > But here, you're checking for either local or global relocations.
> > > 
> > Fixed.
> > > +      if (rel.get_r_offset() >= 2
> > > +         && view[-2] == 0x8b
> > > +         && ((gsym == NULL && !psymval->is_ifunc_symbol())
> > > +             || (gsym != NULL
> > > +                 && gsym->type() != elfcpp::STT_GNU_IFUNC
> > > +                 && !gsym->is_from_dynobj()
> > > +                 && !gsym->is_undefined ()
> > > +                 && (!parameters->options().shared()
> > > +                     || (gsym->visibility() != elfcpp::STV_DEFAULT
> > > +                         && gsym->visibility() != elfcpp::STV_PROTECTED)
> > > +                     || parameters->options().Bsymbolic())
> > > +                 && !gsym->is_preemptible())))
> > > 
> > > This is a complicated condition, and it shares a lot with the
> > > condition in Scan::global(). I'd still like to see the common parts of
> > > the two refactored into a predicate function.
> > >
> > I've intoduced can_convert_mov_to_lea.
> > 
> > Ok for trunk?
> > 
> > ---
> >  gold/ChangeLog                    |  16 +++++
> >  gold/i386.cc                      | 127 ++++++++++++++++++++++++++++----------
> >  gold/testsuite/Makefile.am        |  56 +++++++++++++++++
> >  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 ++++
> >  gold/testsuite/i386_mov_to_lea5.s |  12 ++++
> >  9 files changed, 251 insertions(+), 33 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
> >  create mode 100644 gold/testsuite/i386_mov_to_lea5.s
> > 
> > diff --git a/gold/ChangeLog b/gold/ChangeLog
> > index f94e170..1124b2f 100644
> > --- a/gold/ChangeLog
> > +++ b/gold/ChangeLog
> > @@ -1,3 +1,19 @@
> > +2015-03-18  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_lea5.s: Ditto.
> > +	* testsuite/i386_mov_to_lea.sh: Ditto.
> > +
> >  2015-03-11  Cary Coutant  <ccoutant@google.com>
> >  
> >  	* options.cc (General_options::finalize): Don't allow -z relro
> > diff --git a/gold/i386.cc b/gold/i386.cc
> > index 24f4103..3cbcb4b 100644
> > --- a/gold/i386.cc
> > +++ b/gold/i386.cc
> > @@ -738,6 +738,26 @@ class Target_i386 : public Sized_target<32, false>
> >    static tls::Tls_optimization
> >    optimize_tls_reloc(bool is_final, int r_type);
> >  
> > +  // Check if relocation against this symbol is a candidate for
> > +  // conversion from
> > +  // mov foo@GOT(%reg), %reg
> > +  // to
> > +  // lea foo@GOTOFF(%reg), %reg
> > +  static bool
> > +  can_convert_mov_to_lea(const Symbol* gsym)
> > +  {
> > +    gold_assert(gsym != NULL);
> > +    return (gsym->type() != elfcpp::STT_GNU_IFUNC
> > +	    && !gsym->is_undefined ()
> > +	    && !gsym->is_from_dynobj()
> > +	    && !gsym->is_preemptible()
> > +	    && (!parameters->options().shared()
> > +		|| (gsym->visibility() != elfcpp::STV_DEFAULT
> > +		    && gsym->visibility() != elfcpp::STV_PROTECTED)
> > +		|| parameters->options().Bsymbolic())
> > +	    && strcmp(gsym->name(), "_DYNAMIC") != 0);
> > +  }
> > +
> >    // Get the GOT section, creating it if necessary.
> >    Output_data_got<32, false>*
> >    got_section(Symbol_table*, Layout*);
> > @@ -1835,8 +1855,26 @@ 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 +2267,24 @@ Target_i386::Scan::global(Symbol_table* symtab,
> >  
> >      case elfcpp::R_386_GOT32:
> >        {
> > -	// The symbol requires a GOT entry.
> > +	// The symbol requires a GOT section.
> >  	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.
> > +	if (reloc.get_r_offset() >= 2
> > +	    && Target_i386::can_convert_mov_to_lea (gsym))
> > +	  {
> > +	    section_size_type stype;
> > +	    const unsigned char* view = object->section_contents(data_shndx,
> > +								 &stype, true);
> > +	    if (view[reloc.get_r_offset() - 2] == 0x8b)
> > +	      break;
> > +	  }
> > +
> >  	if (gsym->final_value_is_known())
> >  	  {
> >  	    // For a STT_GNU_IFUNC symbol we want the PLT address.
> > @@ -2732,35 +2786,6 @@ Target_i386::Relocate::relocate(const Relocate_info<32, false>* relinfo,
> >  	}
> >      }
> >  
> > -  // Get the GOT offset if needed.
> > -  // The GOT pointer points to the end of the GOT section.
> > -  // We need to subtract the size of the GOT section to get
> > -  // the actual offset to use in the relocation.
> > -  bool have_got_offset = false;
> > -  unsigned int got_offset = 0;
> > -  switch (r_type)
> > -    {
> > -    case elfcpp::R_386_GOT32:
> > -      if (gsym != NULL)
> > -	{
> > -	  gold_assert(gsym->has_got_offset(GOT_TYPE_STANDARD));
> > -	  got_offset = (gsym->got_offset(GOT_TYPE_STANDARD)
> > -			- target->got_size());
> > -	}
> > -      else
> > -	{
> > -	  unsigned int r_sym = elfcpp::elf_r_sym<32>(rel.get_r_info());
> > -	  gold_assert(object->local_has_got_offset(r_sym, GOT_TYPE_STANDARD));
> > -	  got_offset = (object->local_got_offset(r_sym, GOT_TYPE_STANDARD)
> > -			- target->got_size());
> > -	}
> > -      have_got_offset = true;
> > -      break;
> > -
> > -    default:
> > -      break;
> > -    }
> > -
> >    switch (r_type)
> >      {
> >      case elfcpp::R_386_NONE:
> > @@ -2809,8 +2834,44 @@ Target_i386::Relocate::relocate(const Relocate_info<32, false>* relinfo,
> >        break;
> >  
> >      case elfcpp::R_386_GOT32:
> > -      gold_assert(have_got_offset);
> > -      Relocate_functions<32, false>::rel32(view, got_offset);
> > +      // Convert
> > +      // mov foo@GOT(%reg), %reg
> > +      // to
> > +      // lea foo@GOTOFF(%reg), %reg
> > +      // if possible
> > +      if (rel.get_r_offset() >= 2
> > +	  && view[-2] == 0x8b
> > +	  && ((gsym == NULL && !psymval->is_ifunc_symbol())
> > +	      || (gsym != NULL
> > +		  && Target_i386::can_convert_mov_to_lea(gsym))))
> > +	{
> > +	  view[-2] = 0x8d;
> > +	  elfcpp::Elf_types<32>::Elf_Addr value;
> > +	  value = (psymval->value(object, 0)
> > +	           - target->got_plt_section()->address());
> > +	  Relocate_functions<32, false>::rel32(view, value);
> > +	}
> > +      else
> > +	{
> > +	  // The GOT pointer points to the end of the GOT section.
> > +	  // We need to subtract the size of the GOT section to get
> > +	  // the actual offset to use in the relocation.
> > +	  unsigned int got_offset = 0;
> > +	  if (gsym != NULL)
> > +	    {
> > +	      gold_assert(gsym->has_got_offset(GOT_TYPE_STANDARD));
> > +	      got_offset = (gsym->got_offset(GOT_TYPE_STANDARD)
> > +			    - target->got_size());
> > +	    }
> > +	  else
> > +	    {
> > +	      unsigned int r_sym = elfcpp::elf_r_sym<32>(rel.get_r_info());
> > +	      gold_assert(object->local_has_got_offset(r_sym, GOT_TYPE_STANDARD));
> > +	      got_offset = (object->local_got_offset(r_sym, GOT_TYPE_STANDARD)
> > +			    - target->got_size());
> > +	    }
> > +	  Relocate_functions<32, false>::rel32(view, got_offset);
> > +	}
> >        break;
> >  
> >      case elfcpp::R_386_GOTOFF:
> > diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
> > index 43da146..0ea57d2 100644
> > --- a/gold/testsuite/Makefile.am
> > +++ b/gold/testsuite/Makefile.am
> > @@ -957,6 +957,62 @@ endif FN_PTRS_IN_SO_WITHOUT_PIC
> >  
> >  endif TLS
> >  
> > +if DEFAULT_TARGET_I386
> > +
> > +check_SCRIPTS += i386_mov_to_lea.sh
> > +check_DATA += i386_mov_to_lea1.stdout  i386_mov_to_lea2.stdout \
> > +	i386_mov_to_lea3.stdout  i386_mov_to_lea4.stdout \
> > +	i386_mov_to_lea5.stdout i386_mov_to_lea6.stdout \
> > +	i386_mov_to_lea7.stdout i386_mov_to_lea8.stdout
> > +MOSTLYCLEANFILES += i386_mov_to_lea1 i386_mov_to_lea2 i386_mov_to_lea3 \
> > +	i386_mov_to_lea4 i386_mov_to_lea5 i386_mov_to_lea6 \
> > +	i386_mov_to_lea7 i386_mov_to_lea8
> > +
> > +i386_mov_to_lea1.o: i386_mov_to_lea1.s
> > +	$(TEST_AS) --32 -o $@ $<
> > +i386_mov_to_lea2.o: i386_mov_to_lea2.s
> > +	$(TEST_AS) --32 -o $@ $<
> > +i386_mov_to_lea3.o: i386_mov_to_lea3.s
> > +	$(TEST_AS) --32 -o $@ $<
> > +i386_mov_to_lea4.o: i386_mov_to_lea4.s
> > +	$(TEST_AS) --32 -o $@ $<
> > +i386_mov_to_lea5.o: i386_mov_to_lea5.s
> > +	$(TEST_AS) --32 -o $@ $<
> > +i386_mov_to_lea1: i386_mov_to_lea1.o
> > +	../ld-new -Bsymbolic -shared -melf_i386  -o $@ $<
> > +i386_mov_to_lea2: i386_mov_to_lea1.o
> > +	../ld-new  -pie -melf_i386  -o $@ $<
> > +i386_mov_to_lea3: i386_mov_to_lea1.o
> > +	../ld-new -melf_i386  -o $@ $<
> > +i386_mov_to_lea4: i386_mov_to_lea1.o
> > +	../ld-new -melf_i386 -shared  -o $@ $<
> > +i386_mov_to_lea5: i386_mov_to_lea2.o
> > +	../ld-new -melf_i386 -shared  -o $@ $<
> > +i386_mov_to_lea6: i386_mov_to_lea3.o
> > +	../ld-new -melf_i386 -shared  -o $@ $<
> > +i386_mov_to_lea7: i386_mov_to_lea4.o
> > +	../ld-new -melf_i386 -shared  -o $@ $<
> > +i386_mov_to_lea8: i386_mov_to_lea5.o
> > +	../ld-new -melf_i386 -shared  -o $@ $<
> > +i386_mov_to_lea1.stdout: i386_mov_to_lea1
> > +	$(TEST_OBJDUMP) -dw $< > $@
> > +i386_mov_to_lea2.stdout: i386_mov_to_lea2
> > +	$(TEST_OBJDUMP) -dw $< > $@
> > +i386_mov_to_lea3.stdout: i386_mov_to_lea3
> > +	$(TEST_OBJDUMP) -dw $< > $@
> > +i386_mov_to_lea4.stdout: i386_mov_to_lea4
> > +	$(TEST_OBJDUMP) -dw $< > $@
> > +i386_mov_to_lea5.stdout: i386_mov_to_lea5
> > +	$(TEST_OBJDUMP) -dw $< > $@
> > +i386_mov_to_lea6.stdout: i386_mov_to_lea6
> > +	$(TEST_OBJDUMP) -dw $< > $@
> > +i386_mov_to_lea7.stdout: i386_mov_to_lea7
> > +	$(TEST_OBJDUMP) -dw $< > $@
> > +i386_mov_to_lea8.stdout: i386_mov_to_lea8
> > +	$(TEST_OBJDUMP) -dw $< > $@
> > +
> > +endif DEFAULT_TARGET_I386
> > +
> >  check_PROGRAMS += many_sections_test
> >  many_sections_test_SOURCES = many_sections_test.cc
> >  many_sections_test_DEPENDENCIES = gcctestdir/ld
> > diff --git a/gold/testsuite/i386_mov_to_lea.sh b/gold/testsuite/i386_mov_to_lea.sh
> > new file mode 100755
> > index 0000000..0370372
> > --- /dev/null
> > +++ b/gold/testsuite/i386_mov_to_lea.sh
> > @@ -0,0 +1,36 @@
> > +#!/bin/sh
> > +
> > +# i386_mov_to_lea.sh -- a test for mov2lea conversion.
> > +
> > +# Copyright (C) 2010-2015 Free Software Foundation, Inc.
> > +# Written by Tocar Ilya <ilya.tocar@intel.com>
> > +
> > +# This file is part of gold.
> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write to the Free Software
> > +# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
> > +# MA 02110-1301, USA.
> > +
> > +set -e
> > +
> > +grep -q "lea    -0x[a-f0-9]\+(%ecx),%eax" i386_mov_to_lea1.stdout
> > +grep -q "lea    -0x[a-f0-9]\+(%ecx),%eax" i386_mov_to_lea2.stdout
> > +grep -q "lea    -0x[a-f0-9]\+(%ecx),%eax" i386_mov_to_lea3.stdout
> > +grep -q "mov    -0x[a-f0-9]\+(%ecx),%eax" i386_mov_to_lea4.stdout
> > +grep -q "lea    -0x[a-f0-9]\+(%ecx),%eax" i386_mov_to_lea5.stdout
> > +grep -q "mov    -0x[a-f0-9]\+(%ecx),%eax" i386_mov_to_lea6.stdout
> > +grep -q "lea    -0x[a-f0-9]\+(%ecx),%eax" i386_mov_to_lea7.stdout
> > +grep -q "mov    -0x[a-f0-9]\+(%ecx),%eax" i386_mov_to_lea8.stdout
> > +
> > +exit 0
> > diff --git a/gold/testsuite/i386_mov_to_lea1.s b/gold/testsuite/i386_mov_to_lea1.s
> > new file mode 100644
> > index 0000000..6d0f436
> > --- /dev/null
> > +++ b/gold/testsuite/i386_mov_to_lea1.s
> > @@ -0,0 +1,11 @@
> > +	.text
> > +	.globl	foo
> > +	.type	foo, @function
> > +foo:
> > +	ret
> > +	.size	foo, .-foo
> > +	.globl	bar
> > +	.type	bar, @function
> > +bar:
> > +	movl	foo@GOT(%ecx), %eax
> > +	.size	bar, .-bar
> > diff --git a/gold/testsuite/i386_mov_to_lea2.s b/gold/testsuite/i386_mov_to_lea2.s
> > new file mode 100644
> > index 0000000..65f1bfd
> > --- /dev/null
> > +++ b/gold/testsuite/i386_mov_to_lea2.s
> > @@ -0,0 +1,10 @@
> > +	.text
> > +	.type	foo, @function
> > +foo:
> > +	ret
> > +	.size	foo, .-foo
> > +	.globl	bar
> > +	.type	bar, @function
> > +bar:
> > +	movl	foo@GOT(%ecx), %eax
> > +	.size	bar, .-bar
> > diff --git a/gold/testsuite/i386_mov_to_lea3.s b/gold/testsuite/i386_mov_to_lea3.s
> > new file mode 100644
> > index 0000000..6785e71
> > --- /dev/null
> > +++ b/gold/testsuite/i386_mov_to_lea3.s
> > @@ -0,0 +1,4 @@
> > +	.type	bar, @function
> > +bar:
> > +	movl	_DYNAMIC@GOT(%ecx), %eax
> > +	.size	bar, .-bar
> > diff --git a/gold/testsuite/i386_mov_to_lea4.s b/gold/testsuite/i386_mov_to_lea4.s
> > new file mode 100644
> > index 0000000..7e4c4db
> > --- /dev/null
> > +++ b/gold/testsuite/i386_mov_to_lea4.s
> > @@ -0,0 +1,12 @@
> > +	.text
> > +	.globl	foo
> > +	.hidden foo
> > +	.type	foo, @function
> > +foo:
> > +	ret
> > +	.size	foo, .-foo
> > +	.globl	bar
> > +	.type	bar, @function
> > +bar:
> > +	movl	foo@GOT(%ecx), %eax
> > +	.size	bar, .-bar
> > diff --git a/gold/testsuite/i386_mov_to_lea5.s b/gold/testsuite/i386_mov_to_lea5.s
> > new file mode 100644
> > index 0000000..6999aae
> > --- /dev/null
> > +++ b/gold/testsuite/i386_mov_to_lea5.s
> > @@ -0,0 +1,12 @@
> > +	.text
> > +	.globl	foo
> > +	.protected foo
> > +	.type	foo, @function
> > +foo:
> > +	ret
> > +	.size	foo, .-foo
> > +	.globl	bar
> > +	.type	bar, @function
> > +bar:
> > +	movl	foo@GOT(%ecx), %eax
> > +	.size	bar, .-bar
> > -- 
> > 1.8.3.1
> >