Re: [PATCH][ARM][gas] Warn on deprecated ldm r<n>, {..., sp,...} and ldm r<n>, {...,lr,pc,...} forms

[Richard Earnshaw <rearnsha at arm dot com>]

On 20/01/15 10:13, Kyrill Tkachov wrote:
> 
> On 20/01/15 10:10, Kyrill Tkachov wrote:
>> On 14/01/15 11:32, Ramana Radhakrishnan wrote:
>>> On Wed, Jan 14, 2015 at 11:12 AM, Richard Earnshaw <rearnsha@arm.com> wrote:
>>>> On 14/01/15 10:24, Kyrill Tkachov wrote:
>>>>> Hi all,
>>>>>
>>>>> ARMv7-A deprecates ldm instructions that use the SP register in their list
>>>>> as well as ldm instructions that use both LR and PC. GAS should warn
>>>>> about them.
>>>>>
>>>>> This patch adds a warning for these forms.
>>>>> Tests are added/updated.
>>>>>
>>>>> Tested check-gas in arm-none-eabi and I've had it sitting in my tree
>>>>> being tested
>>>>> with gcc for a few months now.
>>>>>
>>>>> What do people think?
>>>>>
>>>> I think firstly, that we should use as_tsktsk for deprecations, rather
>>>> than warnings.
>>> as_tsktsk would be better but we need to change a whole load of
>>> deprecations to this form rather than warnings in the ARM port. I
>>> don't think all our other deprecations use tsktsk. I think we need to
>>> move people on when they use this on v7-a and newer revisions of the
>>> architecture.
>> Shall we go the way of as_tsktsk then and convert the other deprecation
>> warnings to as_tsktsk.
> 
> That should have been a question :)
> 

Yes, we should be consistent.  And I think tsktsk is the right level for
these.  We really do understand what the user has asked for, it's just
that it has *potentially* undefined behaviour at run time.

A separate patch to make the messages consistent would be welcomed.  For
this patch, just fix it to use tsktsk.

R.

