Page MenuHomePhabricator

[ARM] permit PC as destination of MOVS
AbandonedPublic

Authored by nickdesaulniers on Jan 27 2021, 8:38 PM.

Details

Summary

While trying to assemble arch/arm/probes/kprobes/test-arm.c in the Linux
kernel with Clang's integrated assembler, I hit what seems to be a
discrepancy between GNU as (aka GAS) and Clang's integrated assembler.

It appears that GAS permits movs to use the PC (r15) as the destination
register. Further, the above code tests that this is possible.

Relax the requirement that the destination register not be the PC.

Link: https://github.com/ClangBuiltLinux/linux/issues/1271

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Jan 27 2021, 8:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 8:38 PM
nickdesaulniers edited the summary of this revision. (Show Details)Jan 27 2021, 8:46 PM
llvm/lib/Target/ARM/ARMInstrInfo.td
3599–3602

it might be of interest to note that MOVs with an immediate operand didn't have the same restriction on the destination.

llvm/lib/Target/ARM/ARMInstrInfo.td
6323

same

ARS, LSR, ROR have the following warning on the ARM ARM:

if d == 15 || n == 15 || m == 15 then UNPREDICTABLE;

I'm not sure GAS should be doing this at all... Looks like an oversight.

MOVs and RRX don't in the ARM version, but do in the Thumb2 ones.

However, not all encodings are allowed.

For example, MOVS PC is SUBS, but MOV PC is not allowed. OTOH, RRX only allows the PC is S isn't specified.

Maybe the kernel is using those instructions in the precise way the ARM ARM allows, but this patch is also allowing unpredictable encodings, which we really shouldn't.

  • limit changes to just MOVs with register operand

ARS, LSR, ROR have the following warning on the ARM ARM:

if d == 15 || n == 15 || m == 15 then UNPREDICTABLE;

