Gold Patch to fix ICF bug.

[Sriraman Tallam]

Thanks for the quick review.

On Fri, Apr 23, 2010 at 6:16 AM, Ian Lance Taylor <iant@google.com> wrote:
> 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?

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.


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