Implement a localised graph builder for indirect control flow instructions. Main interface is through GraphBuilder::buildFlowGraph, which will build a flow graph around an indirect CF instruction. Various modifications to FileVerifier are also made to const-expose some members needed for machine code analysis done by the graph builder.
Details
Diff Detail
- Build Status
Buildable 10703 Build 10703: arc lint + arc unit
Event Timeline
I'm having difficulty following this because you document what the GraphBuilder methods do but you never say how and I get lost before I figure that out. Maybe that could go in the GraphBuilder.cpp file header comments?
tools/llvm-cfi-verify/GraphBuilder.cpp | ||
---|---|---|
2 | Missing header | |
tools/llvm-cfi-verify/GraphBuilder.h | ||
50 | Do these need to be externally visible? | |
68 | Should be a method of ConditionalBranchNode (also, prefer to accept a raw_ostream& instead of hardcoding outs() for print methods, dump() typically writes to dbgs() I think) | |
69 | ditto | |
77 | Why are these pointers? |
tools/llvm-cfi-verify/GraphBuilder.h | ||
---|---|---|
50 | They're modified by the tests in order to guarantee behaviour, even if the default change. | |
77 | 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. |
tools/llvm-cfi-verify/GraphBuilder.cpp | ||
---|---|---|
233 | Reword this. | |
294 | 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.) | |
311 | Reword 'fall back on itself' | |
314 | Replace with std::find_if() ? | |
327 | Why not use createParentNode() here? | |
336 | Place the error first, and also avoid the nesting here as well. | |
tools/llvm-cfi-verify/GraphBuilder.h | ||
54 | just print( | |
55 | just flatten( | |
63 | just print( | |
67 | instrument CFI protection? | |
77 | Would it make sense to just place ConditionalBranchNodes in a vector without dynamically allocating them so they don't have to be manually freed? | |
105 | 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) |
tools/llvm-cfi-verify/GraphBuilder.h | ||
---|---|---|
76 | Could we represent this as (1) a uint64_t SrcAddr -> uint64_t DstAddr mapping and (2) a std::vector<uint64_t> of orphaned nodes? Then you would not need the Next pointers, and presumably FlowLength can simply be maintained as an argument to buildFlowGraphImpl(). You could maintain O(1) lookup by using llvm::DenseMap which is hash-table based and lighter-weight than std::map (see http://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h) and that seems cleaner to me than the current structure, WDYT? |
tools/llvm-cfi-verify/lib/GraphBuilder.cpp | ||
---|---|---|
96 ↗ | (On Diff #118988) | Why not just pass this in? |
108 ↗ | (On Diff #118988) | s/chain/basic block/? |
152 ↗ | (On Diff #118988) | ditto s/chain/basic block/? |
154 ↗ | (On Diff #118988) | ditto |
176 ↗ | (On Diff #118988) | ditto |
196 ↗ | (On Diff #118988) | nit: eliminate braces |
227 ↗ | (On Diff #118988) | 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? |
tools/llvm-cfi-verify/lib/GraphBuilder.h | ||
57 ↗ | (On Diff #118988) | Could this be reworded? I'm still not sure what it means. |
86 ↗ | (On Diff #118988) | Sentence cuts off |
94 ↗ | (On Diff #118988) | fully evaluated, or enumerated? Not sure what evaluated might refer to in this context. |
108 ↗ | (On Diff #118988) | s/acyclic/cycle/ |
tools/llvm-cfi-verify/lib/GraphBuilder.h | ||
---|---|---|
62 ↗ | (On Diff #118988) | type: posible |
68 ↗ | (On Diff #118988) | It's not all keys, just keys for all valid instructions in that range right? |
unittests/tools/llvm-cfi-verify/GraphBuilder.cpp | ||
60 ↗ | (On Diff #118988) | Why template it just for std::vector? |
131 ↗ | (On Diff #118988) | This is never reached since an unhandled Error (e.g. at destruction time) cause an immediate failure. |
453 ↗ | (On Diff #118988) | ??? +----------+ +--------------+ | 20 | <-- | 0 | +----------+ +--------------+ | | | | v v +----------+ +--------------+ | 21 | | 2 | +----------+ +--------------+ | | | | v v +----------+ +--------------+ | 22 (ud2) | +> | 7 | +----------+ | +--------------+ ^ | | | | | | | v +----------+ | +--------------+ | 4 | | | 8 | +----------+ | +--------------+ | | | | | | v | v +----------+ | +--------------+ +------------+ | 6 | -+ | 9 (indirect) | <-- | 13 | +----------+ +--------------+ +------------+ ^ | | | | v +--------------+ +------------+ | 11 | | 15 (error) | +--------------+ +------------+ |
Updated with Vlad's comments. Also patched the unit tests to succeed when x86 targets are not built, instead of not compiling.
tools/llvm-cfi-verify/lib/GraphBuilder.cpp | ||
---|---|---|
108 ↗ | (On Diff #118988) | I've changed them all /s/chain/block. These blocks may contain direct unconditional branches as well :) |
227 ↗ | (On Diff #118988) | 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. |
tools/llvm-cfi-verify/lib/GraphBuilder.h | ||
68 ↗ | (On Diff #118988) | 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. |
unittests/tools/llvm-cfi-verify/GraphBuilder.cpp | ||
60 ↗ | (On Diff #118988) | Was initially using it to print IntermediateNodes (type: DenseMap) as well, now it's just printing vector's I'll remove the templating. Thanks! |
453 ↗ | (On Diff #118988) | It's beautiful! |
unittests/tools/llvm-cfi-verify/FileAnalysis.cpp | ||
---|---|---|
68 ↗ | (On Diff #119725) | Could we (1) break the x86 test changes out into a follow-up CL and (2) check the error message to make sure we only skip tests for the specific error message we expect to see? I want to make sure we don't break something else in the future that accidentally disables unit tests without us realizing. |
This patch causes the same unittest compilation failure which was fixed in https://reviews.llvm.org/D39020.
Please fix it asap or revert the patch.
llvm/trunk/unittests/tools/llvm-cfi-verify/CMakeLists.txt | ||
---|---|---|
17 ↗ | (On Diff #119928) | what is the reason you removed this line? |
Actually I took a liberty to submit https://reviews.llvm.org/rL316422 to unblock myself. Feel free to re-write it as you need but please ensure that it does not restore the issue.
Do these need to be externally visible?