This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [GOLD][PATCH PROPOSAL] prevent discarding of needed local symbols for the relocatable objects
Ian,
please find attached updated patch, which fix the local symbol
discarding behavior for the relocatable output objects (when used the -r
and -X/-x linker options together). I've updated this patch and the unit
tests accordingly.
2010-02-24 Viktor Kutuzov (vkutuzov@accesssoftek.com)
* target-reloc.h (scan_relocatable_relocs): Marking needed local
symbols for the relocatable objects.
* object.h (Symbol_value<int>::set_output_symtab_index): Updated
assert to support a special index value (-2) for the needed local
symbols.
(Symbol_value<size>::set_no_output_symtab_entry): Likewise.
(Symbol_value<size>::set_needs_in_output_symtab): New method.
(Symbol_value<size>::needs_in_output_symtab): New method.
(Sized_relobj<size, big_endianl>::set_needs_in_output_symtab): New
method.
* object.cc (Sized_relobj<size,
big_endian>::do_count_local_symbols): prevent discarding needed local
symbols for the relocatable output objects.
(Sized_relobj<size, big_endian>::write_local_symbols): Likewise.
* testsuite/discard_locals_relocatable_test.c: New file.
* testsuite/discard_locals_test.sh: Updated with new test cases.
* testsuite/Makefile.am: Likewise.
* testsuite/Makefile.in: Likewise.
-Viktor.
On Wed, 2010-02-10 at 18:14 -0800, Ian Lance Taylor wrote:
> Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:
>
> > I'm trying to cross build llvm and llvm-gcc on Linux for ARM by using
> > GOLD as the linker. This has exposed some problems we have in GOLD.
> >
> > One of them related to the assert in the relocate_for_relocatable()
> > method (target-reloc.h, line 557):
> >
> > new_symndx = object->symtab_index(r_sym);
> > gold_assert(new_symndx != -1U);
> >
> > This assert gets triggered when the build links glibc with the -r -X
> > flags (remove local symbols).
> >
> > This happens because with the given -X option GOLD removes all local
> > symbols, including those which still needed to resolve static relocs
> > later.
> >
> > LD keeps that kind of local symbols in the symbol table even if -X
> > requested. I guess GOLD should do the same.
> >
> > Please find attached the patch that fixes this issue.
>
> Thanks, but this patch doesn't work. It fails the testsuite. The
> problem is that at the time your patch checks
> needs_output_symtab_entry, it will always return true.
> needs_output_symtab_entry will only return false if
> set_no_output_symtab_entry has been called, and that only happens at
> the end of the loop you patched.
>
> I think what we need to do here is mark the local symbol as
> appropriate in scan_relocatable_relocs. The use of the
> output_symtab_index_ field right now is a bit complicated. It is
> initialized to 0. In Sized_relobj::do_count_local_symbols we set the
> field to -1 if the symbol is not needed. In
> Sized_relobj::do_finalize_local_symbols, if the field is still 0, we
> set it to the index in the output file. Perhaps we should change that
> 0 == uninitialized, -1 == no index, -2 == needs index. Then
> do_finalize_local_symbols should never see 0.
>
> Ian
--
-Viktor
--- binutils.orig/src/gold/target-reloc.h 2010-01-14 10:23:06.000000000 -0800
+++ binutils.gold/src/gold/target-reloc.h 2010-02-24 12:19:57.000000000 -0800
@@ -491,6 +491,16 @@
if (strategy != Relocatable_relocs::RELOC_DISCARD)
object->output_section(shndx)->set_needs_symtab_index();
}
+
+ if ((parameters->options().discard_locals()
+ || parameters->options().discard_all())
+ && parameters->options().relocatable()
+ && strategy != Relocatable_relocs::RELOC_DISCARD)
+ {
+ // For the relocatable output objects, mark this local
+ // symbol as needed in the output symbol table.
+ object->set_needs_in_output_symtab(r_sym);
+ }
}
}
--- binutils.orig/src/gold/object.h 2010-02-24 12:01:23.000000000 -0800
+++ binutils.gold/src/gold/object.h 2010-02-24 12:25:59.000000000 -0800
@@ -1105,7 +1105,8 @@
void
set_output_symtab_index(unsigned int i)
{
- gold_assert(this->output_symtab_index_ == 0);
+ gold_assert(this->output_symtab_index_ == 0
+ || this->output_symtab_index_ == -2U);
this->output_symtab_index_ = i;
}
@@ -1114,10 +1115,27 @@
void
set_no_output_symtab_entry()
{
- gold_assert(this->output_symtab_index_ == 0);
+ gold_assert(this->output_symtab_index_ == 0
+ || this->output_symtab_index_ == -2U);
this->output_symtab_index_ = -1U;
}
+ // Record that this symbol should go into the output symbol table.
+ void
+ set_needs_in_output_symtab()
+ {
+ // Keep a valid symtab index
+ if (this->output_symtab_index_ == 0
+ || this->output_symtab_index_ == -1U)
+ this->output_symtab_index_ = -2U;
+ }
+
+ // For the relocatable ouput object, return whether this symbol
+ // should go into the output symbol table. This method always returns
+ // false for any other type of the output objects.
+ bool needs_in_output_symtab()
+ { return (this->output_symtab_index_ == -2U); }
+
// Set the index in the output dynamic symbol table.
void
set_needs_output_dynsym_entry()
@@ -1429,6 +1447,14 @@
this->local_values_[sym].set_needs_output_dynsym_entry();
}
+ // Record that local symbol SYM should an output symbol entry.
+ void
+ set_needs_in_output_symtab(unsigned int sym)
+ {
+ gold_assert(sym < this->local_values_.size());
+ this->local_values_[sym].set_needs_in_output_symtab();
+ }
+
// Return whether the local symbol SYMNDX has a GOT offset.
// For TLS symbols, the GOT entry will hold its tp-relative offset.
bool
--- binutils.orig/src/gold/object.cc 2010-02-24 12:01:23.000000000 -0800
+++ binutils.gold/src/gold/object.cc 2010-02-24 12:18:17.000000000 -0800
@@ -1591,7 +1590,7 @@
++dyncount;
}
- if (discard_all)
+ if (discard_all && !lv.needs_in_output_symtab())
{
lv.set_no_output_symtab_entry();
continue;
@@ -1612,6 +1611,7 @@
if (discard_locals
&& sym.get_st_type() != elfcpp::STT_FILE
&& !lv.needs_output_dynsym_entry()
+ && !lv.needs_in_output_symtab()
&& parameters->target().is_local_label_name(name))
{
lv.set_no_output_symtab_entry();
@@ -1937,7 +1937,8 @@
st_shndx = out_sections[st_shndx]->out_shndx();
if (st_shndx >= elfcpp::SHN_LORESERVE)
{
- if (lv.needs_output_symtab_entry() && !strip_all)
+ if (lv.needs_output_symtab_entry()
+ && !(strip_all && !lv.needs_in_output_symtab()))
symtab_xindex->add(lv.output_symtab_index(), st_shndx);
if (lv.needs_output_dynsym_entry())
dynsym_xindex->add(lv.output_dynsym_index(), st_shndx);
@@ -1946,7 +1947,8 @@
}
// Write the symbol to the output symbol table.
- if (!strip_all && lv.needs_output_symtab_entry())
+ if (!(strip_all && !lv.needs_in_output_symtab())
+ && lv.needs_output_symtab_entry())
{
elfcpp::Sym_write<size, big_endian> osym(ov);
--- binutils.orig/src/gold/testsuite/discard_locals_relocatable_test.c 1969-12-31 16:00:00.000000000 -0800
+++ binutils.gold/src/gold/testsuite/discard_locals_relocatable_test.c 2010-02-24 11:38:15.000000000 -0800
@@ -0,0 +1,48 @@
+/* discard_locals_relocatable_test.c -- test --discard-locals/--discard-all
+ options for the relocatable output objects
+ (linked with -relocatable option).
+
+ Note: use GCC -fPIC option to compile this test.
+
+ Copyright 2010 Free Software Foundation, Inc.
+ Viktor Kutuzov <vkutuzov@accesssoftek.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.
+
+ This is a test of a common symbol in the main program and a
+ versioned symbol in a shared library. The common symbol in the
+ main program should override the shared library symbol. */
+
+/* Local symbol format for generic ELF target.
+ Use GCC -Wa,-L option to preserve this local symbol
+ in the output object file. */
+asm (".Lshould_be_discarded:");
+
+int func(void);
+
+extern void print_func(const char* s);
+
+int func (void)
+{
+ print_func(
+ "This string is splitted to force generation of"
+ "the temporary local symbols like .LXx and the related relocations."
+ "We expect the .LC0 symbol in the source object file.");
+ return 0;
+}
+
--- binutils.orig/src/gold/testsuite/discard_locals_test.sh 2009-06-05 14:32:57.000000000 -0700
+++ binutils.gold/src/gold/testsuite/discard_locals_test.sh 2010-02-24 10:51:35.000000000 -0800
@@ -27,11 +27,12 @@
# the resulting executable and check that symbols from two test library
# archives are correctly hidden or left unmodified.
-check()
+check_discarded()
{
file=$1
+ sym=$2
- found=`egrep "should_be_discarded" $file`
+ found=`egrep $sym $file`
if test -n "$found"; then
echo "These local symbols are not discarded in $file:"
echo "$found"
@@ -39,6 +40,24 @@
fi
}
-check "discard_locals_test.syms"
+check_non_discarded()
+{
+ file=$1
+ sym=$2
+
+ found=`egrep $sym $file`
+ if test -z "$found"; then
+ echo "This local symbol is discarded in $file:"
+ echo "$2"
+ exit 1
+ fi
+}
+
+check_discarded "discard_locals_test.syms" "should_be_discarded"
+
+check_non_discarded "discard_locals_relocatable_test1.syms" ".LC0"
+check_discarded "discard_locals_relocatable_test1.syms" "should_be_discarded"
+check_non_discarded "discard_locals_relocatable_test2.syms" ".LC0"
+check_discarded "discard_locals_relocatable_test2.syms" "should_be_discarded"
exit 0
--- binutils.orig/src/gold/testsuite/Makefile.am 2010-02-24 12:01:23.000000000 -0800
+++ binutils.gold/src/gold/testsuite/Makefile.am 2010-02-24 10:32:03.000000000 -0800
@@ -1285,8 +1285,11 @@
check_PROGRAMS += discard_locals_test
check_SCRIPTS += discard_locals_test.sh
-check_DATA += discard_locals_test.syms
-MOSTLYCLEANFILES += discard_locals_test.syms
+check_DATA += discard_locals_test.syms \
+ discard_locals_relocatable_test1.syms discard_locals_relocatable_test2.syms
+MOSTLYCLEANFILES += discard_locals_test.syms \
+ discard_locals_relocatable_test1.syms discard_locals_relocatable_test2.syms \
+ discard_locals_relocatable_test1.out discard_locals_relocatable_test2.out
discard_locals_test_SOURCES = discard_locals_test.c
discard_locals_test_LDFLAGS = -Bgcctestdir/ -Wl,--discard-locals
discard_locals_test.syms: discard_locals_test
@@ -1295,6 +1298,18 @@
discard_locals_test.o: discard_locals_test.c
$(COMPILE) -c -Wa,-L -o $@ $<
+discard_locals_relocatable_test1.syms: discard_locals_relocatable_test1.out
+ $(TEST_READELF) -sW $< >$@ 2>/dev/null
+discard_locals_relocatable_test.o: discard_locals_relocatable_test.c
+ $(COMPILE) -c -Wa,-L -fPIC -o $@ $<
+discard_locals_relocatable_test1.out: discard_locals_relocatable_test.o
+ ../ld-new --discard-locals -relocatable -o $@ $<
+
+discard_locals_relocatable_test2.syms: discard_locals_relocatable_test2.out
+ $(TEST_READELF) -sW $< >$@ 2>/dev/null
+discard_locals_relocatable_test2.out: discard_locals_relocatable_test.o
+ ../ld-new --discard-all -relocatable -o $@ $<
+
if MCMODEL_MEDIUM
check_PROGRAMS += large
large_SOURCES = large.c
--- binutils.orig/src/gold/testsuite/Makefile.in 2010-02-24 12:01:23.000000000 -0800
+++ binutils.gold/src/gold/testsuite/Makefile.in 2010-02-24 10:41:20.000000000 -0800
@@ -276,6 +276,8 @@
@GCC_TRUE@@NATIVE_LINKER_TRUE@ no_version_test.sh
@GCC_TRUE@@NATIVE_LINKER_TRUE@am__append_28 = exclude_libs_test.syms \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ discard_locals_test.syms \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ discard_locals_relocatable_test1.syms \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ discard_locals_relocatable_test2.syms \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ hidden_test.err \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ retain_symbols_file_test.stdout \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ no_version_test.stdout
@@ -284,6 +286,10 @@
@GCC_TRUE@@NATIVE_LINKER_TRUE@ libexclude_libs_test_2.a \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ alt/libexclude_libs_test_3.a \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ discard_locals_test.syms \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ discard_locals_relocatable_test1.syms \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ discard_locals_relocatable_test2.syms \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ discard_locals_relocatable_test1.out \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ discard_locals_relocatable_test2.out \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ hidden_test hidden_test.err \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ retain_symbols_file_test \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ retain_symbols_file_test.in \
@@ -3136,6 +3142,18 @@
# '-Wa,-L' is required to preserve the local label used for testing.
@GCC_TRUE@@NATIVE_LINKER_TRUE@discard_locals_test.o: discard_locals_test.c
@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(COMPILE) -c -Wa,-L -o $@ $<
+
+@GCC_TRUE@@NATIVE_LINKER_TRUE@discard_locals_relocatable_test1.syms: discard_locals_relocatable_test1.out
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(TEST_READELF) -sW $< >$@ 2>/dev/null
+@GCC_TRUE@@NATIVE_LINKER_TRUE@discard_locals_relocatable_test.o: discard_locals_relocatable_test.c
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(COMPILE) -c -Wa,-L -fPIC -o $@ $<
+@GCC_TRUE@@NATIVE_LINKER_TRUE@discard_locals_relocatable_test1.out: discard_locals_relocatable_test.o
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ ../ld-new --discard-locals -relocatable -o $@ $<
+
+@GCC_TRUE@@NATIVE_LINKER_TRUE@discard_locals_relocatable_test2.syms: discard_locals_relocatable_test2.out
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(TEST_READELF) -sW $< >$@ 2>/dev/null
+@GCC_TRUE@@NATIVE_LINKER_TRUE@discard_locals_relocatable_test2.out: discard_locals_relocatable_test.o
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ ../ld-new --discard-all -relocatable -o $@ $<
@GCC_TRUE@@NATIVE_LINKER_TRUE@libhidden.so: hidden_test_1.c gcctestdir/ld
@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(COMPILE) -Bgcctestdir/ -g -shared -fPIC -w -o $@ $(srcdir)/hidden_test_1.c
@GCC_TRUE@@NATIVE_LINKER_TRUE@hidden_test: hidden_test_main.o libhidden.so gcctestdir/ld