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

[Joel Galenson via Phabricator via llvm-commits]

jgalenson added inline comments.


================
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";
----------------
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.


https://reviews.llvm.org/D48836