Skip to content
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.

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

Shared: add in/out barriers with flow state #14305

Merged
merged 7 commits into from Sep 28, 2023
Merged

Shared: add in/out barriers with flow state #14305

merged 7 commits into from Sep 28, 2023

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Sep 25, 2023 •
edited

Adds overloads of isBarrierIn and isBarrierOut that take a FlowState.

predicate isBarrierIn(Node node, FlowState state)
predicate isBarrierOut(Node node, FlowState state)

The most immediate use-case for these is to be able to implement these as calls to isSource or isSink, without losing the flow state:

predicate isBarrierIn(Node node, FlowState state) { isSource(node, state) }
predicate isBarrierOut(Node node, FlowState state) { isSink(node, state) }

This is needed for the JS data flow migration. Since this isn't merged yet, I haven't added the use sites in this PR, though I have verified that it works in the JS queries where I have used it.

To get a bit more validation, I also ran a C++ experiment with this commit resulting in this evaluation which seems to look fine.

@asgerf asgerf force-pushed the shared/flow-state-inout-barriers branch from c23e88c to d501856 Compare September 25, 2023 08:05
@asgerf asgerf marked this pull request as ready for review September 26, 2023 13:14
@asgerf asgerf requested review from a team as code owners September 26, 2023 13:14
hvitved
hvitved requested changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll Outdated Show resolved Hide resolved
shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll Outdated Show resolved Hide resolved
hvitved
hvitved previously approved these changes Sep 27, 2023
View reviewed changes
aschackmull
aschackmull requested changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

This only checks the new barriers on the state-transitioning steps. It also needs to be checked for a lot of other case (state-preserving local steps, jump steps, read steps, store steps, and call edges).

@asgerf
Copy link
Contributor Author

asgerf commented Sep 27, 2023

Thanks for catching this @aschackmull.

As discussed offline, I've put check at the PathNode level since these barriers are unlikely to affect pruning and so aren't that important to check in MkStage. However, the checks weren't quite as simple as initially discussed because subpaths and PartialPathNode needed to be updated as well.

aschackmull
aschackmull reviewed Sep 27, 2023
View reviewed changes
shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll
@@ -3888,6 +3903,7 @@ module MakeImpl<InputSig Lang> {
) {
exists(ArgNodeEx arg, ArgumentPosition apos |
pathNode(mid, arg, state, cc, _, t, ap, _) and
not outBarrier(arg, state) and
Copy link
Contributor

Choose a reason for hiding this comment

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

s/out/in/, right? no never mind. outBarrier is right.

aschackmull
aschackmull approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

LGTM

@asgerf
Copy link
Contributor Author

asgerf commented Sep 28, 2023

DCA runs look peaceful so I'll go ahead and merge

@asgerf asgerf merged commit 0d96ed8 into github:main Sep 28, 2023
53 checks passed
@asgerf asgerf mentioned this pull request Sep 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschackmull aschackmull aschackmull approved these changes

@hvitved hvitved hvitved left review comments

Assignees
No one assigned
Labels
C# C++ DataFlow Library documentation Go Java Python Ruby Swift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@asgerf @hvitved @aschackmull