Home Original page

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