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][ARM][gas] Warn on deprecated ldm r<n>, {..., sp,...} and ldm r<n>, {...,lr,pc,...} forms

From: Kyrill Tkachov <kyrylo dot tkachov at arm dot com>
To: Richard Earnshaw <Richard dot Earnshaw at arm dot com>, Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>
Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
Date: Thu, 12 Feb 2015 15:57:20 +0000
Subject: Re: [PATCH][ARM][gas] Warn on deprecated ldm r<n>, {..., sp,...} and ldm r<n>, {...,lr,pc,...} forms
Authentication-results: sourceware.org; auth=none
References: <54B643E9 dot 10607 at arm dot com> <54B64F1D dot 2090102 at arm dot com> <CAJA7tRZQGTHeKSYUk=BwyJqGrkys2wOcE9ruVh4mO28KVCHbqQ at mail dot gmail dot com> <54BE29A3 dot 1000709 at arm dot com> <54BE2A64 dot 6090303 at arm dot com> <54BE5F7E dot 3060905 at arm dot com> <54D207E5 dot 9070603 at arm dot com>

Ping.

https://sourceware.org/ml/binutils/2015-02/msg00029.html

Richard, if we apply this patch we'll get some noise in the GCC 5.0testsuite since some of the tests there generate the sequences with-mapcs (which is deprecated for GCC 5).


I have a patch to adjust the testcases here:
https://gcc.gnu.org/ml/gcc-patches/2015-01/msg01014.html
but I'm not sure if it will be accepted.

Do we want to go ahead with this change?

Thanks,
Kyrill


On 04/02/15 11:52, Kyrill Tkachov wrote:

On 20/01/15 14:00, Richard Earnshaw wrote:

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.

Ok, here it is, using as_tsktsk.
I'll look into moving the other deprecation warnings to tsktsk as well,
though I guess the -mwarn-deprecated option will be not "technically"
warning the user, just notifying?

Kyrill

[gas/]
2015-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

         * config/tc-arm.c (encode_ldmstm): Add deprecation message when
         SP or both LR and PC are used in ldm register list.

[gas/testsuite]
2015-02-04  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.

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

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

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