[PATCH, GAS/ARM] Fix expansion of ldr pseudo instruction

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH, GAS/ARM] Fix expansion of ldr pseudo instruction

Thomas Preudhomme
Hi,

The LDR rX, =cst pseudo-instruction suffers from two issues for loading
integer constants in Thumb mode:

- movs is used if the constant and register can be encoded using that
   instruction which leads to unexpected behavior due to its flag-setting
   behavior
- mov.w, movw and mvn are used for r13 (sp) and r15 (pc) but these
   encoding are marked as UNPREDICTABLE

This patch fixes those issues and update testing accordingly.

ChangeLog entry is as follows:

*** gas/ChangeLog ***

2017-04-20  Thomas Preud'homme  <[hidden email]>

        * config/tc-arm.c (move_or_literal_pool): Remove code generating MOVS.
        Forbid MOV.W and MOVW if destination is SP or PC.
        * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s: Explain
        expectation of LDR not generating a MOVS for low registers and small
        constants.  Add tests of MOVW generation.
        * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d: Update
        expected disassembly.


Testsuite shows no regression with this patch.

Is this ok for master?

Best regards,

Thomas

fix_ldr_pseudo_insn_expansion.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, GAS/ARM] Fix expansion of ldr pseudo instruction

Nick Clifton
Hi Thomas,

> 2017-04-20  Thomas Preud'homme  <[hidden email]>
>
>      * config/tc-arm.c (move_or_literal_pool): Remove code generating MOVS.
>      Forbid MOV.W and MOVW if destination is SP or PC.
>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s: Explain
>      expectation of LDR not generating a MOVS for low registers and small
>      constants.  Add tests of MOVW generation.
>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d: Update
>      expected disassembly.

Approved - please apply.

Cheers
  Nick
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, GAS/ARM] Fix expansion of ldr pseudo instruction

Thomas Preudhomme
Patch applies cleanly on binutils-2_28-branch and shows no testsuite regression.
Is this ok to commit to binutils 2.28?

Best regards,

Thomas

On 24/04/17 14:07, Nick Clifton wrote:

> Hi Thomas,
>
>> 2017-04-20  Thomas Preud'homme  <[hidden email]>
>>
>>      * config/tc-arm.c (move_or_literal_pool): Remove code generating MOVS.
>>      Forbid MOV.W and MOVW if destination is SP or PC.
>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s: Explain
>>      expectation of LDR not generating a MOVS for low registers and small
>>      constants.  Add tests of MOVW generation.
>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d: Update
>>      expected disassembly.
>
> Approved - please apply.
>
> Cheers
>   Nick
>

fix_ldr_movs_expansion.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, GAS/ARM] Fix expansion of ldr pseudo instruction

Ramana Radhakrishnan-7
On Wed, Jun 14, 2017 at 9:49 AM, Thomas Preudhomme
<[hidden email]> wrote:
> Patch applies cleanly on binutils-2_28-branch and shows no testsuite
> regression. Is this ok to commit to binutils 2.28?
>

can you mark this as fixing PR21590 in your changelog for 2.28 ?

However you need an ACK from Tristan about 2.28.

Also could you please note the sha1 which fixed this on trunk on the
bz for posterity ?

regards
Ramana


> Best regards,
>
> Thomas
>
>
> On 24/04/17 14:07, Nick Clifton wrote:
>>
>> Hi Thomas,
>>
>>> 2017-04-20  Thomas Preud'homme  <[hidden email]>
>>>
>>>      * config/tc-arm.c (move_or_literal_pool): Remove code generating
>>> MOVS.
>>>      Forbid MOV.W and MOVW if destination is SP or PC.
>>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s: Explain
>>>      expectation of LDR not generating a MOVS for low registers and small
>>>      constants.  Add tests of MOVW generation.
>>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d: Update
>>>      expected disassembly.
>>
>>
>> Approved - please apply.
>>
>> Cheers
>>   Nick
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, GAS/ARM] Fix expansion of ldr pseudo instruction

