C++: IR return indirections for this
#3587
Conversation
Bring in fix for duplicate virtual variables for parameter indirections
| result = TIndirectReturnKind(-1) and | ||
| primary.isThisIndirection() | ||
| or | ||
| result = TIndirectReturnKind(primary.getParameter().getIndex()) |
There was a problem hiding this comment.
This is an example of how this gets second-class treatment compared to other parameters and still needs to be special-cased. The synthetic varargs parameter is even easier to forget.
How can we make it easier to treat synthetic and non-synthetic parameters uniformly, without having to enumerate all types of synthetic parameters every time? Create an IRParameter class? Create a ReturnIndirectionInstruction.getParameterIndex() predicate and rename getParameter to getASTParameter so it's clearer that it may not always return a value?
Is one of these things easy enough to be done as part of this PR?
There was a problem hiding this comment.
I think we'd want to do both of those, and make the same getASTParameter change to the initialization instructions and argument operands. That's enough that I'd rather merge this as-is and do a sweeping parameter change separately.
|
Yes, semantic merge conflicts. All the merge changes are positive, with the exception of a few conversions without locations being treated as sinks. I've removed those sinks in a separate commit. |
|
I've started https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1181/, hoping that our Jenkins workers are in better shape this morning than they were last night. The test changes look great! I hope the CPP-Differences will live up to that. |
|
The new |
The IR false positives are due to the same path length limit as the AST false positives on the same line.
|
LGTM, but there's a merge conflict. |
Accept test changes - dataflow changes are all positive
There was unfortunately a semantic merge conflict between github#3419 and github#3587 that caused a performance regression on (at least) OpenJDK. This reverts commit 982fb38, reversing changes made to b841cac.
No description provided.