[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.h:62
+// The canonical graph result structure returned by GraphBuilder. The members
+// in this structure encapsulate all posible code paths to the instruction
+// located at `BaseAddress`.
----------------
type: posible


================
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)
----------------
It's not all keys, just keys for all valid instructions in that range right?


================
Comment at: unittests/tools/llvm-cfi-verify/GraphBuilder.cpp:60
+// Printing helpers for gtest.
+template <typename Container>
+std::string HexStringifyContainer(const Container &C) {
----------------
Why template it just for std::vector?


================
Comment at: unittests/tools/llvm-cfi-verify/GraphBuilder.cpp:131
+    if (Analysis.initialiseDisassemblyMembers()) {
+      exit(EXIT_FAILURE);
+    }
----------------
This is never reached since an unhandled Error (e.g. at destruction time) cause an immediate failure.


================
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 {
----------------
???
```
+----------+     +--------------+
|    20    | <-- |      0       |
+----------+     +--------------+
  |                |
  |                |
  v                v
+----------+     +--------------+
|    21    |     |      2       |
+----------+     +--------------+
  |                |
  |                |
  v                v
+----------+     +--------------+
| 22 (ud2) |  +> |      7       |
+----------+  |  +--------------+
  ^           |    |
  |           |    |
  |           |    v
+----------+  |  +--------------+
|    4     |  |  |      8       |
+----------+  |  +--------------+
  |           |    |
  |           |    |
  v           |    v
+----------+  |  +--------------+     +------------+
|    6     | -+  | 9 (indirect) | <-- |     13     |
+----------+     +--------------+     +------------+
                   ^                    |
                   |                    |
                   |                    v
                 +--------------+     +------------+
                 |      11      |     | 15 (error) |
                 +--------------+     +------------+
```


https://reviews.llvm.org/D38427