Skip to content

C++: Refactor ProductFlow to have a DataFlow::ConfigSig-like interface #12615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jketema merged 7 commits into from
Apr 6, 2023
Merged

C++: Refactor ProductFlow to have a DataFlow::ConfigSig-like interface #12615

jketema merged 7 commits into from
Apr 6, 2023

Conversation

@jketema
Copy link
Contributor

@jketema jketema commented Mar 21, 2023

No description provided.

@github-actions github-actions bot added the C++ label Mar 21, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 21, 2023
View reviewed changes
cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll
module Internal {
class Conf1 extends DataFlow::Configuration {
Conf1() { this = "Conf1" }
private module Config1 implements DataFlow::StateConfigSig {

Check warning

Code scanning / CodeQL

Data flow configuration module naming

Modules implementing a data flow configuration should end in `Config`.
cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll
exists(Configuration conf, DataFlow::PathNode source1 |
conf.isSourcePair(source1.getNode(), source1.getState(), source, state) and
any(Conf1 c).hasFlowPath(source1, _)
module Config2 implements DataFlow::StateConfigSig {

Check warning

Code scanning / CodeQL

Data flow configuration module naming

Modules implementing a data flow configuration should end in `Config`.
MathiasVP
MathiasVP previously approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@jketema
Copy link
Contributor Author

jketema commented Mar 21, 2023

Should I run DCA on this with memory-corruption-queries.qls?

@MathiasVP
Copy link
Contributor

MathiasVP commented Mar 21, 2023

Should I run DCA on this with memory-corruption-queries.qls?

Yes, please. I haven't used that suite in this quarter, but I assume it still works 🤞

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 22, 2023
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

MathiasVP
MathiasVP previously approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM! I especially like that last commit 😍.

@jketema
Copy link
Contributor Author

jketema commented Mar 22, 2023

I did see a slow-down on the first DCA experiment I ran. Re-running now to see if that still occurs with the latest commit.

@jketema jketema force-pushed the product-configsig branch from b2ecbca to 9c2e702 Compare March 24, 2023 08:16
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 24, 2023
View reviewed changes
cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll
Config::isBarrier2(node)
}

predicate isBarrier1 = Config::isBarrier1/1;

Check warning

Code scanning / CodeQL

Dead code

This code is never used, and it's not publicly exported.
cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll

predicate isBarrier1 = Config::isBarrier1/1;

predicate isBarrier2 = Config::isBarrier2/1;

Check warning

Code scanning / CodeQL

Dead code

This code is never used, and it's not publicly exported.
cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll

predicate isBarrierOut2 = Config::isBarrierOut2/1;

predicate isAdditionalFlowStep1 = Config::isAdditionalFlowStep1/2;

Check warning

Code scanning / CodeQL

Dead code

This code is never used, and it's not publicly exported.
cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll
Config::isAdditionalFlowStep1(node1, node2)
}

predicate isAdditionalFlowStep2 = Config::isAdditionalFlowStep2/2;

Check warning

Code scanning / CodeQL

Dead code

This code is never used, and it's not publicly exported.
MathiasVP
MathiasVP previously approved these changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM. I included a small suggestion, but since this is already a major performance boost compared to the prior DCA run I think it's fine to merge this as-is if DCA comes back happy 🎉.

@jketema jketema force-pushed the product-configsig branch from bc9bb5a to 12702b5 Compare April 6, 2023 15:13
@jketema
Copy link
Contributor Author

jketema commented Apr 6, 2023

DCA looks somewhat better after the join-order fixes and a rebase. Still maybe a bit too much slowdown?

@MathiasVP
Copy link
Contributor

MathiasVP commented Apr 6, 2023

DCA looks somewhat better after the join-order fixes and a rebase. Still maybe a bit too much slowdown?

Fantastic! There may be a very small slowdown, but I don't think this is something that should block the merge.

@jketema jketema marked this pull request as ready for review April 6, 2023 17:41
@jketema jketema requested a review from a team as a code owner April 6, 2023 17:41
@jketema jketema added the no-change-note-required This PR does not need a change note label Apr 6, 2023
MathiasVP
MathiasVP approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@jketema jketema merged commit 5ee9711 into github:main Apr 6, 2023
@jketema jketema deleted the product-configsig branch April 6, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MathiasVP MathiasVP MathiasVP approved these changes

Assignees

No one assigned

Labels

C++ no-change-note-required This PR does not need a change note

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jketema @MathiasVP