Re: Gold Patch to fix ICF bug.

[Sriraman Tallam <tmsriram at google dot com>]

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
>