Skip to content

C++: instruction -> operand field flow#4573

Merged
dbartol merged 6 commits into
github:mainfrom
MathiasVP:interleave-op-instr-field-flow
Nov 12, 2020
Merged

C++: instruction -> operand field flow#4573
dbartol merged 6 commits into
github:mainfrom
MathiasVP:interleave-op-instr-field-flow

Conversation

@MathiasVP

@MathiasVP MathiasVP commented Oct 29, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR changes readStep so that it performs instruction -> operand flow, and storeStep so that it performs operand -> instruction flow. This matches better with the way we do simple flow steps.

CPP-differences: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1527/

@MathiasVP
C++: Store steps now go from operands to instructions, and read steps…
43a231f 
… now go from instructions and operands. There are a couple of read steps that still target instructions because I couldn't decide on an operand to target.
@MathiasVP
C++: Accept tests
835d3fc
@MathiasVP MathiasVP added the C++ label Oct 29, 2020
@MathiasVP MathiasVP requested a review from a team as a code owner October 29, 2020 08:51
@MathiasVP

MathiasVP commented Oct 29, 2020

Copy link
Copy Markdown
Contributor Author

The failing tests all look like changes in path explanation in the internal repo. Once the changes here have been approved I'll make an internal PR.

dbartol
dbartol suggested changes Oct 29, 2020
View reviewed changes

@dbartol dbartol left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions and a few comments.

Would you mind waiting to merge this until the temporary object PR is merged? I don't think we have any merge conflicts quite yet, but we'll probably conflict on those path explanation tests in the private repo, and your PR may provoke real analysis change in mine.

Comment thread cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
Comment thread cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
Comment thread cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll Outdated
Comment thread cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll Outdated
@MathiasVP

MathiasVP commented Oct 29, 2020

Copy link
Copy Markdown
Contributor Author

Would you mind waiting to merge this until the temporary object PR is merged? I don't think we have any merge conflicts quite yet, but we'll probably conflict on those path explanation tests in the private repo, and your PR may provoke real analysis change in mine.

That's a good point. I'll let this PR wait until your temporary object PR is merged.

@MathiasVP

MathiasVP commented Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

CPP-differences: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1527/

Performance looks alright. The only worry is a 4% analysis-time increase on boost, but it's not something I can reproduce locally, and there's nothing suspicious in the evaluation log.

@MathiasVP MathiasVP force-pushed the interleave-op-instr-field-flow branch from 3c77886 to 177f943 Compare October 30, 2020 14:59
@MathiasVP MathiasVP added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Nov 9, 2020
@MathiasVP

MathiasVP commented Nov 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Now that #4432 has been merged I think this PR is good to go. I've created an internal PR for the test changes that are making Actions to complain.
(I believe all of your comments have been addressed @dbartol.)

@dbartol dbartol self-assigned this Nov 12, 2020
dbartol
dbartol approved these changes Nov 12, 2020
View reviewed changes

@dbartol dbartol left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes. LGTM.

@dbartol dbartol merged commit f43d911 into github:main Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@rdmarsh2 rdmarsh2 rdmarsh2 left review comments
@dbartol dbartol dbartol approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@dbartol dbartol

Labels

C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MathiasVP @rdmarsh2 @dbartol