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

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: tools/llvm-cfi-verify/FileVerifier.cpp:108
+
+  if (KV++ == Instructions.end() || KV == Instructions.end())
+    return nullptr;
----------------
I think this would be clearer if you prefix incremented the second term instead of the first.


================
Comment at: tools/llvm-cfi-verify/FileVerifier.cpp:233
+    InstrMeta.SectionName = SectionName;
+    InstrMeta.Bad = false;
+
----------------
231-239 can be simplified to `InstrMeta.Bad = BadInstruction; addInstruction(InstrMeta); if (BadInstruction) continue;`


================
Comment at: tools/llvm-cfi-verify/FileVerifier.cpp:1
+#include "FileVerifier.h"
+
----------------
Missing header


================
Comment at: tools/llvm-cfi-verify/FileVerifier.cpp:73
+Error FileVerifier::printIndirectCFInstructions() const {
+  for (const auto &Address : IndirectInstructions) {
+    const auto &InstructionKV = Instructions.find(Address);
----------------
Seems clearer to specify Address' type here (e.g. is a reference needed given that it's a uint64_t?)


================
Comment at: tools/llvm-cfi-verify/FileVerifier.h:45
+
+// This class is used to verify CFI instrumentation of a single file.
+class FileVerifier {
----------------
This class seems like it's a whole program disassembly with a print method attached to report CFI status, does that seem fair? Would it make sense to rename it and split out the print method?


================
Comment at: tools/llvm-cfi-verify/FileVerifier.h:53
+    uint64_t InstructionSize; // Size of this instruction.
+    std::string SectionName;  // Section this instruction is found in.
+    bool Bad; // Is this instruction bad (in which case, Instr::Instruction is
----------------
Should this be a ptr/ref to avoid duplicating memory? Or should there just be a method that gets the SectionName by iterating through the Sections instead of storing it for every Instr?


================
Comment at: tools/llvm-cfi-verify/FileVerifier.h:54
+    std::string SectionName;  // Section this instruction is found in.
+    bool Bad; // Is this instruction bad (in which case, Instr::Instruction is
+              // invalid).
----------------
What is bad?


================
Comment at: tools/llvm-cfi-verify/FileVerifier.h:65
+  FileVerifier() = delete;
+  FileVerifier(const FileVerifier &) = delete;
+  FileVerifier(FileVerifier &&Other) = default;
----------------
Could we also include copy & move assignment constructors


================
Comment at: tools/llvm-cfi-verify/FileVerifier.h:79
+  // Returns a pointer to the previous/next instruction in sequence,
+  // respectively. Retusn nullptr if the next/prev instruction doesn't exist, or
+  // if the provided instruction doesn't exist.
----------------
typo: Retusn


================
Comment at: tools/llvm-cfi-verify/unittests/FileVerifier.cpp:65
+protected:
+  virtual void SetUp() {
+    if (Verifier.initialiseDisassemblyMembers()) {
----------------
This is run before every test, is that what you intended or is this meant to run only once (e.g. SetUpTestCase)?


================
Comment at: tools/llvm-cfi-verify/unittests/FileVerifier.cpp:67
+    if (Verifier.initialiseDisassemblyMembers()) {
+      exit(EXIT_FAILURE);
+    }
----------------
Is it possible this will mask failures? Other unit tests seem to eschew error checking here, presumably allowing unit tests to fail later.


https://reviews.llvm.org/D38379