[PATCH] D36732: [ARM, Asm] Harden GNU LDRD/STRD aliases against invalid inputs

[Oliver Stannard via Phabricator via llvm-commits]

olista01 added inline comments.


================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5791
+    return;
+  if (!Op3.isMem())
+    return;
----------------
rengolin wrote:
> These were asserts, now they're returns... I wonder if that's going to allow some weird cases. The assembler is not straight forward enough to not have weird matches going through behind our backs, and a future commit can mix this one up.
The idea is that, if the instruction doesn't match what we expect, we just don't change it, and let the asm parser report whatever error it would for the instruction as written.


================
Comment at: test/MC/ARM/ldrd-strd-gnu-sp.s:11
+
+// V7: error: invalid instruction
+// V8: ldrd    r12, sp, [r0, #32]      @ encoding: [0xd0,0xc2,0xc0,0xe1]
----------------
rengolin wrote:
> No note that this is valid in v8?
Since the instruction doesn't get transformed by fixupGNULDRDAlias, it has a missing operand compared to what the table-generated matcher expects, which isn't something we have a diagnostic for.

We could report a diagnostic from fixupGNULDRDAlias, but that would add a lot of complexity, because we'd want to report at least 2 notes: "requires V8", and "Rt must be in [r0,r11]". I'm not sure how commonly these aliases are used (I'm not sure why you'd want an instruction to implicitly read/write another register you didn't mention), so I didn't think that would be worth it.

What I would like to add (I've not tried this yet though) is different diagnostics for "unknown mnemonic" and "known mnemonic with invalid operands", which would at least give us something here.


================
Comment at: test/MC/ARM/ldrd-strd-gnu-thumb-bad-regs.s:5
 .thumb
-@ CHECK: error: operand must be a register in range [r0, r12] or r14
+@ CHECK: error: invalid instruction
 @ CHECK:         ldrd    r12, [r0, #512]
----------------
rengolin wrote:
> hum, I was expecting something a bit more explanatory here...
Same as above.


Repository:
  rL LLVM

https://reviews.llvm.org/D36732