Gold Patch to fix ICF bug.
Sriraman Tallam
tmsriram@google.com
Fri Apr 23 18:50:00 GMT 2010
I committed the patch with the changes.
2010-04-23 Sriraman Tallam <tmsriram@google.com>
* gc.h (gc_process_relocs): Pass information on relocs pointing to
sections that are not ordinary to icf.
* icf.cc (get_section_contents): Handle relocation pointing to section
with no object or shndx information.
* testsuite/Makefile.am: Remove icf_virtual_function_folding_test.sh
* testsuite/Makefile.in: Regenerate.
* testsuite/icf_virtual_function_folding_test.cc: Remove printf.
* testsuite/icf_virtual_function_folding_test.sh: Delete file.
On Fri, Apr 23, 2010 at 10:44 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Fri, Apr 23, 2010 at 10:41 AM, Ian Lance Taylor <iant@google.com> wrote:
>> Sriraman Tallam <tmsriram@google.com> writes:
>>
>>>> When I look at this test I don't understand what it is testing. It
>>>> seems like the program is always going to run successfully, and you
>>>> aren't testing anything else here. What is the point of this test?
>>>
>>> In this test, fn1 is folded into fn2 but still the linker must
>>> generate a dynamic relocation for the vtable entry for fn1 as it is a
>>> pie executable and fn1 is virtual. Otherwise, the program segfaults. I
>>> am simply testing this. Is this alright ? I had
>>> icf_virtual_function_folding_test run using a shell script before and
>>> I realized that was not necessary.
>>
>> If it segfaults before your patch and stops segfaulting after your
>> patch then this test is fine. I guess I don't yet see why it
>> segfaults before your patch, but I haven't tried running it myself.
>
> Yes, this was the test case used to expose the bug. Not checking for
> section folding was causing the linker to omit the dynamic reloc
> corresponding to fn1 and this caused the virtual call to crash as it
> was not dynamically relocated.
>
>>
>> Ian
>>
>
-------------- next part --------------
Index: gc.h
===================================================================
RCS file: /cvs/src/src/gold/gc.h,v
retrieving revision 1.11
diff -u -u -p -r1.11 gc.h
--- gc.h 21 Feb 2010 00:57:59 -0000 1.11
+++ gc.h 23 Apr 2010 18:39:22 -0000
@@ -228,14 +228,14 @@ gc_process_relocs(
unsigned int shndx = lsym.get_st_shndx();
bool is_ordinary;
shndx = src_obj->adjust_sym_shndx(r_sym, shndx, &is_ordinary);
- if (!is_ordinary)
- continue;
dst_obj = src_obj;
dst_indx = shndx;
- Section_id dst_id(dst_obj, dst_indx);
if (is_icf_tracked)
{
- (*secvec).push_back(dst_id);
+ if (is_ordinary)
+ (*secvec).push_back(Section_id(dst_obj, dst_indx));
+ else
+ (*secvec).push_back(Section_id(NULL, 0));
(*symvec).push_back(NULL);
long long symvalue = static_cast<long long>(lsym.get_st_value());
(*addendvec).push_back(std::make_pair(symvalue,
@@ -247,7 +247,8 @@ gc_process_relocs(
// When doing safe folding, check to see if this relocation is that
// of a function pointer being taken.
- if (check_section_for_function_pointers
+ if (is_ordinary
+ && check_section_for_function_pointers
&& lsym.get_st_type() != elfcpp::STT_OBJECT
&& scan.local_reloc_may_be_function_pointer(symtab, NULL, NULL,
src_obj, src_indx,
@@ -256,7 +257,7 @@ gc_process_relocs(
symtab->icf()->set_section_has_function_pointers(
src_obj, lsym.get_st_shndx());
- if (shndx == src_indx)
+ if (!is_ordinary || shndx == src_indx)
continue;
}
else
@@ -265,16 +266,20 @@ gc_process_relocs(
gold_assert(gsym != NULL);
if (gsym->is_forwarder())
gsym = symtab->resolve_forwards(gsym);
- if (gsym->source() != Symbol::FROM_OBJECT)
- continue;
- bool is_ordinary;
- dst_obj = gsym->object();
- dst_indx = gsym->shndx(&is_ordinary);
- Section_id dst_id(dst_obj, dst_indx);
+
+ dst_obj = NULL;
+ dst_indx = 0;
+ bool is_ordinary = false;
+ if (gsym->source() == Symbol::FROM_OBJECT)
+ {
+ dst_obj = gsym->object();
+ dst_indx = gsym->shndx(&is_ordinary);
+ }
// When doing safe folding, check to see if this relocation is that
// of a function pointer being taken.
- if (check_section_for_function_pointers
+ if (gsym->source() == Symbol::FROM_OBJECT
+ && check_section_for_function_pointers
&& gsym->type() != elfcpp::STT_OBJECT
&& (!is_ordinary
|| scan.global_reloc_may_be_function_pointer(
@@ -282,9 +287,6 @@ gc_process_relocs(
r_type, gsym)))
symtab->icf()->set_section_has_function_pointers(dst_obj, dst_indx);
- if (!is_ordinary)
- continue;
-
// If the symbol name matches '__start_XXX' then the section with
// the C identifier like name 'XXX' should not be garbage collected.
// A similar treatment to symbols with the name '__stop_XXX'.
@@ -300,7 +302,10 @@ gc_process_relocs(
}
if (is_icf_tracked)
{
- (*secvec).push_back(dst_id);
+ if (is_ordinary && gsym->source() == Symbol::FROM_OBJECT)
+ (*secvec).push_back(Section_id(dst_obj, dst_indx));
+ else
+ (*secvec).push_back(Section_id(NULL, 0));
(*symvec).push_back(gsym);
Sized_symbol<size>* sized_gsym =
static_cast<Sized_symbol<size>* >(gsym);
@@ -312,6 +317,11 @@ gc_process_relocs(
convert_to_section_size_type(reloc.get_r_offset());
(*offsetvec).push_back(reloc_offset);
}
+
+ if (gsym->source() != Symbol::FROM_OBJECT)
+ continue;
+ if (!is_ordinary)
+ continue;
}
if (parameters->options().gc_sections())
{
Index: icf.cc
===================================================================
RCS file: /cvs/src/src/gold/icf.cc,v
retrieving revision 1.12
diff -u -u -p -r1.12 icf.cc
--- icf.cc 20 Apr 2010 21:13:30 -0000 1.12
+++ icf.cc 23 Apr 2010 18:39:22 -0000
@@ -263,8 +263,11 @@ get_section_contents(bool first_iteratio
{
Icf::Sections_reachable_info v =
(it_reloc_info_list->second).section_info;
+ // Stores the information of the symbol pointed to by the reloc.
Icf::Symbol_info s = (it_reloc_info_list->second).symbol_info;
+ // Stores the addend and the symbol value.
Icf::Addend_info a = (it_reloc_info_list->second).addend_info;
+ // Stores the offset of the reloc.
Icf::Offset_info o = (it_reloc_info_list->second).offset_info;
Icf::Sections_reachable_info::iterator it_v = v.begin();
Icf::Symbol_info::iterator it_s = s.begin();
@@ -285,6 +288,24 @@ get_section_contents(bool first_iteratio
static_cast<long long>((*it_a).first),
static_cast<long long>((*it_a).second),
static_cast<unsigned long long>(*it_o));
+
+ // If the symbol pointed to by the reloc is not in an ordinary
+ // section or if the symbol type is not FROM_OBJECT, then the
+ // object is NULL.
+ if (it_v->first == NULL)
+ {
+ if (first_iteration)
+ {
+ // If the symbol name is available, use it.
+ if ((*it_s) != NULL)
+ buffer.append((*it_s)->name());
+ // Append the addend.
+ buffer.append(addend_str);
+ buffer.append("@");
+ }
+ continue;
+ }
+
Section_id reloc_secn(it_v->first, it_v->second);
// If this reloc turns back and points to the same section,
@@ -406,8 +427,7 @@ get_section_contents(bool first_iteratio
else if ((*it_s) != NULL)
{
// If symbol name is available use that.
- const char *sym_name = (*it_s)->name();
- buffer.append(sym_name);
+ buffer.append((*it_s)->name());
// Append the addend.
buffer.append(addend_str);
buffer.append("@");
cvs diff: Diffing po
cvs diff: Diffing testsuite
Index: testsuite/Makefile.am
===================================================================
RCS file: /cvs/src/src/gold/testsuite/Makefile.am,v
retrieving revision 1.131
diff -u -u -p -r1.131 Makefile.am
--- testsuite/Makefile.am 22 Apr 2010 14:12:42 -0000 1.131
+++ testsuite/Makefile.am 23 Apr 2010 18:39:22 -0000
@@ -193,7 +193,6 @@ icf_safe_so_test_1.stdout: icf_safe_so_t
icf_safe_so_test_2.stdout: icf_safe_so_test
$(TEST_READELF) -h icf_safe_so_test > icf_safe_so_test_2.stdout
-check_SCRIPTS += icf_virtual_function_folding_test.sh
check_PROGRAMS += icf_virtual_function_folding_test
MOSTLYCLEANFILES += icf_virtual_function_folding_test
icf_virtual_function_folding_test.o: icf_virtual_function_folding_test.cc
Index: testsuite/icf_virtual_function_folding_test.cc
===================================================================
RCS file: /cvs/src/src/gold/testsuite/icf_virtual_function_folding_test.cc,v
retrieving revision 1.1
diff -u -u -p -r1.1 icf_virtual_function_folding_test.cc
--- testsuite/icf_virtual_function_folding_test.cc 20 Apr 2010 21:13:30 -0000 1.1
+++ testsuite/icf_virtual_function_folding_test.cc 23 Apr 2010 18:39:22 -0000
@@ -25,11 +25,8 @@
// for the virtual call fn1 entry in the vtable. This test makes sure
// the call to Foo::fn1 works correctly after the folding.
-#include <stdio.h>
-
int fn2(void *)
{
- printf("fn1==fn2\n");
return 0xA;
}
@@ -54,7 +51,6 @@ class Foo : public Bar
int Foo::fn1()
{
- printf("fn1==fn2\n");
return 0xA;
}
Index: testsuite/icf_virtual_function_folding_test.sh
===================================================================
RCS file: testsuite/icf_virtual_function_folding_test.sh
diff -N testsuite/icf_virtual_function_folding_test.sh
--- testsuite/icf_virtual_function_folding_test.sh 20 Apr 2010 21:13:30 -0000 1.1
+++ /dev/null 1 Jan 1970 00:00:00 -0000
@@ -1,35 +0,0 @@
-#!/bin/sh
-
-# icf_virtual_function_folding_test.sh -- test --icf
-
-# Copyright 2010 Free Software Foundation, Inc.
-# Written by Sriraman Tallam <tmsriram@google.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.
-
-check()
-{
- ./icf_virtual_function_folding_test | grep $1 > /dev/null 2>&1
- if [ $? -gt 0 ]
- then
- echo "Program output incorrect after folding." $2
- exit 1
- fi
-}
-
-check "fn1==fn2"
More information about the Binutils
mailing list