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

[Joel Galenson via Phabricator via llvm-commits]

jgalenson added a comment.

Thanks for the comments!  I'll try to update the patch on Friday.

Sorry for the lack of tests; I was having trouble compiling testcases the right way, but I think I have it figured out now.



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


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:305
+    // We walk backwards from the indirect CF.
+    SmallVector<uint64_t, 4> Nodes;
     while (Node != Graph.BaseAddress) {
----------------
vlad.tsyrklevich wrote:
> I believe the loop to compute this below can be replaced by `Graph.flattenAddress(Node)`
Ah, that looks like just what I want.  Thanks!


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:319
 
-      for (unsigned RegNum : RegisterNumbers) {
+      for (auto RI = CurRegisterNumbers.begin(), RE = CurRegisterNumbers.end();
+           RI != RE; ++RI) {
----------------
vlad.tsyrklevich wrote:
> Any reason to not use a range-for here?
The reason is that on line 326 when I removed the defined register from the target set I needed an iterator not the value, and it seemed more efficient to reuse an iterator than call find.  I'd be happy to lose a bit of performance to improve readability if you'd prefer me to use a foreach loop.


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:325
+          if (canLoad && InstrDesc.mayLoad()) {
+            canLoad = false;
+            CurRegisterNumbers.erase(RI);
----------------
vlad.tsyrklevich wrote:
> This variable is set outside of the for loop so it will still be false for the next conditional branch node we iterate over. Please add a test for this case.
Good point, I'll move the definition inside the outermost for loop.

The reason I didn't write a testcase for this was that I wasn't sure how to construct one.  Are there existing tests that create this pattern, or should I write a unit test, which is presumably simpler to create?


Repository:
  rL LLVM

https://reviews.llvm.org/D48836