[PATCH] [AArch64] Fix problems in handling generic MSR/MRS instructions

[Renato Golin]

@t.p.northover, @jmolloy, please have a look. I'm probably being overzealous, but I'd rather be safe than sorry.


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp:1506
@@ +1505,3 @@
+  // If the name is not valid then the instruction must be threated as the
+  // extended MSR.
+  // FIXME This should be specified in TableGen but it is not currently
----------------
petpav01 wrote:
> rengolin wrote:
> > This seems like a gross exaggeration. Is it possible that the name would come invalid for any other reason?
> I am not sure if I am really answering your question but `DecodeSystemPStateInstruction()` is called only for `MSRpstate` and this instruction aliases with an extended `MSR` format (introduced in r218753). All cases not allowed by `MSRpstate` are valid `MSR` cases.
> 
> 
> Some more background on this code:
> 
> MC instruction `MSRpstate` is defined to have:
>   Inst{31-19} = 0b1101010100000;
>   Inst{18-16} = pstatefield{5-3};
>   Inst{15-12} = 0b0100;
>   Inst{11-8} = imm;
>   Inst{7-5} = pstatefield{2-0};
>   Inst{4-0} = 0b11111
> 
> A problem here is that there does not seem to be a good way how to specify that only a subset of `pstatefield` is allowed.
> 
> Disassembling a bitpattern that should be handled as an extended `MSR` (but which aliases `MSRpstate`):
>   $ echo "0x1f 0x40 0x00 0xd5" | ./llvm-mc -triple=aarch64 -show-inst -show-encoding -disassemble -debug
>   Args: ./llvm-mc -triple=aarch64 -show-inst -show-encoding -disassemble -debug 
>           .text
>   0: OPC_ExtractField(26, 3): 5
>   3: OPC_FilterValue(2, 1110): FAIL: continuing at 1117
>   1117: OPC_FilterValue(3, 28396): FAIL: continuing at 29517
>   29517: OPC_FilterValue(4, 447): FAIL: continuing at 29968
>   29968: OPC_FilterValue(5, 474): PASS: continuing at 29972
>   29972: OPC_ExtractField(29, 3): 6
>   29975: OPC_FilterValue(0, 4): FAIL: continuing at 29983
>   29983: OPC_FilterValue(1, 39): FAIL: continuing at 30026
>   30026: OPC_FilterValue(2, 17): FAIL: continuing at 30047
>   30047: OPC_FilterValue(4, 4): FAIL: continuing at 30055
>   30055: OPC_FilterValue(5, 39): FAIL: continuing at 30098
>   30098: OPC_FilterValue(6, 10539): PASS: continuing at 30102
>   30102: OPC_ExtractField(21, 5): 8
>   30105: OPC_FilterValue(0, 30): FAIL: continuing at 30139
>   30139: OPC_FilterValue(1, 10): FAIL: continuing at 30153
>   30153: OPC_FilterValue(2, 11): FAIL: continuing at 30168
>   30168: OPC_FilterValue(5, 30): FAIL: continuing at 30202
>   30202: OPC_FilterValue(8, 122): PASS: continuing at 30206
>   30206: OPC_ExtractField(0, 8): 31
>   30209: OPC_FilterValue(95, 11): FAIL: continuing at 30224
>   30224: OPC_FilterValue(159, 11): FAIL: continuing at 30240
>   30240: OPC_FilterValue(191, 11): FAIL: continuing at 30256
>   30256: OPC_FilterValue(223, 11): FAIL: continuing at 30272
>   30272: OPC_ExtractField(0, 5): 31
>   30275: OPC_FilterValue(31, 33): PASS: continuing at 30279
>   30279: OPC_ExtractField(12, 4): 4
>   30282: OPC_FilterValue(2, 11): FAIL: continuing at 30297
>   30297: OPC_FilterValue(4, 11): PASS: continuing at 30301
>   30301: OPC_CheckField(19, 1, 0, 5): FieldValue = 0, ExpectedValue = 0: PASS
>   30307: OPC_Decode: opcode 1124, using decoder 216
>   ----- DECODE SUCCESSFUL -----
>   <stdin>:1:1: warning: invalid instruction encoding
>   0x1f 0x40 0x00 0xd5
>   ^
> 
> My understanding of the generated disassembler is that it works in two phases. In the first phase (before "DECODE SUCCESSFUL") the disassembler matches all constant bits and determines an MC opcode. In the second phase it calls a decode method specific to the determined opcode. In this case it calls explicit decode method `DecodeSystemPStateInstruction()`, it sees that `pstatefield` is not one of `SPSel`, `DAIFSet`, `DAIFClr` and rejects the instruction. No programmatic limitation on field values (without changing the resulting opcode) in the first phase, or a jump from the second phase back to the first one to handle the instruction as an extended `MSR` (after it is rejected as `MSRpstate`) seems possible.
> 
> I also considered an option to instantiate only the allowed `MSR` (immed) instructions (e.g. to have `MSRpstateSPSel`, `MSRpstateDAIFSet`, `MSRpstateDAIFClr`) but that does not look right because it creates a new MC opcode for each allowed `pstatefield` value.
> 
> A similar problem exists in the ARM backend where the preload hints fit into holes in the encodings of the `LDRH/STRH` instructions. It seems to be handled similarly in the instruction decode method.
I understand why it's safe to do that in your case, but I worry that there would be other cases where an invalid name won't match in your pattern. What I'm saying is that I'd like to see a way of that function return Invalid for all the cases where it IS invalid, even with your extension.

I don't know all the variations that function can cope with, but I'd be surprised if all validation on all fields was made before entering this function. You may get it right on your examples, but that doesn't mean there are other examples where that would get it wrong.

Basically, you need an active selection to make a conscious choice of acceptance and reject the unknown, instead of blindly convert whatever comes into an MSR, even if it was an MSR to begin with, it could have wrong operand values, etc.

Tim and James may have a better idea of how to do that.

http://reviews.llvm.org/D7174

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/