Re: [BFD][AARCH64]Properly truncate no overflow checking relocation value for load/store immediate.

[Peter Smith <peter dot smith at linaro dot org>]

Hello Renlin,

No objections. Apologies if you were waiting for me I'd assumed that
you were able to update based on your prior approval.

Peter

On 20 June 2018 at 09:48, Renlin Li <renlin.li@foss.arm.com> wrote:
> Ping~
>
>
>
>
> On 04/27/2018 10:46 PM, Renlin Li wrote:
>>
>> Hi Peter,
>>
>> Thanks for the comment!
>> (I noticed that when I am working on the fix. But I forgot why I didn't do
>> it there)
>>
>> I update the patch by using PG_OFFSET when resolving the relocation.
>> Regression test Okay.
>>
>> Regards,
>> Renlin
>>
>> bfd/ChangeLog:
>>
>> 2018-04-27  Renlin Li  <renlin.li@arm.com>
>>
>>      * elfxx-aarch64.c (_bfd_aarch64_elf_resolve_relocation): Use
>> PG_OFFSET
>>      to resolve BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC,
>>      BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC,
>>      BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC,
>>      BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC,
>>      BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC,
>>      BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC,
>>      BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC,
>>      BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC.
>>
>> ld/ChangeLog:
>>
>> 2018-04-27 Renlin Li  <renlin.li@arm.com>
>>
>>       * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new
>> value.
>>       * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.
>>       * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.
>>
>> On 04/26/2018 03:53 PM, Peter Smith wrote:
>>>
>>> Hello Renlin,
>>>
>>> I've checked that the patch works on an example I had that previously
>>> gave different results to Gold and an in-progress implementation in
>>> LLD. To the best of my knowledge the bitsizes in the HOWTO and the
>>> method to truncate the result will work. One thing that I'm a bit
>>> curious about is that relocations such as
>>> BFD_RELOC_AARCH64_LDST64_LO12 use "value = PG_OFFSET (value +
>>> addend);" to calculate the addend, whereas relocations like
>>> BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC use "value = value +
>>> addend". In the former case the truncation is done in
>>> _bfd_aarch64_elf_resolve_relocation whereas in the latter you are
>>> having to truncate in _bfd_aarch64_elf_put_addend; it seems like you
>>> might be able to use PG_OFFSET for all the LO12 relocations and not
>>> need to put an extra truncation step in? I'm no expert in the BFD
>>> codebase, so there may be good reasons not to do that so just treat
>>> that as an observation.
>>>
>>> Thanks for the fix.
>>>
>>> Peter
>>>
>>> On 26 April 2018 at 14:05, Renlin Li <renlin.li@foss.arm.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>>
>>>> In aarch64, there are relocations for the unsigned immediate offset of
>>>> load/store instruction.
>>>> The size of the immediate field is 12-bit. In the scaled case, not all
>>>> 12-bit will be used.
>>>>
>>>> For example: R_AARCH64_LD64_GOT_LO12_NC
>>>> Set the LD/ST immediate field to bits [11:3] of relocated value. No
>>>> overflow
>>>> check.
>>>>
>>>> In this case, the top 3 bits of the immediate field of the load
>>>> instruction
>>>> should
>>>> be all 0. Only 9 bits from the value should be written into the
>>>> instruction
>>>> encoding.
>>>>
>>>> For relocations with over-flow check, if the value is larger than
>>>> 12-bits,
>>>> it
>>>> will be caught by the checking mechanism.
>>>>
>>>> But for relocations without no overflow check, the value could be larger
>>>> than
>>>> 12 bits, after scaling, it might take more bits than the specification
>>>> specified. The value should be properly truncated.
>>>>
>>>> The patch here correct the bitsize field of a few relocations, and the
>>>> bitsize
>>>> field is used to properly truncate the value written into the
>>>> instruction
>>>> encoding.
>>>>
>>>> Okay to commit?
>>>>
>>>> bfd/ChangeLog:
>>>>
>>>> 2018-04-26  Renlin Li  <renlin.li@arm.com>
>>>>
>>>>          * elfnn-aarch64.c (elfNN_aarch64_howto_table): Correct the
>>>> bitsize
>>>>          field of R_AARCH64_LD64_GOT_LO12_NC,
>>>> R_AARCH64_P32_LD32_GOT_LO12_NC,
>>>>          R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC,
>>>>          R_AARCH64_P32_LD32_GOTTPREL_LO12_NC,
>>>>          R_AARCH64_LDST16_ABS_LO12_NC,
>>>>          R_AARCH64_P32_TLSDESC_LD32_LO12_NC.
>>>>          R_AARCH64_P32_TLSDESC_LD32_LO12_NC.
>>>>          R_AARCH64_TLSDESC_LD64_LO12_NC.
>>>>          R_AARCH64_LDST32_ABS_LO12_NC,
>>>>          R_AARCH64_LDST64_ABS_LO12_NC,
>>>>          R_AARCH64_LDST128_ABS_LO12_NC,
>>>>          * elfxx-aarch64.c (_bfd_aarch64_elf_put_addend): Truncate the
>>>>          immediate value for load/store instruction.
>>>>
>>>> ld/ChangeLog:
>>>>
>>>> 2018-04-26 Renlin Li  <renlin.li@arm.com>
>>>>
>>>>          * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new
>>>> value.
>>>>          * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.
>>>>          * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.
>>
>>
>