[PATCH] D39764: [cfi-verify] Made FileAnalysis operate on a GraphResult rather than build one and validate it.

Vlad Tsyrklevich via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 9 12:04:06 PST 2017


vlad.tsyrklevich added inline comments.


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:70
+  }
+  return "FAIL_UNKNOWN";
+}
----------------
This should be an llvm_unreachable()


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.h:52
+enum CFIProtectionStatus {
+  PROTECTED,            // This instruction is protected by CFI.
+  FAIL_NOT_INDIRECT_CF, // The instruction is not an indirect control flow
----------------
Could we move the comments above the entries?


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.h:59
+                               // check the destination for this vcall/icall.
+  FAIL_UNKNOWN, // Something strange happened, e.g. the instruction was not in
+                // this file.
----------------
This is only returned in one place so why not make it a specific error, rather than a generic catch-all?


================
Comment at: unittests/tools/llvm-cfi-verify/FileAnalysis.cpp:502
+  EXPECT_EQ(FAIL_NOT_INDIRECT_CF, Analysis.validateCFIProtection(Result));
+  Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADC0DE);
+  EXPECT_EQ(FAIL_UNKNOWN, Analysis.validateCFIProtection(Result));
----------------
Could we make this an easier to differentiate constant? Took me a second to understand why this was failing on not finding the instruction


https://reviews.llvm.org/D39764





More information about the llvm-commits mailing list