C++: Refactor some side effect generation code #5643
Merged
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
This change was necessary for my upcoming changes to introduce side effect instructions for indirections of smart pointers. The code to decide which parameters have which side effects appeared in both the IPA constructor for `TTranslatedSideEffect` and in `TranslatedCall`. These two versions didn't quite agree, especially once the `SideEffectFunction` model provides its own side effects instead of the defaults. The relevant code has now been factored out into `SideEffects.qll`. This queries the model if one exists, and provides default side effects if no model exists. This fixes at least one existing issue, where we were emitting a buffer read side effect for `*this` instead of an indirect read side effect. This accounts for all of the IR diffs in the tests.
rdmarsh2
reviewed
Apr 9, 2021
Contributor
rdmarsh2
left a comment
rdmarsh2
left a comment
There was a problem hiding this comment.
IR generation changes look good to me, with a couple nitpicks. Once the field flow tests are fixed, those changes should probably be reviewed by @MathiasVP
cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll
Show resolved
Hide resolved
cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll
Outdated
Show resolved
Hide resolved
added 2 commits
April 12, 2021 18:11
We have special code to handle field flow for single-field structs, but that special case was too specific. Some `Store`s to single-field structs have no `Chi` instruction, which is the case that we handled already. However, it is possible for the `Store` to have a `Chi` instruction (e.g. for `{AllAliased}`), but still have a use of the result of the `Store` directly. We now add a `PostUpdateNode` for the result of the `Store` itself in those cases, just like we already did if the `Store` had no `Chi`.
Author
|
@MathiasVP I've added another commit to fix the broken dataflow tests. Hopefully the comment in the commit and in the code describes the change well enough. |
Contributor
Commit 697b2dc LGTM (except for formatting). |
Contributor
|
LGTM once formatting is fixed. |
rdmarsh2
approved these changes
Apr 14, 2021
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 change was necessary for my upcoming changes to introduce side effect instructions for indirections of smart pointers. The code to decide which parameters have which side effects appeared in both the IPA constructor for
TTranslatedSideEffectand inTranslatedCall. These two versions didn't quite agree, especially once theSideEffectFunctionmodel provides its own side effects instead of the defaults.The relevant code has now been factored out into
SideEffects.qll. This queries the model if one exists, and provides default side effects if no model exists. This fixes at least one existing issue, where we were emitting a buffer read side effect for*thisinstead of an indirect read side effect. This accounts for all of the IR diffs in the tests.