[PATCH] D38427: Graph builder implementation.

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.cpp:232
+
+    // Finalise ready for the next loop.
+    CurrentMetaPtr = NextMetaPtr;
----------------
Reword this.


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.cpp:293
+      continue;
+    } else {
+      // Evaluate the branch target to ascertain whether this XRef is the result
----------------
Since the if() statement continue;s as the end, place the below without an else statement to avoid nesting (makes it a bit easier to read and understand.)


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.cpp:310
+        if (BranchTarget == ChildMeta.VMAddress) {
+          // Ensure the unconditional branch can't fall back on itself. Check
+          // the current tree.
----------------
Reword 'fall back on itself'


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.cpp:313
+          bool IsUniqueInTree = true;
+          for (const auto &InstructionAddressInTree :
+               ChildNode->flattenControlFlowNodeAddresses()) {
----------------
Replace with `std::find_if()` ?


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.cpp:326
+
+          ControlFlowNode *NewNode = new ControlFlowNode();
+          NewNode->Next = ChildNode;
----------------
Why not use `createParentNode()` here?


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.cpp:335
+        } else {
+          errs() << "Control flow to " << format_hex(ChildMeta.VMAddress, 2)
+                 << ", but target resolution of "
----------------
Place the error first, and also avoid the nesting here as well.


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:76
+  uint64_t BaseInstructionAddress;
+  std::vector<ConditionalBranchNode *> BranchNodes;
+  std::vector<ControlFlowNode *> OrphanedNodes;
----------------
hctim wrote:
> vlad.tsyrklevich wrote:
> > Why are these pointers?
> Don't want to mix storage (stack/heap) of nodes. Intermediate nodes cannot be stored on stack, as they are neither a branch node or an orphaned node. Consider the following:
> 
> ```
> [0x0: jmp 2]
>        \
>   [0x2: jmpq %rax]
> ```
> 
> `0x2` is neither a branch node, nor an orphaned node. The only reference to it is via `0x0`, which is a branch node.
> 
> Having all these nodes be heap-allocated makes the clean up much easier.
Would it make sense to just place ConditionalBranchNodes in a vector without dynamically allocating them so they don't have to be manually freed?


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:53
+struct ControlFlowNode {
+  void printControlFlowNode(unsigned depth, raw_ostream& os) const;
+  std::vector<uint64_t> flattenControlFlowNodeAddresses() const;
----------------
just `print(`


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:54
+  void printControlFlowNode(unsigned depth, raw_ostream& os) const;
+  std::vector<uint64_t> flattenControlFlowNodeAddresses() const;
+
----------------
just `flatten(`


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:62
+struct ConditionalBranchNode {
+  void printConditionalBranchNode(raw_ostream& os) const;
+  ControlFlowNode *Target;
----------------
just `print(`


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:66
+  uint64_t InstructionAddress;
+  // Does this conditional branch successfully instrument CFI protections.
+  bool CFIProtection;
----------------
instrument CFI protection?


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:104
+  static void
+  buildFlowGraphImpl(const FileAnalysis &Analysis, ControlFlowNode *Node,
+                     std::vector<ConditionalBranchNode *> *BranchNodes,
----------------
Would be nice to have a comment here that briefly explains the approach (e.g. there are some bits that build the graph backwards and some that build it forwards)


https://reviews.llvm.org/D38427