This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2][gold] AArch64: Implement MOVW_UABS_* and MOVW_SABS_* relocations.


Hi Han,

Thank you for applying this.
Unfortunatelly, "aarch64_relocs.sh" doesn't have "execute" bit set.
I wonder if something is wrong with my patches because there was
the same issue with my previous patch. At least, when I applied the patch
on my Linux system, the file was marked as executable, but maybe I miss
something?

Best regards,
Igor Kudrin
________________________________________
From: Han Shen <shenhan@google.com>
Sent: Tuesday, July 26, 2016 9:58 PM
To: Igor Kudrin
Cc: Cary Coutant; binutils@sourceware.org
Subject: Re: [PATCH v2][gold] AArch64: Implement MOVW_UABS_* and MOVW_SABS_* relocations.

Hi Igor, thanks. Done.
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=8769bc4bab847cefc2bb5682a0a0dad579528ac8

Han

On Mon, Jul 25, 2016 at 3:29 AM, Igor Kudrin <ikudrin@accesssoftek.com> wrote:
> Hello Han,
>
> Thanks for the review.
>
> Could someone apply the patch, please? I don't have write permissions yet.
>
> Best regards,
> Igor Kudrin
> ________________________________________
> From: Han Shen <shenhan@google.com>
> Sent: Wednesday, July 6, 2016 10:47 PM
> To: Igor Kudrin
> Cc: Cary Coutant; binutils@sourceware.org
> Subject: Re: [PATCH v2][gold] AArch64: Implement MOVW_UABS_* and MOVW_SABS_* relocations.
>
> Thanks, Igor, LGTM.
>
> Han
>
> On Thu, Jun 30, 2016 at 10:14 AM, Igor Kudrin <ikudrin@accesssoftek.com> wrote:
>> Hi Cary, Han,
>>
>> Thank you very much for the review comments!
>>
>> Here is an updated patch with the following changes:
>> * Rebased to the top.
>> * Changed "(uint64_t)1" to "1ULL".
>> * Added a comment for "Rvalue_bit_select_impl<0, 0>".
>> * Changed "rela_general<64>" to "rela_general<32>".
>>
>> Best regards,
>> Igor Kudrin
>>
>> ---
>> gold/ChangeLog
>>
>>         * aarch64-reloc-property.cc (Rvalue_bit_select_impl): New class.
>>         (rvalue_bit_select): Use Rvalue_bit_select_impl.
>>         * aarch64-reloc.def (MOVW_UABS_G0, MOVW_UABS_G0_NC, MOVW_UABS_G1,
>>         MOVW_UABS_G1_NC, MOVW_UABS_G2, MOVW_UABS_G2_NC, MOVW_UABS_G3,
>>         MOVW_SABS_G0, MOVW_SABS_G1, MOVW_SABS_G2): New relocations.
>>         * aarch64.cc (Target_aarch64::Scan::local): Add cases for new
>>         MOVW_UABS_* and MOVW_SABS_* relocations.
>>         (Target_aarch64::Scan::global): Likewise.
>>         (Target_aarch64::Relocate::relocate): Add cases and handlings
>>         for new MOVW_UABS_* and MOVW_SABS_* relocations.
>>         * testsuite/Makefile.am (aarch64_relocs): New test.
>>         * testsuite/Makefile.in: Regenerate.
>>         * testsuite/aarch64_globals.s: New test source file.
>>         * testsuite/aarch64_relocs.s: Likewise.
>>         * testsuite/aarch64_relocs.sh: New test script.
>>
>> ________________________________________
>> From: Han Shen <shenhan@google.com>
>> Sent: Wednesday, June 29, 2016 8:48 AM
>> To: Cary Coutant
>> Cc: Igor Kudrin; binutils@sourceware.org
>> Subject: Re: [PATCH][gold] AArch64: Implement MOVW_UABS_* and MOVW_SABS_* relocations.
>>
>> Hi Igor, could you add comments to the specialization that L=U=0 means
>> no check, otherwise readers might mistake it for selecting LSB. (The
>> origin code should have added it.)
>>
>> +class Rvalue_bit_select_impl<0, 0>
>> +{
>> +public:
>> +  static uint64_t
>> +  calc(uint64_t x)
>> +  {
>> +    return x;
>> +  }
>> +};
>>
>> Also in the below segment - "rela_general<64>" should be
>> "rela_general<32>", cause we are patching a 32 bit mov insn.
>>
>> +    case elfcpp::R_AARCH64_MOVW_UABS_G3:
>> +      reloc_status = Reloc::template rela_general<64>(
>> + view, object, psymval, addend, reloc_property);
>> +      break;
>>
>> Thanks,
>> Han
>>
>> On Tue, Jun 28, 2016 at 3:18 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>>> [+shenhan this time]
>>>
>>> On Tue, Jun 28, 2016 at 3:17 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>>>>> This patch implements R_AARCH64_MOVW_UABS_G* and R_AARCH64_MOVW_SABS_G*
>>>>> relocations for AArch64 target.
>>>>>
>>>>> Best regards,
>>>>> Igor Kudrin
>>>>>
>>>>> ---
>>>>> gold/ChangeLog
>>>>>
>>>>>         * aarch64-reloc-property.cc (Rvalue_bit_select_impl): New class.
>>>>>         (rvalue_bit_select): Use Rvalue_bit_select_impl.
>>>>>         * aarch64-reloc.def (MOVW_UABS_G0, MOVW_UABS_G0_NC, MOVW_UABS_G1,
>>>>>         MOVW_UABS_G1_NC, MOVW_UABS_G2, MOVW_UABS_G2_NC, MOVW_UABS_G3,
>>>>>         MOVW_SABS_G0, MOVW_SABS_G1, MOVW_SABS_G2): New relocations.
>>>>>         * aarch64.cc (Target_aarch64::Scan::local): Add cases for new
>>>>>         MOVW_UABS_* and MOVW_SABS_* relocations.
>>>>>         (Target_aarch64::Scan::global): Likewise.
>>>>>         (Target_aarch64::Relocate::relocate): Add cases and handlings
>>>>>         for new MOVW_UABS_* and MOVW_SABS_* relocations.
>>>>>         * testsuite/Makefile.am (aarch64_relocs): New test.
>>>>>         * testsuite/Makefile.in: Regenerate.
>>>>>         * testsuite/aarch64_globals.s: New test source file.
>>>>>         * testsuite/aarch64_relocs.s: New test source file.
>>>>>         * testsuite/aarch64_relocs.sh: New test script.
>>>>
>>>> +  static uint64_t
>>>> +  calc(uint64_t x)
>>>> +  {
>>>> +    return (x & (((uint64_t)1 << (U+1)) - 1)) >> L;
>>>> +  }
>>>>
>>>> I know this was the same before your patch, but I'd prefer using
>>>> "1ULL" instead of the C-style cast.
>>>>
>>>> Han, can you take a look at this patch?
>>>>
>>>> -cary
>>
>>
>>
>> --
>> Han Shen |  Software Engineer |  shenhan@google.com |  +1-650-440-3330
>
>
>
> --
> Han Shen |  Software Engineer |  shenhan@google.com |  +1-650-440-3330



--
Han Shen |  Software Engineer |  shenhan@google.com |  +1-650-440-3330


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]