[PATCH] D38427: Graph builder implementation.

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
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) {
----------------
vlad.tsyrklevich wrote:
> s/chain/basic block/?
I've changed them all /s/chain/block. These blocks may contain direct unconditional branches as well :)


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.cpp:227
+                         Depth + 1);
+      Result.IntermediateNodes[ParentMeta.VMAddress] = Address;
+      HasValidCrossRef = true;
----------------
vlad.tsyrklevich wrote:
> 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?
Yep, don't want to add a conditional branch to `IntermediateNodes`. I can't really see a good way to not repeat this LOC without introcuding a status variable which results in the same reuse.


================
Comment at: tools/llvm-cfi-verify/lib/GraphBuilder.h:68
+  // Map between an instruction address, and the address of the next instruction
+  // that will be executed. This map will contain all keys in the range:
+  //   - [orphaned node, base address)
----------------
vlad.tsyrklevich wrote:
> It's not all keys, just keys for all valid instructions in that range right?
I think it's all keys, each key-value pair is built from the cross-references to the value. Invalid instructions (e.g. an indirect branch) will be an entry in `OrphanedNodes`, with a key-value pair between the invalid instruction -> its child.


================
Comment at: unittests/tools/llvm-cfi-verify/GraphBuilder.cpp:60
+// Printing helpers for gtest.
+template <typename Container>
+std::string HexStringifyContainer(const Container &C) {
----------------
vlad.tsyrklevich wrote:
> Why template it just for std::vector?
Was initially using it to print `IntermediateNodes` (type: DenseMap) as well, now it's just printing vector's I'll remove the templating. Thanks!


================
Comment at: unittests/tools/llvm-cfi-verify/GraphBuilder.cpp:453
+TEST_F(BasicGraphBuilderTest, BuildFlowGraphComplexExample) {
+  // The following code has this graph (in DOT format):
+  //   digraph G {
----------------
vlad.tsyrklevich wrote:
> ???
> ```
> +----------+     +--------------+
> |    20    | <-- |      0       |
> +----------+     +--------------+
>   |                |
>   |                |
>   v                v
> +----------+     +--------------+
> |    21    |     |      2       |
> +----------+     +--------------+
>   |                |
>   |                |
>   v                v
> +----------+     +--------------+
> | 22 (ud2) |  +> |      7       |
> +----------+  |  +--------------+
>   ^           |    |
>   |           |    |
>   |           |    v
> +----------+  |  +--------------+
> |    4     |  |  |      8       |
> +----------+  |  +--------------+
>   |           |    |
>   |           |    |
>   v           |    v
> +----------+  |  +--------------+     +------------+
> |    6     | -+  | 9 (indirect) | <-- |     13     |
> +----------+     +--------------+     +------------+
>                    ^                    |
>                    |                    |
>                    |                    v
>                  +--------------+     +------------+
>                  |      11      |     | 15 (error) |
>                  +--------------+     +------------+
> ```
It's beautiful!


https://reviews.llvm.org/D38427