> Kyrill
> 
>> I do see some deprecations using as_tsktsk and some using
>> as_warn.
>>
>> Kyrill
>>
>>>    I don't like hiding these behind flags or folks will not fix their
>>> issues without the compiler or assembler warning / shouting at them.
>>>
>>>> I'm still somewhat concerned about the use of SP in LDM lists.  This is
>>>> going to make any legacy ABI code generated by GCC (-mapcs-frame) very
>>>> verbose.  I don't expect we will want to fix GCC to avoid this sequence.
>>> Personally I think (not very strongly) we should fix GCC to avoid this
>>> sequence if we can and then deprecate all uses of mapcs-frame.
>>> Unfortunately it's one of these options that appears to linger on in
>>> build systems for no apparently good reason.
>>>
>>> Ramana
>>>
>>>> I'd like opinions from others on this one...
>>>>
>>>> R.
>>>>
>>>>> Thanks,
>>>>> Kyrill
>>>>>
>>>>> [gas/]
>>>>> 2015-01-14  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>
>>>>>        * config/tc-arm.c (encode_ldmstm): Add deprecation warning when
>>>>>        SP or both LR and PC are used in ldm register list.
>>>>>
>>>>> [gas/testsuite]
>>>>> 2015-01-14  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>
>>>>>        * gas/cfi/cfi-arm-1.s: Change usage of sp to ip in 'ldmea'.
>>>>>        * gas/arm/arm-ldm-bad.s: New file.
>>>>>        * gas/arm/arm-ldm-bad.d: Likewise.
>>>>>        * gas/arm/arm-ldm-bad.l: Likewise.
>>>>>
>>>>>
>>>>> arm-gas-ldm-warnings.patch
>>>>>
>>>>>
>>>>> commit 945c748ad5075d3d0e7d54e6258d8ab9bcf5fa36
>>>>> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>>>> Date:   Mon Jun 30 17:28:56 2014 +0100
>>>>>
>>>>>       [ARM][gas] ldm sp warnings
>>>>>
>>>>> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
>>>>> index ce0532b..5755b4b 100644
>>>>> --- a/gas/config/tc-arm.c
>>>>> +++ b/gas/config/tc-arm.c
>>>>> @@ -8130,6 +8130,15 @@ encode_ldmstm(int from_push_pop_mnem)
>>>>>         }
>>>>>        }
>>>>>
>>>>> +  if (inst.instruction & LOAD_BIT
>>>>> +      && ARM_CPU_HAS_FEATURE (cpu_variant, arm_ext_v7a))
>>>>> +    {
>>>>> +      if (range & (1 << REG_SP))
>>>>> +        as_warn (_("usage of SP in register list is deprecated"));
>>>>> +      else if (range & (1 << REG_PC) && range & (1 << REG_LR))
>>>>> +        as_warn (_("usage of both LR and PC in register list is deprecated"));
>>>>> +    }
>>>>> +
>>>>>      /* If PUSH/POP has only one register, then use the A2 encoding.  */
>>>>>      one_reg = only_one_reg_in_list (range);
>>>>>      if (from_push_pop_mnem && one_reg >= 0)
>>>>> diff --git a/gas/testsuite/gas/arm/arm-ldm-bad.d b/gas/testsuite/gas/arm/arm-ldm-bad.d
>>>>> new file mode 100644
>>>>> index 0000000..8ce6a54
>>>>> --- /dev/null
>>>>> +++ b/gas/testsuite/gas/arm/arm-ldm-bad.d
>>>>> @@ -0,0 +1,3 @@
>>>>> +#name: ARM ldm/stm warnings
>>>>> +#as: -march=armv7-a
>>>>> +#error-output: arm-ldm-bad.l
>>>>> diff --git a/gas/testsuite/gas/arm/arm-ldm-bad.l b/gas/testsuite/gas/arm/arm-ldm-bad.l
>>>>> new file mode 100644
>>>>> index 0000000..1609594
>>>>> --- /dev/null
>>>>> +++ b/gas/testsuite/gas/arm/arm-ldm-bad.l
>>>>> @@ -0,0 +1,5 @@
>>>>> +[^:]*: Assembler messages:
>>>>> +[^:]*:4: Warning: usage of both LR and PC in register list is deprecated
>>>>> +[^:]*:5: Warning: usage of both LR and PC in register list is deprecated
>>>>> +[^:]*:6: Warning: usage of SP in register list is deprecated
>>>>> +[^:]*:7: Warning: usage of SP in register list is deprecated
>>>>> diff --git a/gas/testsuite/gas/arm/arm-ldm-bad.s b/gas/testsuite/gas/arm/arm-ldm-bad.s
>>>>> new file mode 100644
>>>>> index 0000000..d16cf5d
>>>>> --- /dev/null
>>>>> +++ b/gas/testsuite/gas/arm/arm-ldm-bad.s
>>>>> @@ -0,0 +1,7 @@
>>>>> +     .global entry
>>>>> +     .text
>>>>> +entry:
>>>>> +     ldm sp, {r4-r7,r11,lr,pc}
>>>>> +     ldm sp!, {r4-r7,r11,lr,pc}
>>>>> +     ldm r1, {r4-r7,sp}
>>>>> +     ldm r1!, {r4-r7,sp}
>>>>> diff --git a/gas/testsuite/gas/cfi/cfi-arm-1.s b/gas/testsuite/gas/cfi/cfi-arm-1.s
>>>>> index d962442..481f4f2 100644
>>>>> --- a/gas/testsuite/gas/cfi/cfi-arm-1.s
>>>>> +++ b/gas/testsuite/gas/cfi/cfi-arm-1.s
>>>>> @@ -23,7 +23,7 @@ foo:
>>>>>         .cfi_offset r1,  -16
>>>>>         .cfi_offset s1,  -20
>>>>>         .cfi_offset d11, -48
>>>>> -
>>>>> -     ldmea   fp, {fp, sp, pc}
>>>>> +
>>>>> +     ldmea   fp, {fp, ip, pc}
>>>>>         .cfi_endproc
>>>>>         .size   foo, .-foo
>>>>>
>>
>>
>