C++: Move the LoadInstruction from ++ to e in e++.
#2686
Merged
+288
−211
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, the `Load` would be associated with the `CrementOperation` rather than its operand, which gave surprising results when mapping taint sinks back to `Expr`. The changes in `raw_ir.expected` are to add `Copy` operations on the `x++` in code like `y = x++`. This is now needed because the result that `x++` would otherwise have (the Load) no longer belongs to the `++` expression. Copies are inserted to ensure that all expressions are associated with an `Instruction` result. The changes in `*aliased_ssa_ir.expected` appear to be just wobble.
This commit undoes the code sharing between `TranslatedAssignExpr` (`=`) and `TranslatedAssignOperation` (`+=`, `<<=`, ...). In the next commit, when we change how the `Load` works on the LHS of `TranslatedAssignOperation`, these classes will become so different that sharing is no longer helpful.
This is analogous to what was done for `CrementOperation`.
jbj
added a commit
to jbj/ql
that referenced
this pull request
Jan 27, 2020
The issue for that test is being tested and fixed on PR github#2686. Adding a test here will cause a semantic merge conflict.
Update test output to fix semantic merge conflict.
Conflicts: cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll
dbartol
previously approved these changes
Feb 5, 2020
dbartol
left a comment
dbartol
left a comment
There was a problem hiding this comment.
LGTM
I think this was a really clean way of handling these constructs.
Conflicts: cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir.expected cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir_unsound.expected
Conflicts: cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir.expected cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir_unsound.expected cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ir.expected cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ir_unsound.expected
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the structure of the IR translation in order to improve the mapping between
ExprandInstruction. Previously, the result instruction ofeine++was the address corresponding to the lvalue ofe. With this PR, the result instruction ofeis instead the instruction that loads frome. We use the result instruction to map results from IR-based analyses back to their AST elements. I expect this PR to have the following effects on such libraries:x = source; x++;, there will now be flow to thexinx++. This fixes the problem observed by @MathiasVP onArithmeticWithExtremeValues.ql. That query has at least one more problem, which I'll address in a separate PR. The fix can be observed onflow0.expected.Expr, then inx = 0; x++;, thexinx++should now be considered equal to 0. Previously it would instead be considered equal to the stack address of thexvariable. The same effect can be observed in this PR onSignAnalysis.expected.f(x); f(x++);, the twoVariableAccesses toxshould now get the same value number. Previously, the value number of the second load fromxwas not the result of anyExpr, so thexinx++would instead share its value number with any occurrences of&x.I think it can be argued that it's technically correct to associate the
eine++with the lvalue (address) oferather than the loaded value. That's how it'll still be for overloadede++, but a crucial difference is that an overloadede++has an associated read-side-effect instruction on which the value should appear. There was and is no such thing for the non-overloadede++.Everything I wrote above about
e++also applies to--e,e <<= ..., etc.I recommend reading each commit individually.