Java: IPA the CFG (second try) #17970
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| private ControlFlowNode mainBranchSucc(ControlFlowNode n, boolean b) { | ||
| result = succ(n, BooleanCompletion(_, b)) | ||
| } | ||
| private Node mainBranchSucc(Node n, boolean b) { result = succ(n, BooleanCompletion(_, b)) } |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
| @@ -1487,8 +1555,8 @@ | |||
| * In the latter case, when `n` occurs as the last node in a finally block, there might be | |||
| * multiple different such successors. | |||
| */ | |||
| private ControlFlowNode otherBranchSucc(ControlFlowNode n, boolean b) { | |||
| exists(ControlFlowNode main | main = mainBranchSucc(n, b.booleanNot()) | | |||
| private Node otherBranchSucc(Node n, boolean b) { | |||
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
| result = this.(Expr).getEnclosingStmt() | ||
| module ControlFlow { | ||
| private predicate hasControlFlow(Expr e) { | ||
| not e.getEnclosingStmt() instanceof ConstCase and |
There was a problem hiding this comment.
In a line like case "a" -> null; I think this is trying to exclude the "a" but is accidentally excluding the null as well. I guess this wasn't an issue until switch expressions got added.
There was a problem hiding this comment.
Or maybe it's the arrow syntax that caused the problem, as extra expressions are now part of this statement.
|
The language test failure can only be fixed with an internal PR to update a .expected file. I will prepare one when this PR has been approved. |
|
DCA has finished and it shows a mild analysis time increase (1.6% median, 1.8% mean). I ran some queries on apache/flink using before and after commits and looked at the tuple counts and predicate timings and didn't see anything obviously taking much longer. I don't think it's a bad join order or anything like that. But I guess that adding another layer will make analysis take a little longer. |
| * | ||
| * This is needed for the equivalence relation on basic blocks in range | ||
| * analysis. |
There was a problem hiding this comment.
That's not its only use. Also, it seems reasonable to have predicate like this without such a specific-sounding constraint.
| * | |
| * This is needed for the equivalence relation on basic blocks in range | |
| * analysis. |
There was a problem hiding this comment.
Did you mean to suggest a different wording?
There was a problem hiding this comment.
Ah no, I meant to simply delete it. Edited.
There was a problem hiding this comment.
Would you expect every control flow node to be able to supply this? Or is it optional?
There was a problem hiding this comment.
Hmm, there are two different potentially relevant predicates, one that provides the inverse of the getControlFlowNode on Expr and Stmt, and one that projects every Cfg node to an associated Ast node. I think I'd name the latter something with project, and let this predicate take the semantics of the former. Hence, I'd say that this is optional and that we should postfix its qldoc with the customary "if any".
Edit: Which I see it already has. LGTM then.
|
@aschackmull This is ready to re-review. I have accepted all your review suggestions. The failing test is because we need an internal PR as well. |
owen-mc
added
the
depends on internal PR
Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
owen-mc
force-pushed
the
java/lightweight-IR-layer
branch
from
0df1427
to
42104fd
Compare
This converts the control flow graph to an IPA type to give us some more options in how we model control flow. There should be no change in any query results.
This resurrects this PR from almost six years ago. The CFG code hasn't changed much in the interim, thankfully.
The commit history is pretty garbled, so it's probably best not to review commit-by-commit.