Thomas Preudhomme


On 14/06/17 10:11, Ramana Radhakrishnan wrote:
> On Wed, Jun 14, 2017 at 9:49 AM, Thomas Preudhomme
> <[hidden email]> wrote:
>> Patch applies cleanly on binutils-2_28-branch and shows no testsuite
>> regression. Is this ok to commit to binutils 2.28?
>>
>
> can you mark this as fixing PR21590 in your changelog for 2.28 ?

Sure, please ignore the ChangeLog in the patch, this is coming from the
cherry-pick. The proposed ChangeLog would be:

2017-06-14  Thomas Preud'homme  <[hidden email]>

        Backport from mainline
        2017-04-24  Thomas Preud'homme  <[hidden email]>

        PR 21590
        * config/tc-arm.c (move_or_literal_pool): Remove code generating MOVS.
        Forbid MOV.W and MOVW if destination is SP or PC.
        * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s: Explain
        expectation of LDR not generating a MOVS for low registers and small
        constants.  Add tests of MOVW generation.
        * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d: Update
        expected disassembly.

>
> However you need an ACK from Tristan about 2.28.

Yes indeed, my mistake.

>
> Also could you please note the sha1 which fixed this on trunk on the
> bz for posterity ?

Done.

Best regards,

Thomas

>
> regards
> Ramana
>
>
>> Best regards,
>>
>> Thomas
>>
>>
>> On 24/04/17 14:07, Nick Clifton wrote:
>>>
>>> Hi Thomas,
>>>
>>>> 2017-04-20  Thomas Preud'homme  <[hidden email]>
>>>>
>>>>      * config/tc-arm.c (move_or_literal_pool): Remove code generating
>>>> MOVS.
>>>>      Forbid MOV.W and MOVW if destination is SP or PC.
>>>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s: Explain
>>>>      expectation of LDR not generating a MOVS for low registers and small
>>>>      constants.  Add tests of MOVW generation.
>>>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d: Update
>>>>      expected disassembly.
>>>
>>>
>>> Approved - please apply.
>>>
>>> Cheers
>>>   Nick
>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, GAS/ARM, ping] Fix expansion of ldr pseudo instruction

Thomas Preudhomme
Ping Tristan?

Best regards,

Thomas

On 14/06/17 10:25, Thomas Preudhomme wrote:

>
>
> On 14/06/17 10:11, Ramana Radhakrishnan wrote:
>> On Wed, Jun 14, 2017 at 9:49 AM, Thomas Preudhomme
>> <[hidden email]> wrote:
>>> Patch applies cleanly on binutils-2_28-branch and shows no testsuite
>>> regression. Is this ok to commit to binutils 2.28?
>>>
>>
>> can you mark this as fixing PR21590 in your changelog for 2.28 ?
>
> Sure, please ignore the ChangeLog in the patch, this is coming from the
> cherry-pick. The proposed ChangeLog would be:
>
> 2017-06-14  Thomas Preud'homme  <[hidden email]>
>
>     Backport from mainline
>     2017-04-24  Thomas Preud'homme  <[hidden email]>
>
>     PR 21590
>     * config/tc-arm.c (move_or_literal_pool): Remove code generating MOVS.
>     Forbid MOV.W and MOVW if destination is SP or PC.
>     * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s: Explain
>     expectation of LDR not generating a MOVS for low registers and small
>     constants.  Add tests of MOVW generation.
>     * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d: Update
>     expected disassembly.
>
>>
>> However you need an ACK from Tristan about 2.28.
>
> Yes indeed, my mistake.
>
>>
>> Also could you please note the sha1 which fixed this on trunk on the
>> bz for posterity ?
>
> Done.
>
> Best regards,
>
> Thomas
>
>>
>> regards
>> Ramana
>>
>>
>>> Best regards,
>>>
>>> Thomas
>>>
>>>
>>> On 24/04/17 14:07, Nick Clifton wrote:
>>>>
>>>> Hi Thomas,
>>>>
>>>>> 2017-04-20  Thomas Preud'homme  <[hidden email]>
>>>>>
>>>>>      * config/tc-arm.c (move_or_literal_pool): Remove code generating
>>>>> MOVS.
>>>>>      Forbid MOV.W and MOVW if destination is SP or PC.
>>>>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s: Explain
>>>>>      expectation of LDR not generating a MOVS for low registers and small
>>>>>      constants.  Add tests of MOVW generation.
>>>>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d: Update
>>>>>      expected disassembly.
>>>>
>>>>
>>>> Approved - please apply.
>>>>
>>>> Cheers
>>>>   Nick
>>>>
>>>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, GAS/ARM, ping] Fix expansion of ldr pseudo instruction