Then why does llvm/test/MC/Disassembler/ARM/unpredictable-LSL-regform.txt test LSL, and not ARS, LSR, or ROR? (There are other tests llvm/test/MC/Disassembler/ARM/unpredictable-, but there don't seem to be tests for those).

I'm not sure GAS should be doing this at all... Looks like an oversight.

MOVs and RRX don't in the ARM version, but do in the Thumb2 ones.

However, not all encodings are allowed.

For example, MOVS PC is SUBS, but MOV PC is not allowed. OTOH, RRX only allows the PC is S isn't specified.

Maybe the kernel is using those instructions in the precise way the ARM ARM allows, but this patch is also allowing unpredictable encodings, which we really shouldn't.

Ah, looks like out of movs, lsl, lsr, asr, ror, and rrx, the kernel is only using movs with pc destination, in ARM mode (non-Thumb). I can restrict the patch to movs, though the test case in llvm/test/MC/Disassembler/ARM/unpredictable-LSL-regform.txt would still fail. And it still begs the question, why is it ok for the immediate operand versions of these to permit pc destination?

but this patch is also allowing unpredictable encodings, which we really shouldn't.

I guess I don't really follow whether the recommendation is to do something else in LLVM, or remove the kernel code?

nickdesaulniers retitled this revision from [ARM] permit PC as destination of LSL to [ARM] permit PC as destination of MOVS.Jan 29 2021, 10:39 AM
nickdesaulniers edited the summary of this revision. (Show Details)
nickdesaulniers added a reviewer: rovka.
rengolin added a comment.EditedJan 30 2021, 4:39 AM

Then why does llvm/test/MC/Disassembler/ARM/unpredictable-LSL-regform.txt test LSL, and not ARS, LSR, or ROR? (There are other tests llvm/test/MC/Disassembler/ARM/unpredictable-, but there don't seem to be tests for those).

Who knows why! :)

But we shouldn't rely on tests to tell us what the architecture does.

Ah, looks like out of movs, lsl, lsr, asr, ror, and rrx, the kernel is only using movs with pc destination, in ARM mode (non-Thumb). I can restrict the patch to movs, though the test case in llvm/test/MC/Disassembler/ARM/unpredictable-LSL-regform.txt would still fail.

I figured as much. Hand-written ARM assembly is usually in ARM mode, especially kernel stuff. So much so that GNU inline assembly assumes ARM mode.

And it still begs the question, why is it ok for the immediate operand versions of these to permit pc destination?

That's a hardware design decision, documented in the manual, which compilers have to follow, so we need to implement it as is.

I guess I don't really follow whether the recommendation is to do something else in LLVM, or remove the kernel code?

The recommendation will always be to "do the right thing in LLVM and change user code to match". We have fixed a staggering amount of bugs and broken code in the kernel, android and chromium with that attitude.

However, in this case in particular, it seems the kernel code isn't wrong (ie. it uses the allowed exceptions in MOVs only), so just adding the exception for MOVs would probably fix the kernel problem without making LLVM generate code that is unpredictable.

Not much to add over what Renato has said. I've put some context into the special meaning of MOVS with the PC as destination. I'm not entirely sure why the MOVS with immediate is unrestricted, my guess is that it is a historical quirk of the encoding and has likely never been used.

MOVS with the PC as destination has a special meaning in AArch32 for returning from exceptions (see examples in https://developer.arm.com/documentation/dui0056/d/handling-processor-exceptions/entering-and-leaving-an-exception/the-return-address-and-return-instruction). It has the extra property of restoring the CPSR from the banked SPSR. I would expect that the kernel code is doing an exception return as I think its use outside of that is CONSTRAINED UNPREDICTABLE. There aren't that many alternatives, RFE and something like LDMFD sp!,{R0-R12,pc}^ (the ^ indicates an exception return) coming to mind.

It will be worth looking at the Arm ARM for the conditions

F5.1.112 MOV, MOVS (register)
PC as destination:

The MOVS variant of the instruction performs an exception return without the use of the stack. In this case:
— The PE branches to the address written to the PC, and restores PSTATE from SPSR_<current_mode>.
— The PE checks SPSR_<current_mode> for an illegal return event. See Illegal return events from
AArch32 state on page G1-5524.
— The instruction is UNDEFINED in Hyp mode.
— The instruction is CONSTRAINED UNPREDICTABLE in User mode and System mode.

Later in Assembler Symbols (Arm encoding)

For the MOVS variant, the instruction performs an exception return, that restores PSTATE
from SPSR_<current_mode>. Arm deprecates use of the instruction if <Rn> is not the LR,
or if the optional shift or RRX argument is specified.

Which implies that ideally only MOVS pc, lr should be used, but deprecation doesn't mean can't be used.

F5.1.113 MOV, MOVS (register-shifted register)
Does not permit use of the PC in the registers if d == 15 || m == 15 || s == 15 then UNPREDICTABLE;

llvm/test/MC/ARM/lsl-zero-errors.s
157

I think this is unpredictable as it is the encoding MOV, MOVS (register-shifted register) which does not permit PC as the destination, only MOV, MOVS (register) permits PC as the destination (the encoding allows a rotation, but not shift).

My reading of the manual says this change is correct. All the exceptions and deprecations in MOVs and LSL seem to not be in ARM mode with the S flag set and without the PC in Rm (source register).

But it's been a while since I reviewed ARM ASM code especially those around so many exceptions. So I'd like people that are still actively looking at ARM code to review this (@psmith, @rovka).

llvm/test/MC/Disassembler/ARM/unpredictable-LSL-regform.txt
12

Actually, this is an unpredictable test. so we either change it to add the actual unpredictable instructions or we delete the test altogether.

rengolin added inline comments.Jan 30 2021, 5:00 AM
llvm/test/MC/ARM/lsl-zero-errors.s
157

Argh, I missed that! You're right, this is wrong. I wonder if this isn't reuse of table-gen encoding (like we have for others) for the shifted case and the reason LLVM hasn't used it so far?

Sorry, it's still not clear to me what I should do to proceed.

The kernel has the following block of code:

 167   /* Data-processing with PC as a target and status registers updated */                                                                                                   
 168   TEST_UNSUPPORTED("movs  pc, r1")                                                                                                                                         
 169   TEST_UNSUPPORTED("movs  pc, r1, lsl r2")                 
^ this line fails with clang's integrated assembler.                                                                                                                
 170   TEST_UNSUPPORTED("movs  pc, #0x10000")                                                                                                                                   
 171   TEST_UNSUPPORTED("adds  pc, lr, r1")                                                                                                                                     
 172   TEST_UNSUPPORTED("adds  pc, lr, r1, lsl r2")                                                                                                                             
 173   TEST_UNSUPPORTED("adds  pc, lr, #4")

I think it might seem silly to kernel reviewers if I send them a patch that removes movs pc, r1, lsl r2 when there's a movs pc, r1 right above it that's accepted.

If I should send a kernel patch, how should I justify it?

If we should support this form of this instruction with these operands, how should I proceed on this patch?

This is literally the last thing blocking me from building a 32b ARM "allyesconfig" Linux kernel with Clang's integrated assembler, which I would like to ship in the next release of Android. (THUMB2 has it's own issues, but is not part of "allyesconfig.") I have about 2 months until we begin to lock down the toolchain release for that version of Android.

nickdesaulniers added a comment.EditedFeb 3 2021, 11:55 AM
$ cat foo.s
movs  pc, r1, lsl r2
$ arm-linux-gnueabi-as foo.s -o foo.o
foo.s: Assembler messages:
foo.s:1: Warning: using r15 results in unpredictable behaviour
$ arm-linux-gnueabi-objdump -d foo.o    

foo.o:     file format elf32-littlearm


Disassembly of section .text:

00000000 <.text>:
   0:   e1b0f211        lsls    pc, r1, r2      ; <UNPREDICTABLE>

so GNU as will assemble it, but GNU objdump acknowledges that this encoding is unpredictable (or does <UNPREDICTABLE> mean that modifying the program counter results in unpredictable control flow?).

So I guess I should:

  1. send a kernel patch removing that test.
  2. abandon this patch

Is that correct?

nickdesaulniers added a comment.EditedFeb 3 2021, 11:57 AM

In fact, of:

movs  pc, r1
movs  pc, r1, lsl r2
movs  pc, #0x10000

GNU objdump only warns with <UNPREDICTABLE> for the second form (which is the form this patch is concerned with). That seems like good justification for removing the test from the kernel.

It looks like GNU objdump also warns <UNPREDICTABLE> for adds pc, lr, r1, lsl r2 and yet the integrated assembler accepted that, so it looks like a pre-existing bug/inconsistency in support for unpredictable encodings.

GNU objdump only warns with <UNPREDICTABLE> for the second form (which is the form this patch is concerned with). That seems like good justification for removing the test from the kernel.

Agreed.

It looks like GNU objdump also warns <UNPREDICTABLE> for adds pc, lr, r1, lsl r2 and yet the integrated assembler accepted that, so it looks like a pre-existing bug/inconsistency in support for unpredictable encodings.

Indeed. Limiting this would be a patch to LLVM.

As I understand it, clang is correct to reject movs pc, r1, lsl r2 as this is unpredictable.

(mostly the same references Peter used, but this confusing enough I'll restate them)

F5.1.113 MOV, MOVS (register-shifted register)

Decode for all variants of this encoding
 d = UInt(Rd); m = UInt(Rm); s = UInt(Rs);
 setflags = (S == '1'); shift_t = DecodeRegShift(stype);
 if d == 15 || m == 15 || s == 15 then UNPREDICTABLE

The other one is merely "deprecated" and I don't know about GAS but we certainly don't have any way to signal that, so accepting it is fine. You can justify that by citing F5.1.112 MOV, MOVS (register) in the latest ARMARM.
The actual wording is rather confusing (and potentially has some typos, I'll flag this up) but the main point is that the only conditions listed there are for "deprecated" not "unpredictable" and that's all the assembler needs to know.

The reasoning, I think Peter's explanation about the exception handling case covers it as best I could. I don't think you need that reasoning to justify clang's choice though.

If I should send a kernel patch, how should I justify it?

If we should support this form of this instruction with these operands, how should I proceed on this patch?

Send a kernel patch, with the justification that the instruction is unpredictable according to the spec and so GAS should be rejecting it anyway, just like clang.

There are other lines that encode unpredictable instructions but use the encoding directly to subvert GAS's checks so you could do that:

TEST_UNSUPPORTED(__inst_arm(0xe15c0f1e) " @ cmp r12, r14, asl pc"

No mysterious gap in the tests that way.

FWIW UNPREDICTABLE in the Architecture, is essentially anything can happen. There is a more specific CONSTRAINED UNPREDICTABLE, which for each case states a number of possible behaviours for the implementation.

The term UNPREDICTABLE describes a number of cases where the architecture has a feature that software must not
use. For execution in AArch32 state, where previous versions of the architecture define behavior as
UNPREDICTABLE, the Armv8-A architecture specifies a narrow range of permitted behaviors. This range is the range
of CONSTRAINED UNPREDICTABLE behavior. All implementations that are compliant with the architecture must
follow the CONSTRAINED UNPREDICTABLE behavior
nickdesaulniers added a comment.EditedFeb 5 2021, 2:19 PM

FWIW UNPREDICTABLE in the Architecture, is essentially anything can happen. ...

As I understand it, clang is correct to reject movs pc, r1, lsl r2 as this is unpredictable.
...
There are other lines that encode unpredictable instructions but use the encoding directly to subvert GAS's checks so you could do that:

TEST_UNSUPPORTED(__inst_arm(0xe15c0f1e) " @ cmp r12, r14, asl pc"

No mysterious gap in the tests that way.

I would think that if "essentially anything can happen," then all UNPREDICTABLE tests should be removed from the kernel? Whether they use the instruction mnemonics or open code the instruction/operands in hex.

But I have submitted a kernel patch to LKML, let's see what the feedback is there.
https://lore.kernel.org/linux-arm-kernel/20210205223557.3097894-1-ndesaulniers@google.com/T/#u

I would think that if "essentially anything can happen," then all UNPREDICTABLE tests should be removed from the kernel? Whether they use the instruction mnemonics or open code the instruction/operands in hex.

Only if those tests run the instructions. My understanding was that this test checks that you can't put a kprobe on the instruction, so it's not running them. If it was/is then there's a debate to be had.

ACK

It looks like the kernel also uses

movs pc, lr

for thumb2 kernel images. Maybe I should repurpose this review for supporting pc operand in thumb? llvm/test/MC/ARM/thumb-mov.s/llvm/test/MC/ARM/lsl-zero-errors.s, or create a new review?

ACK

It looks like the kernel also uses

movs pc, lr

for thumb2 kernel images. Maybe I should repurpose this review for supporting pc operand in thumb? llvm/test/MC/ARM/thumb-mov.s/llvm/test/MC/ARM/lsl-zero-errors.s, or create a new review?

err, is there a way to express that an inst alias should pass a literal 0?

diff --git a/llvm/lib/Target/ARM/ARMInstrThumb2.td b/llvm/lib/Target/ARM/ARMInstrThumb2.td
index 5642cab32e7c..82f9096957eb 100644
--- a/llvm/lib/Target/ARM/ARMInstrThumb2.td
+++ b/llvm/lib/Target/ARM/ARMInstrThumb2.td
@@ -4144,6 +4144,7 @@ def t2SUBS_PC_LR : T2I <(outs), (ins imm0_255:$imm), NoItinerary,
   bits<8> imm;
   let Inst{7-0} = imm;
 }
+def : t2InstAlias<"movs pc, lr", (t2SUBS_PC_LR imm0_255:"0")>;
 
 // Hypervisor Call is a system instruction.
 let isCall = 1 in {
llvm-project/llvm/lib/Target/ARM/ARMInstrThumb2.td:4147:57: error: expected variable name in dag literal
def : t2InstAlias<"movs pc, lr", (t2SUBS_PC_LR imm0_255:"0")>;
                                                        ^

???

diff --git a/llvm/lib/Target/ARM/ARMInstrThumb2.td b/llvm/lib/Target/ARM/ARMInstrThumb2.td
index 5642cab32e7c..d21fe9d04095 100644
--- a/llvm/lib/Target/ARM/ARMInstrThumb2.td
+++ b/llvm/lib/Target/ARM/ARMInstrThumb2.td
@@ -4144,6 +4144,8 @@ def t2SUBS_PC_LR : T2I <(outs), (ins imm0_255:$imm), NoItinerary,
   bits<8> imm;
   let Inst{7-0} = imm;
 }
+def : t2InstAlias<"movs{$p}\tpc, lr", (t2SUBS_PC_LR 0, pred:$p)>;
+def : t2InstAlias<"movs{$p}.w\tpc, lr", (t2SUBS_PC_LR 0, pred:$p)>;
 
 // Hypervisor Call is a system instruction.
 let isCall = 1 in {

seems to work, will work on fixing the up tests more tomorrow

nickdesaulniers abandoned this revision.Feb 10 2021, 11:02 AM