Re: Patch for ICF string inline bug for SHT_REL sections.

[Sriraman Tallam <tmsriram at google dot com>]

On Mon, Jul 19, 2010 at 12:16 AM, Ian Lance Taylor <iant@google.com> wrote:
> Sriraman Tallam <tmsriram@google.com> writes:
>
>> 2010-07-18 ?Sriraman Tallam ?<tmsriram@google.com>
>>
>> ? ? ? * gc.h (gc_process_relocs): Save the section header type for use by
>> ? ? ? ICF.
>> ? ? ? * icf.cc (get_section_contents): Get the addend from the text section
>> ? ? ? for SHT_REL relocation sections.
>> ? ? ? * icf.h (Icf::Sh_type_info): New typedef.
>> ? ? ? (Icf::Reloc_info): Add new member sh_type_info.
>
> Can we add a test case?
>
>> + ? ? ? ? ? ? ? // For SHT_REL relocation sections, the addend is stored in the
>> + ? ? ? ? ? ? ? // relocation offset in the text section as a 32-bit value.
>
> That's not true in general. ?It's not even true on x86. ?The R_386_16
> reloc stores a 16-bit value in the section. ?What you need here is
> something similar to Relocatable_relocs::Reloc_strategy, in which the
> general code asks the target for the size of the addend, among other
> things. ?By default on i386 that uses Relocatable_size_for_reloc, which
> you should be able to reuse for this.

Ok thanks, I guess I should have thought of that !.

>
> Also I note that saving the relocation section type for each reloc is
> overkill, though it may not matter much.
>
> I'm not sure you need strings to show the problem, I think it would
> occur in code like
>
> int a[100];
> int f1() { return a[1]; }
> int f2() { return a[2]; }

If I understand you correctly this is not a problem. There is no bug
here because the addend gets hashed as part of the text section.
String inlining is really the only problem because it inlines the
wrong string.

Thanks,
-Sri.

>
> Ian
>