Gold Patch to fix ICF bug.

Ian Lance Taylor iant@google.com
Fri Apr 23 13:17:00 GMT 2010


Sriraman Tallam <tmsriram@google.com> writes:

> 	* 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.


> @@ -256,7 +257,8 @@ 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))

No need for a line break here.  Write
          if (!is_ordinary || shndx == src_indx)




> +              if (is_ordinary && (gsym->source() == Symbol::FROM_OBJECT))

You don't need to put parentheses around == as a clause of &&, and
most of gold does not.  I guess you can if you really want to.


> @@ -312,6 +318,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;

The indentation looks wrong here.


> +	  // The object pointed to by the reloc is NULL.
> +	  if (it_v->first == NULL)

This comment is not helpful.  The comment should instead say what it
means for the object to be NULL.  Perhaps something like
	  // Check for symbol not associated with any specific object,
	  // such as a common symbol.


> 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 00:31:51 -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

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?


This patch is OK with the above changes.  Let me know if you want me
to look at the testsuite changes.

Thanks.

Ian



More information about the Binutils mailing list