[PATCH] D37937: Introduce the llvm-cfi-verify tool.

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: docs/CFIVerify.rst:37
+implemented around all indirect control flows by analysing the output machine
+code. The analysis of machine code is important as it ensure that any bugs
+present in linker or compiler do not subvert CFI protections in the final
----------------
ensures


================
Comment at: docs/CFIVerify.rst:55
+This tool will disassemble binaries and DSO's from their machine code format and
+analyse the disassembled bytecode. The tool will inspect virtual calls and
+indirect function calls. This tool will also inspect indirect jumps, as inlined
----------------
nit: bytecode is traditionally used to refer to instructions run by a VM/interpreter, replace with 'machine code' instead


================
Comment at: tools/llvm-cfi-verify/llvm-cfi-verify.cpp:1
+//===-- llvm-cfi-verify.cpp - CFI Verification tool for LLVM --------------===//
+//
----------------
Run clang-format


================
Comment at: tools/llvm-cfi-verify/llvm-cfi-verify.cpp:212
+    if (Section.getContents(SectionContents)) {
+      outs() << "Failed to retrieve section contents.\n";
+      return EXIT_FAILURE;
----------------
`errs()`


================
Comment at: tools/llvm-cfi-verify/llvm-cfi-verify.cpp:235
+      // Skip instructions that do not affect the control flow.
+      const auto& InstrDesc = MII->get(Instruction.getOpcode());
+      if (BadInstruction ||
----------------
This can assert() on a bad opcode, should we check `if (BadInstruction)` before this?


================
Comment at: tools/llvm-cfi-verify/llvm-cfi-verify.cpp:243
+      bool UsesRegisterOperand = false;
+      for (unsigned i = 0; i < Instruction.getNumOperands(); ++i) {
+        if (Instruction.getOperand(i).isReg()) {
----------------
I believe you can use a range-based for loop here: `for (auto &Op : Instruction)`


https://reviews.llvm.org/D37937