thumb missing .w InstAlias' for ldr and str #1296

nickdesaulniers · 2021-02-08T22:07:26Z

arch/arm/lib/copy_template.S

<instantiation>:1:23: error: too many operands for instruction
9999: ldr.w r3, [r1], #4; .pushsection __ex_table,"a"; .align 3; .long 9999b,20f; .popsection
                      ^
arch/arm/lib/copy_template.S:131:3: note: while in macro instantiation
  ldr1w r1, r3, abort=20f
  ^
<instantiation>:1:17: error: too many operands for instruction
str.w r3, [r0], #4
                ^
arch/arm/lib/copy_template.S:149:3: note: while in macro instantiation
  str1w r0, r3, abort=20f
  ^

(forked from #1270, cc @arndb )

The text was updated successfully, but these errors were encountered:

nickdesaulniers · 2021-02-09T19:38:51Z

https://llvm.org/pr49118

nickdesaulniers · 2021-02-09T19:52:31Z

I suspect this is arm code mixed in or built as -Wa,-mthumb.

nickdesaulniers · 2021-02-09T19:57:55Z

yeah, that's it.

diff --git a/arch/arm/lib/copy_template.S b/arch/arm/lib/copy_template.S
index 810a805d36dc..da82b45f7a6c 100644
--- a/arch/arm/lib/copy_template.S
+++ b/arch/arm/lib/copy_template.S
@@ -69,6 +69,7 @@
  *     than one 32bit instruction in Thumb-2)
  */
 
+.arm
 
        UNWIND( .fnstart                        )
                enter   r4, lr
diff --git a/arch/arm/lib/memmove.S b/arch/arm/lib/memmove.S
index 6fecc12a1f51..45bf44a2b47f 100644
--- a/arch/arm/lib/memmove.S
+++ b/arch/arm/lib/memmove.S
@@ -12,6 +12,7 @@
 #include <asm/unwind.h>
 
                .text
+               .arm
 
 /*
  * Prototype: void *memmove(void *dest, const void *src, size_t n);

@arndb do we need to limit the .arm directives to only the arm instructions (or push/pop that we're using asm instructions)? It looks like GAS permits mixing ARM and Thumb assembly without directives, while Clang does not.

arndb · 2021-02-09T22:18:37Z

I think the .arm and .thumb directives have to be used before a the symbol is defined, either at file scope or by passing the -marm command line parameter. Doing it inside of copy_template.S (which is included inside of functions from the other files) would likely lead to invalid instruction encodings.

If you don't think ias can be fixed to recognize these instructions, we could work around it locally like

--- a/arch/arm/lib/memcpy.S
+++ b/arch/arm/lib/memcpy.S
@@ -15,7 +15,12 @@
 #define STR1W_SHIFT    0
 
        .macro ldr1w ptr reg abort
-       W(ldr) \reg, [\ptr], #4
+#if defined(CONFIG_LLVM_IAS) || !defined(CONFIG_THUMB2_KERNEL)
+       /* clang ias rejects the .w suffix on instructions that are already wide */
+       ldr \reg, [\ptr], #4
+#else
+       ldr.w \reg, [\ptr], #4
+#endif
        .endm
 
        .macro ldr4w ptr reg1 reg2 reg3 reg4 abort

    Arnd

nickdesaulniers · 2021-02-12T21:47:42Z

https://reviews.llvm.org/D96632

nickdesaulniers · 2021-02-12T23:32:48Z

I need to reread the ARM ARM more closely, but if LLVM folks agree that T4 should not have .w suffixes, we might be able to do:

diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
index f8016e3db65d..197cbb6005aa 100644
--- a/arch/arm/lib/copy_from_user.S
+++ b/arch/arm/lib/copy_from_user.S
@@ -60,7 +60,7 @@
 #define LDR1W_SHIFT    0
 
        .macro ldr1w ptr reg abort
