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

Peter Collingbourne via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 12 12:41:10 PDT 2018


pcc 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";
----------------
jgalenson wrote:
> pcc wrote:
> > 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.
> That makes sense, but other similar MCID properties seem to work only one some arches (e.g., isAdd only works on ARM and Hexagon).  And the current behavior of cfi-verify on unsupported arches is to give an incorrect result, so at least this isn't a regression. :)  If we want this, I'd say we could just add an architecture check at the beginning of this tool.
I was mostly not thinking about llvm-cfi-verify but other tools or parts of the compiler that might end up trying to use the isTrap bit thinking that it would work on all architectures. But since those users are hypothetical maybe that's not too much of a concern.

Now that you point it out the architecture check might be a nice thing to have anyway.


https://reviews.llvm.org/D48836





More information about the llvm-commits mailing list