[PATCH] D48836: [cfi-verify] Support AArch64.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc accepted this revision.
pcc added a comment.

LGTM



================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:157
 bool FileAnalysis::isCFITrap(const Instr &InstrMeta) const {
-  return MII->getName(InstrMeta.Instruction.getOpcode()) == "TRAP";
+  return MII->getName(InstrMeta.Instruction.getOpcode()) == "TRAP" ||
+         MII->getName(InstrMeta.Instruction.getOpcode()) == "BRK";
----------------
jgalenson wrote:
> vlad.tsyrklevich wrote:
> > jgalenson wrote:
> > > pcc wrote:
> > > > vlad.tsyrklevich wrote:
> > > > > Instead of doing string comparisons here for every valid value, lets define a TrapOpcode variable during initialization to X86::TRAP / AArch64::BRK depending on the target. (We can bail on other architectures that we haven't explicitly tested on to avoid confusing users who might otherwise believe the results.)
> > > > I don't think we can access backend-specific things like X86::TRAP here though. It might be worth adding an interface to MCInstrAnalysis for asking what the trap instruction is and have that assert on architectures that we don't support yet.
> > > I had envisioned adding a new entry to MCID::Flag so that instruction definitions in .td files could contain IsTrap = 1 on the trap instructions.  And as I said, that seemed the cleanest and most correct option but was quite a large change, especially if we only ever need two architectures.  Adding a new method to MCInstrAnalysis seems a lot simpler, although I like it slightly less than my way.  Any preferences?
> > I don't have a preference between the two.
> I ended up going the TableGen route since it seemed more correct to me (all the other MCInstrAnalysis methods default to calling the MCInstrDesc method anyway).  I'm not sure how to test it by itself, but I couldn't find tests for another of the flags, so hopefully this is good enough.
Well, if this will only ever work on two architectures, wouldn't it be better to implement it on MCInstrAnalysis rather than in tablegen so that we will assert on the unsupported architectures instead of potentially giving an incorrect result? I don't feel too strongly about that, though.


https://reviews.llvm.org/D48836