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

Tue Aug 15 05:04:58 PDT 2017

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Ok, LGTM. Thanks!

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5791
+    return;
+  if (!Op3.isMem())
+    return;
----------------
olista01 wrote:
> 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.
Right, makes sense.

================
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]
----------------
olista01 wrote:
> 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.
Right, I agree the mnemonic messages are a better way out of this mess.

Repository:
  rL LLVM

https://reviews.llvm.org/D36732