-       USERL(\abort, W(ldr) \reg, [\ptr], #4)
+       USERL(\abort, ldr \reg, [\ptr], #4)
        .endm
 
        .macro ldr4w ptr reg1 reg2 reg3 reg4 abort
@@ -80,7 +80,7 @@
 #define STR1W_SHIFT    0
 
        .macro str1w ptr reg abort
-       W(str) \reg, [\ptr], #4
+       str \reg, [\ptr], #4
        .endm
 
        .macro str8w ptr reg1 reg2 reg3 reg4 reg5 reg6 reg7 reg8 abort
diff --git a/arch/arm/lib/copy_to_user.S b/arch/arm/lib/copy_to_user.S
index ebfe4cb3d912..ba58f84c3884 100644
--- a/arch/arm/lib/copy_to_user.S
+++ b/arch/arm/lib/copy_to_user.S
@@ -34,7 +34,7 @@
 #define LDR1W_SHIFT    0
 
        .macro ldr1w ptr reg abort
-       W(ldr) \reg, [\ptr], #4
+       ldr \reg, [\ptr], #4
        .endm
 
        .macro ldr4w ptr reg1 reg2 reg3 reg4 abort
@@ -77,7 +77,7 @@
 #define STR1W_SHIFT    0

        .macro ldr1w ptr reg abort
-       W(ldr) \reg, [\ptr], #4
+       ldr \reg, [\ptr], #4
        .endm
 
        .macro ldr4w ptr reg1 reg2 reg3 reg4 abort
@@ -31,7 +31,7 @@
        .endm
 
        .macro str1w ptr reg abort
-       W(str) \reg, [\ptr], #4
+       str \reg, [\ptr], #4
        .endm
 
        .macro str8w ptr reg1 reg2 reg3 reg4 reg5 reg6 reg7 reg8 abort
diff --git a/arch/arm/lib/memmove.S b/arch/arm/lib/memmove.S
index 6fecc12a1f51..35c5c06b7588 100644
--- a/arch/arm/lib/memmove.S
+++ b/arch/arm/lib/memmove.S
@@ -84,24 +84,24 @@ WEAK(memmove)
                addne   pc, pc, ip              @ C is always clear here
                b       7f
 6:             W(nop)
-               W(ldr)  r3, [r1, #-4]!
-               W(ldr)  r4, [r1, #-4]!
-               W(ldr)  r5, [r1, #-4]!
-               W(ldr)  r6, [r1, #-4]!
-               W(ldr)  r7, [r1, #-4]!
-               W(ldr)  r8, [r1, #-4]!
-               W(ldr)  lr, [r1, #-4]!
+               ldr     r3, [r1, #-4]!
+               ldr     r4, [r1, #-4]!
+               ldr     r5, [r1, #-4]!
+               ldr     r6, [r1, #-4]!
+               ldr     r7, [r1, #-4]!
+               ldr     r8, [r1, #-4]!
+               ldr     lr, [r1, #-4]!
 
                add     pc, pc, ip
                nop
                W(nop)
-               W(str)  r3, [r0, #-4]!
-               W(str)  r4, [r0, #-4]!
-               W(str)  r5, [r0, #-4]!
-               W(str)  r6, [r0, #-4]!
-               W(str)  r7, [r0, #-4]!
-               W(str)  r8, [r0, #-4]!
-               W(str)  lr, [r0, #-4]!
+               str     r3, [r0, #-4]!
+               str     r4, [r0, #-4]!
+               str     r5, [r0, #-4]!
+               str     r6, [r0, #-4]!
+               str     r7, [r0, #-4]!
+               str     r8, [r0, #-4]!
+               str     lr, [r0, #-4]!
 
        CALGN(  bcs     2b                      )

arndb · 2021-02-13T10:37:36Z

I need to reread the ARM ARM more closely, but if LLVM folks agree that T4 should not have .w suffixes, we might be able to do:

diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
index f8016e3db65d..197cbb6005aa 100644
--- a/arch/arm/lib/copy_from_user.S
+++ b/arch/arm/lib/copy_from_user.S
@@ -60,7 +60,7 @@
 #define LDR1W_SHIFT    0
 
        .macro ldr1w ptr reg abort
-       USERL(\abort, W(ldr) \reg, [\ptr], #4)
+       USERL(\abort, ldr \reg, [\ptr], #4)
        .endm

@ardbiesheuvel said that wouldn't work when I suggested the same patch earlier:

https://lore.kernel.org/lkml/CAMj1kXEx-mUCgX5F6xg8-6jKtpqQ=sRosmo4u-0jhW5zu9A-fw@mail.gmail.com/

ardbiesheuvel · 2021-02-13T10:47:44Z

I need to reread the ARM ARM more closely, but if LLVM folks agree that T4 should not have .w suffixes, we might be able to do:
diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
index f8016e3db65d..197cbb6005aa 100644
--- a/arch/arm/lib/copy_from_user.S
+++ b/arch/arm/lib/copy_from_user.S
@@ -60,7 +60,7 @@
 #define LDR1W_SHIFT    0
 
        .macro ldr1w ptr reg abort
-       USERL(\abort, W(ldr) \reg, [\ptr], #4)
+       USERL(\abort, ldr \reg, [\ptr], #4)
        .endm
@ardbiesheuvel said that wouldn't work when I suggested the same patch earlier:

https://lore.kernel.org/lkml/CAMj1kXEx-mUCgX5F6xg8-6jKtpqQ=sRosmo4u-0jhW5zu9A-fw@mail.gmail.com/

In this particular case, I agree that the .w suffix can be dropped. T4 is a wide encoding and it is the only one that has a post-indexed variant, so there is no ambiguity here.

However, the post-indexed variant of T4 is defined as

LDR{<c>}{<q>} <Rt>, [<Rn>], #{+/-}<imm>

where q represents the optional suffix, and so Clang is violating the architecture here by rejecting ldr.w as the mnemonic. Can we please fix this at a more fundamental level as well?

The Linux kernel when built with CONFIG_THUMB2_KERNEL makes use of these instructions with immediate operands and wide encodings. These are the T4 variants of the follow sections from the Arm ARM. F5.1.72 LDR (immediate) F5.1.229 STR (immediate) I wasn't able to represent these simple aliases using t2InstAlias due to the Constraints on the non-suffixed existing instructions, which results in some manual parsing logic needing to be added. F1.2 Standard assembler syntax fields describes the use of the .w (wide) vs .n (narrow) encoding suffix. Link: https://bugs.llvm.org/show_bug.cgi?id=49118 Link: ClangBuiltLinux/linux#1296 Reported-by: Stefan Agner <stefan@agner.ch> Reported-by: Arnd Bergmann <arnd@kernel.org> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed By: DavidSpickett Differential Revision: https://reviews.llvm.org/D96632

nickdesaulniers · 2021-02-23T17:26:49Z

https://reviews.llvm.org/rG1e204ac78952cf7e4fcf335423e80007388d3979

nickdesaulniers · 2021-02-23T17:33:17Z

Can we please fix this at a more fundamental level as well?

I would like to, adding individual aliases is tedious. I'm trying to think through valid logic for this, since not all instructions have wide AND narrow encodings (some have 1 or the other, or both, or multiple encodings).

pseudocode:

if !tryToMatchInstructionAsWritten() && endsWithNORW():
  suffix = stripOffNORW()
  instruction = tryToMatchInstructionWithoutSuffix()
  if instruction is 32b && suffix == 'w':
    return instruction
  if instruction is 16b && suffix == 'n':
    return instruction
  return error

Though I'm not sure that's how the assembler performs its table based parsing, or that I've seen enough of the instruction set to fully grasp all irregularities that might be related.

ardbiesheuvel · 2021-02-23T17:39:28Z

On Tue, 23 Feb 2021 at 18:33, Nick Desaulniers ***@***.***> wrote: Can we please fix this at a more fundamental level as well? I would like to, adding individual aliases is tedious. I'm trying to think through valid logic for this, since not all instructions have wide AND narrow encodings (some have 1 or the other, or both, or multiple encodings).

Not only that. The ARM ARM is clear about the .w prefix being permitted on all A32 (i.e., non-Thumb) encodings as well.

…

pseudocode: if !tryToMatchInstructionAsWritten() && endsWithNORW(): suffix = stripOffNORW() instruction = tryToMatchInstructionWithoutSuffix() if instruction is 32b && suffix == 'w': return instruction if instruction is 16b && suffix == 'n': return instruction return error Though I'm not sure that's how the assembler performs its table based parsing, or that I've seen enough of the instruction set to fully grasp all irregularities that might be related. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1296 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAOQWGWRXCOUVNOH5LJYJTTAPRGZANCNFSM4XJ2EI5A> .

nickdesaulniers · 2021-02-23T18:04:01Z

F1.2 Standard assembler syntax fields:
If neither .W nor .N is specified, the assembler can select either 16-bit or 32-bit encodings. If both
are available, it must select a 16-bit encoding.

So I guess I'd have to adjust my psuedocode.

The ARM ARM is clear about the .w prefix being permitted on
all A32 (i.e., non-Thumb) encodings as well.

Yes, from the same section:

When assembling to the A32 instruction set, the .N qualifier produces an assembler error and the .W
qualifier has no effect.

And I'd bet that's useful for assembler built both as ARM and THUMB2 under different build configs.

ardbiesheuvel · 2021-02-24T07:50:24Z

On Tue, 23 Feb 2021 at 19:04, Nick Desaulniers ***@***.***> wrote: F1.2 Standard assembler syntax fields: If neither .W nor .N is specified, the assembler can select either 16-bit or 32-bit encodings. If both are available, it must select a 16-bit encoding. So I guess I'd have to adjust my psuedocode. The ARM ARM is clear about the .w prefix being permitted on all A32 (i.e., non-Thumb) encodings as well. Yes, from the same section: When assembling to the A32 instruction set, the .N qualifier produces an assembler error and the .W qualifier has no effect. And I'd bet that's useful for assembler built both as ARM and THUMB2 under different build configs.

Yes, and this is actually rather common in the kernel. There are various places, both in crypto/ and in the core code, where the size of the opcodes matter, either because they are used in a computed goto, or because omitting the .w may result in a branch encoding that does not have sufficient range. However, GAS does not implement this correctly either, which is why we have the W() macro to begin with. But I don't think bug-for-bug compatibility with GAS is the goal here, so it would be far better to at least tolerate .w on any mnemonic, whether it has any effect or not.

The Linux kernel when built with CONFIG_THUMB2_KERNEL makes use of these instructions with immediate operands and wide encodings. These are the T4 variants of the follow sections from the Arm ARM. F5.1.72 LDR (immediate) F5.1.229 STR (immediate) I wasn't able to represent these simple aliases using t2InstAlias due to the Constraints on the non-suffixed existing instructions, which results in some manual parsing logic needing to be added. F1.2 Standard assembler syntax fields describes the use of the .w (wide) vs .n (narrow) encoding suffix. Link: https://bugs.llvm.org/show_bug.cgi?id=49118 Link: ClangBuiltLinux/linux#1296 Reported-by: Stefan Agner <stefan@agner.ch> Reported-by: Arnd Bergmann <arnd@kernel.org> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed By: DavidSpickett Differential Revision: https://reviews.llvm.org/D96632

nickdesaulniers added [TOOL] integrated-as The issue is relevant to LLVM integrated assembler [ARCH] arm32 This bug impacts ARCH=arm labels Feb 8, 2021

nickdesaulniers added the Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list. label Feb 9, 2021

nickdesaulniers added [PATCH] Submitted A patch has been submitted for review [BUG] llvm A bug that should be fixed in upstream LLVM labels Feb 12, 2021

nickdesaulniers mentioned this issue Feb 18, 2021

arch/arm/lib/memmove.S:86:3: error: invalid instruction #857

Closed

nickdesaulniers self-assigned this Feb 22, 2021

nickdesaulniers closed this as completed Feb 23, 2021

nickdesaulniers added [FIXED][LLVM] 13 This bug was fixed in LLVM 13.x and removed [PATCH] Submitted A patch has been submitted for review labels Feb 23, 2021

nickdesaulniers mentioned this issue Mar 1, 2021

clang-13 not getting upgraded? ClangBuiltLinux/continuous-integration2#95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

thumb missing .w InstAlias' for ldr and str #1296

thumb missing .w InstAlias' for ldr and str #1296

nickdesaulniers commented Feb 8, 2021

nickdesaulniers commented Feb 9, 2021 •

edited by nathanchance

Loading

nickdesaulniers commented Feb 9, 2021

nickdesaulniers commented Feb 9, 2021

arndb commented Feb 9, 2021

nickdesaulniers commented Feb 12, 2021

nickdesaulniers commented Feb 12, 2021 •

edited

Loading

arndb commented Feb 13, 2021

ardbiesheuvel commented Feb 13, 2021

nickdesaulniers commented Feb 23, 2021

nickdesaulniers commented Feb 23, 2021 •

edited

Loading

ardbiesheuvel commented Feb 23, 2021 via email

nickdesaulniers commented Feb 23, 2021

ardbiesheuvel commented Feb 24, 2021 via email

thumb missing .w InstAlias' for ldr and str #1296

thumb missing .w InstAlias' for ldr and str #1296

Comments

nickdesaulniers commented Feb 8, 2021

nickdesaulniers commented Feb 9, 2021 • edited by nathanchance Loading

nickdesaulniers commented Feb 9, 2021

nickdesaulniers commented Feb 9, 2021

arndb commented Feb 9, 2021

nickdesaulniers commented Feb 12, 2021

nickdesaulniers commented Feb 12, 2021 • edited Loading

arndb commented Feb 13, 2021

ardbiesheuvel commented Feb 13, 2021

nickdesaulniers commented Feb 23, 2021

nickdesaulniers commented Feb 23, 2021 • edited Loading

ardbiesheuvel commented Feb 23, 2021 via email

nickdesaulniers commented Feb 23, 2021

ardbiesheuvel commented Feb 24, 2021 via email

nickdesaulniers commented Feb 9, 2021 •

edited by nathanchance

Loading

nickdesaulniers commented Feb 12, 2021 •

edited

Loading

nickdesaulniers commented Feb 23, 2021 •

edited

Loading