[PATCH] D38379: Classify llvm-cfi-verify.

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 7 inline comments as done.
hctim added inline comments.


================
Comment at: tools/llvm-cfi-verify/FileAnalysis.h:140
+  // A list of addresses of indirect control flow instructions.
+  std::set<uint64_t> IndirectInstructions;
+};
----------------
vlad.tsyrklevich wrote:
> I kept coming back to this trying to figure out what IndirectInstructions were, rename to IndirectBranches/getIndirectBranches()?
I've been trying to keep the rhetoric 'indirect instructions' to cover both indirect branches and indirect calls.

Would you prefer IndirectCallsAndJumps?


================
Comment at: tools/llvm-cfi-verify/unittests/FileVerifier.cpp:65
+protected:
+  virtual void SetUp() {
+    if (Verifier.initialiseDisassemblyMembers()) {
----------------
vlad.tsyrklevich wrote:
> This is run before every test, is that what you intended or is this meant to run only once (e.g. SetUpTestCase)?
Yep, was intended. Setup of the class via the test constructor is pretty cheap and an easy way to reset the internal members to start with a clean slate each time.


================
Comment at: tools/llvm-cfi-verify/unittests/FileVerifier.cpp:67
+    if (Verifier.initialiseDisassemblyMembers()) {
+      exit(EXIT_FAILURE);
+    }
----------------
vlad.tsyrklevich wrote:
> Is it possible this will mask failures? Other unit tests seem to eschew error checking here, presumably allowing unit tests to fail later.
I don't think so, given that we're exiting on a nonzero error code. I've changed this to gtest's FAIL() macro instead though so GTEST can clean up after itself and show the problems in a pretty red colour.


https://reviews.llvm.org/D38379