[PATCH] D38427: Graph builder implementation.

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.cpp:96
+  // Resolve the metadata for the provided node.
+  const auto &BranchInstrMetaPtr = Analysis.getInstruction(BranchNode.Address);
+  if (!BranchInstrMetaPtr) {
----------------
Why not just pass this in?


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.cpp:108
+
+  // Find out the next instruction in the chain and add it to the new node.
+  if (BranchNode.Target && !BranchNode.Fallthrough) {
----------------
s/chain/basic block/?


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.cpp:152
+
+  // Now the branch head has been set properly, complete the rest of the chain.
+  for (uint64_t i = 1; i < SearchLengthForUndef; ++i) {
----------------
ditto s/chain/basic block/?


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.cpp:154
+  for (uint64_t i = 1; i < SearchLengthForUndef; ++i) {
+    // Check to see whether the chain should die.
+    if (Analysis.isCFITrap(*CurrentMetaPtr)) {
----------------
ditto


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.cpp:176
+
+  // Final check of the last thing we added to the chain.
+  if (Analysis.isCFITrap(*CurrentMetaPtr))
----------------
ditto


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.cpp:196
+  // If this flow is already explored, stop here.
+  if (Result.IntermediateNodes.count(Address)) {
+    return;
----------------
nit: eliminate braces


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.cpp:227
+                         Depth + 1);
+      Result.IntermediateNodes[ParentMeta.VMAddress] = Address;
+      HasValidCrossRef = true;
----------------
This is executed here, 240, 253, 260, and 270. Any reason it can't just be done once above? Is it to avoid the ConditionalBranchNode case?


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.h:57
+  uint64_t Fallthrough;
+  // Does this conditional branch implement CFI successfully?
+  bool CFIProtection;
----------------
Could this be reworded? I'm still not sure what it means.


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.h:86
+  // Returns an in-order list of the path between the address provided and the
+  // base. This function is only valid for
+  std::vector<uint64_t> flattenAddress(uint64_t Address) const;
----------------
Sentence cuts off


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.h:94
+  // will enumerate all branch nodes that can lead to this node, and provide
+  // them, fully evaulated, in GraphResult::BranchNodes. It will also provide
+  // any orphaned (i.e. did not make it to a branch node) flows to the provided
----------------
fully evaluated, or enumerated? Not sure what evaluated might refer to in this context.


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.h:108
+  // `Result.OrphanedNodes`. `OpenedNodes` keeps track of the list of nodes
+  // in the current path and is used for acyclic-checking. If the path is found
+  // to be cyclic, it will be added to `Result.OrphanedNodes`.
----------------
s/acyclic/cycle/


https://reviews.llvm.org/D38427