Re: [PATCH, ARM] Fix handling of GOT and PLT access to IFUNC symbols

[Will Newton <will dot newton at linaro dot org>]

On 23 April 2013 18:53, Richard Sandiford <rdsandiford@googlemail.com> wrote:
> Richard Earnshaw <rearnsha@arm.com> writes:
>> On 12/04/13 17:42, Will Newton wrote:
>>>
>>> Hi all,
>>>
>>> The current ARM IFUNC code appears to have a bug when an access is made via
>>> the PLT and GOT in the same object. This results in two relocs being swapped
>>> out into the same slot so one R_ARM_IRELATIVE reloc goes missing.
>>>
>>> This patch changes the behaviour to use an incremented reloc count rather
>>> than the calculated PLT index to match the behaviour of elf32_arm_add_dynreloc
>>> and as a result requires the order of relocs in the ifunc tests to be
>>> adjusted.
>>>
>>> bfd/ChangeLog:
>>>
>>> 2013-04-12  Will Newton  <will.newton@linaro.org>
>>>
>>>      * elf32-arm.c (elf32_arm_populate_plt_entry): Increment reloc_count
>>>      when emitting R_ARM_IRELATIVE relocs.
>>>
>>
>> Hmm, it looks somewhat suspicious to me to be changing the reloc count
>> this late on.
>>
>> Richard, do you know if this is safe?  If not, can you recommend a
>> better approach?
>
> Sorry for the slow reply, been on holiday.  The patch looks good to me.
> At this stage, ->reloc_count is the number of relocations that have been
> filled in, whereas ->size is the number of bytes that have been allocated
> in the relocation section.  .rel.plt is special in that the relocation
> index has to match the PLT index, but as Will says, .rel.iplt is more
> like a normal relocation section, and can have a mixture of both "PLT"
> and non-PLT relocations.
>
> In some ways it might be clearer to use elf32_arm_add_dynreloc in
> the dynindx == -1 case, which would also give us the assert that
> we have allocated enough relocation entries.  Both should be
> correct though.
>
> BTW, there's a typo in the testcase:
>
>>+#------ libfunc1's .iplt entry
>
> should be "appfunc1".

Thanks for the review! I'll post a new version of the patch with these changes.


--
Will Newton
Toolchain Working Group, Linaro