Tristan Gingold-2
On 20/06/2017 11:03, Thomas Preudhomme wrote:
> Ping Tristan?

I've missed your mail, sorry.

Yes, that's ok.

Tristan.

>
> Best regards,
>
> Thomas
>
> On 14/06/17 10:25, Thomas Preudhomme wrote:
>>
>>
>> On 14/06/17 10:11, Ramana Radhakrishnan wrote:
>>> On Wed, Jun 14, 2017 at 9:49 AM, Thomas Preudhomme
>>> <[hidden email]> wrote:
>>>> Patch applies cleanly on binutils-2_28-branch and shows no testsuite
>>>> regression. Is this ok to commit to binutils 2.28?
>>>>
>>>
>>> can you mark this as fixing PR21590 in your changelog for 2.28 ?
>>
>> Sure, please ignore the ChangeLog in the patch, this is coming from the
>> cherry-pick. The proposed ChangeLog would be:
>>
>> 2017-06-14  Thomas Preud'homme  <[hidden email]>
>>
>>     Backport from mainline
>>     2017-04-24  Thomas Preud'homme  <[hidden email]>
>>
>>     PR 21590
>>     * config/tc-arm.c (move_or_literal_pool): Remove code generating
>> MOVS.
>>     Forbid MOV.W and MOVW if destination is SP or PC.
>>     * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s: Explain
>>     expectation of LDR not generating a MOVS for low registers and small
>>     constants.  Add tests of MOVW generation.
>>     * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d: Update
>>     expected disassembly.
>>
>>>
>>> However you need an ACK from Tristan about 2.28.
>>
>> Yes indeed, my mistake.
>>
>>>
>>> Also could you please note the sha1 which fixed this on trunk on the
>>> bz for posterity ?
>>
>> Done.
>>
>> Best regards,
>>
>> Thomas
>>
>>>
>>> regards
>>> Ramana
>>>
>>>
>>>> Best regards,
>>>>
>>>> Thomas
>>>>
>>>>
>>>> On 24/04/17 14:07, Nick Clifton wrote:
>>>>>
>>>>> Hi Thomas,
>>>>>
>>>>>> 2017-04-20  Thomas Preud'homme  <[hidden email]>
>>>>>>
>>>>>>      * config/tc-arm.c (move_or_literal_pool): Remove code generating
>>>>>> MOVS.
>>>>>>      Forbid MOV.W and MOVW if destination is SP or PC.
>>>>>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.s:
>>>>>> Explain
>>>>>>      expectation of LDR not generating a MOVS for low registers
>>>>>> and small
>>>>>>      constants.  Add tests of MOVW generation.
>>>>>>      * testsuite/gas/arm/thumb2_ldr_immediate_highregs_armv6t2.d:
>>>>>> Update
>>>>>>      expected disassembly.
>>>>>
>>>>>
>>>>> Approved - please apply.
>>>>>
>>>>> Cheers
>>>>>   Nick
>>>>>
>>>>