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

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added subscribers: yursha, kcc.
vlad.tsyrklevich added a reviewer: pcc.
vlad.tsyrklevich added a comment.

Hi Joel, thanks for taking this on! Other than the comments mentioned inline, I'd also like to see a couple more tests, in particular:

- Currently there is a CFI-protected tiny.cc example, you should also an unprotected example.
- Given the special handling for vtable loads, it would be good to add a similar simple 'vtable.cc' example with a protected and an unprotected test to make sure that logic doesn't regress.

There are also unittests for llvm-cfi-verify. It would be worth adding a couple of tests for the 'can load' vtable logic to the FileAnalysis tests.



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


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:280
 
 uint64_t FileAnalysis::indirectCFOperandClobber(const GraphResult &Graph) const {
   assert(Graph.OrphanedNodes.empty() && "Orphaned nodes should be empty.");
----------------
Update the header comment for this to mention the new clobber evaluation logic.


================
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) {
----------------
I believe the loop to compute this below can be replaced by `Graph.flattenAddress(Node)`


================
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) {
----------------
Any reason to not use a range-for here?


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:324
+                                      *RegisterInfo)) {
+          if (canLoad && InstrDesc.mayLoad()) {
+            canLoad = false;
----------------
Invert the if condition here so that it's
```
if (...)
  return
....
```


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:325
+          if (canLoad && InstrDesc.mayLoad()) {
+            canLoad = false;
+            CurRegisterNumbers.erase(RI);
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D48836