Skip to content

C++: Flow through uncertain writes#11658

Merged
MathiasVP merged 4 commits intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
MathiasVP:uncertain-writes
Dec 14, 2022
Merged

C++: Flow through uncertain writes#11658
MathiasVP merged 4 commits intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
MathiasVP:uncertain-writes

Conversation

@MathiasVP
Copy link
Copy Markdown
Contributor

Previously, we weren't getting flow in the following example on the use-use feature branch:

char xs[2];
xs[0] = source();
xs[1] = 0;
sink(xs[0]);

because the assignment to xs[1] was seen as overwriting the contents of xs, and thus the assignment xs[0] = source(); was dead according to SSA.

This PR fixes this by recognizing that some writes are "uncertain" (i.e., we don't know if they completely overwrite a piece of memory), and ensures that we have flow from uncertain definition's prior definition to the variable being defined.

I was actually planning on putting up this PR after #11626 had been merged, but seeing as there are some performance issues with that PR I might as well put this on up now (as this PR is largely unrelated to iterators).

@github-actions github-actions Bot added the C++ label Dec 12, 2022
…ces specify that the remote source is both 'isReturnValue' and 'isReturnValueDeref'.
@MathiasVP MathiasVP marked this pull request as ready for review December 12, 2022 13:40
@MathiasVP MathiasVP requested a review from a team as a code owner December 12, 2022 13:40
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Dec 12, 2022
@aschackmull
Copy link
Copy Markdown
Contributor

This seems like a weird use of SSA to me. I'd expect the char xs[2] to be the only write to the xs pointer, and then the subsequent mentions of xs to be reads (the pointer isn't modified). The writes that occur along the way are then store steps in data flow, so nothing is marked as dead by SSA, and there should be no need for uncertain SSA writes.

@MathiasVP
Copy link
Copy Markdown
Contributor Author

MathiasVP commented Dec 13, 2022

This seems like a weird use of SSA to me. I'd expect the char xs[2] to be the only write to the xs pointer, and then the subsequent mentions of xs to be reads (the pointer isn't modified). The writes that occur along the way are then store steps in data flow, so nothing is marked as dead by SSA, and there should be no need for uncertain SSA writes.

But we don't model writes to x[1] as a store step. We model them as an assignment to an allocation which is tracked by SSA. So the situation is basically:

char xs[2]; // xs_allocation = uninitialized
xs[0] = source(); // xs_allocation = source();
xs[1] = 0; // xs_allocation = 0;
sink(xs[0]); // sink(xs_allocation);

@MathiasVP MathiasVP requested a review from rdmarsh2 December 13, 2022 16:09
predicate ssaFlow(Node nodeFrom, Node nodeTo) {
exists(Node nFrom, boolean uncertain |
ssaFlowImpl(nFrom, nodeTo, uncertain) and
if uncertain = true then nodeFrom = [nFrom, getAPriorDefinition(nFrom)] else nodeFrom = nFrom
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to check - this is the only place where we're making a decision based on whether the write is certain, and the rest of the changes are just pushing certainty information through?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, exactly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or rather: We make the decision on whether a write is certain in isDefImpl. And we then consume this information in this predicate.

@MathiasVP MathiasVP merged commit 22b04af into github:mathiasvp/replace-ast-with-ir-use-usedataflow Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants