Re: [PATCH] PR gold/17640

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

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
>