[PATCH] PR gold/17640
Ilya Tocar
tocarip.intel@gmail.com
Thu Mar 5 10:41:00 GMT 2015
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
+ || parameters->options().Bsymbolic())
+ && strcmp(gsym->name(), "_DYNAMIC"))
+ {
+ 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 +2777,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 +2825,49 @@ 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);
+ // 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
+ && ((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
+ || parameters->options().Bsymbolic())
+ && !gsym->is_preemptible())))
+ {
+ 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 7b73f9d..ea04ae7 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -957,6 +957,57 @@ 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_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_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_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_lea6
+ $(TEST_READELF) -s $< > $@
+i386_mov_to_lea8.stdout: i386_mov_to_lea7
+ $(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..9739432
--- /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 "lea -0x[a-f0-9]\+(%ecx),%eax" i386_mov_to_lea6.stdout
+grep -q "_DYNAMIC" i386_mov_to_lea7.stdout
+grep -q "lea -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
--
1.8.3.1
More information about the Binutils
mailing list