From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14782 invoked by alias); 24 Feb 2010 23:17:03 -0000 Received: (qmail 14753 invoked by uid 22791); 24 Feb 2010 23:17:01 -0000 X-SWARE-Spam-Status: No, hits=-1.2 required=5.0 tests=AWL,BAYES_50 X-Spam-Check-By: sourceware.org Received: from mail.accesssoftek.com (HELO mail.accesssoftek.com) (63.145.236.71) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 24 Feb 2010 23:16:56 +0000 Received: from [192.168.159.141] (172.16.0.1) by mail.accesssoftek.com (172.16.0.71) with Microsoft SMTP Server (TLS) id 8.2.234.1; Wed, 24 Feb 2010 15:14:26 -0800 Subject: Re: [GOLD][PATCH PROPOSAL] prevent discarding of needed local symbols for the relocatable objects From: Viktor Kutuzov Reply-To: vkutuzov@accesssoftek.com To: Ian Lance Taylor CC: binutils In-Reply-To: References: <1265843004.2150.342.camel@dp690-dev5v4> Content-Type: multipart/mixed; boundary="=-D7O2+jxhJcVZTAGiokAl" Date: Thu, 25 Feb 2010 00:02:00 -0000 Message-ID: <1267053412.6817.190.camel@dp690-dev5v4> MIME-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2010-02/txt/msg00468.txt.bz2 --=-D7O2+jxhJcVZTAGiokAl Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Content-length: 3098 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::set_output_symtab_index): Updated assert to support a special index value (-2) for the needed local symbols. (Symbol_value::set_no_output_symtab_entry): Likewise. (Symbol_value::set_needs_in_output_symtab): New method. (Symbol_value::needs_in_output_symtab): New method. (Sized_relobj::set_needs_in_output_symtab): New method. * object.cc (Sized_relobj::do_count_local_symbols): prevent discarding needed local symbols for the relocatable output objects. (Sized_relobj::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 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 --=-D7O2+jxhJcVZTAGiokAl Content-Disposition: attachment; filename*0="binutils-gold-prevent_discard_needed_local_symbols-updated.pa"; filename*1="tch" Content-Type: text/x-patch; name="binutils-gold-prevent_discard_needed_local_symbols-updated.patch"; charset="UTF-8" Content-Transfer-Encoding: 7bit Content-length: 11471 --- 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 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 . + + 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 --=-D7O2+jxhJcVZTAGiokAl--