C++: Remove infeasible edges to reachable blocks

[dave-bartolomeo]

The existing unreachable IR removal code only retargeted an infeasible edge to an Unreached instruction if the successor of the edge was an unreachable block. This is too conservative, because it doesn't remove an infeasible edge that targets a block that is still reachable via other paths. The trivial example of this is do { } while (false);, where the back edge is infeasible, but the body block is still reachable from the loop entry.

This change retargets all infeasible edges to Unreached instructions, regardless of the reachability of the successor block.

[pavgust]

This pull request introduces 126 alerts when merging 56bb9dc into b8877f1 - view on LGTM.com

new alerts:

• 25 for Syntax error
• 14 for Unused import
• 12 for Variable defined multiple times
• 9 for First argument of a method is not named 'self'
• 8 for Statement has no effect
• 4 for Empty except
• 4 for Unused local variable
• 2 for Use of the return value of a procedure
• 2 for Unused named argument in formatting call
• 2 for Illegal raise
• 2 for Explicit returns mixed with implicit (fall through) returns
• 2 for Implicit string concatenation in a list
• 2 for Request without certificate validation
• 1 for Too few arguments in formatting call
• 1 for Unmatchable dollar in regular expression
• 1 for Unmatchable caret in regular expression
• 1 for Wrong name for an argument in a call
• 1 for Formatted object is not a mapping
• 1 for Mismatch in multiple assignment
• 1 for Commented out code
• 1 for Unsupported format character
• 1 for Duplication in regular expression character class
• 1 for Comparison using is when operands support __eq__
• 1 for Missing named arguments in formatting call
• 1 for Non-callable called
• 1 for Special method has incorrect signature
• 1 for Maybe missing 'self' in comparison
• 1 for Iterable can be either a string or a sequence
• 1 for Except block handles 'BaseException'
• 1 for Formatting string mixes implicitly and explicitly numbered fields
• 1 for Missing call to __init__ during object initialization
• 1 for Unnecessary 'else' clause in loop
• 1 for First parameter of a class method is not named 'cls'
• 1 for __init__ method is a generator
• 1 for Missing part of special group in regular expression
• 1 for Unused argument in a formatting call
• 1 for Comparison of identical values
• 1 for Unreachable code
• 1 for Explicit export is not defined
• 1 for Wrong number of arguments for format
• 1 for Backspace escape in regular expression
• 1 for __del__ is called explicitly
• 1 for Module is imported with 'import' and 'import from'
• 1 for Redundant assignment
• 1 for Importing value of mutable attribute
• 1 for __init__ method returns a value
• 1 for Nested loops with same variable
• 1 for Hard-coded credentials
• 1 for Nested loops with same variable reused after inner loop body
• 1 for Returning tuples with varying lengths
• 1 for Unnecessary delete statement in function

---

Comment posted by LGTM.com

[jbj]

Otherwise LGTM

[jbj]

This is a dangerous API. It's easy to accidentally call getASuccessor instead of getAFeasibleSuccessor. How much of a hassle would it be to let ReachableBlock extend TIRBlock instead of IRBlock so it doesn't inherit getASuccessor? If it's not convenient to expose TIRBlock, an alternative is to make an IRBlockBase class like I've done with ControlFlowNodeBase.

[dave-bartolomeo]

Good suggestion. I've factored out IRBlockBase, and used it as the base type for both IRBlock and ReachableBlock.

[jbj]

Why not have just a single Unreached instruction, shared by the whole function?

[dave-bartolomeo]

I considered that, but eventually went with one Unreached per edge to avoid introducing "critical" edges (edges whose predecessor block has multiple successors and whose successor block has multiple predecessors). Critical edges make it difficult to insert inference operators. For these edges, though, I guess we wouldn't have anything interesting to infer anyway, so I'll try folding all of the Unreached instructions into one per function.

[jbj]

The tests failed:

13:40:57 Some tests failed!
13:40:57     ql/cpp/ql/test/library-tests/controlflow/guards-ir/tests.ql: Test failure (Expected output does not match)

[dave-bartolomeo]

The test failures are in the IR Guards tests. Some of it is due to new unreachable edges that were removed, but most of the diffs are due to merging all of the Unreached instructions for a function into one. The problem is that I give the Unreached instruction the AST (and location) of the Function, since there wasn't really a single AST that makes sense. This confuses the mapping between BasicBlock and IRBlock that the IR Guards library does. I considered going back to one Unreached instruction per infeasible edge, but for infeasible edges to reachable blocks, we'd have an Unreached instruction with the AST of the still-reachable block, which causes similar problems. Instead, I've just had the IR Guards library stop reporting an unreached block as being controlled by any guards. It's not that there's much point in knowing what conditions were true in unreachable code anyway.