Page MenuHomePhabricator

[clang][dataflow] Handle return statements
ClosedPublic

Authored by samestep on Jul 26 2022, 2:33 PM.

Details

Summary

This patch adds a ReturnLoc field to the Environment, serving a similar to the ThisPointeeLoc field in the DataflowAnalysisContext. It then uses that (along with a new VisitReturnStmt method in TransferVisitor) to handle non-void-returning functions in context-sensitive analysis.

Diff Detail

Event Timeline

samestep created this revision.Jul 26 2022, 2:33 PM
Herald added a project: Restricted Project. · View Herald Transcript
samestep requested review of this revision.Jul 26 2022, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2022, 2:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
samestep edited the summary of this revision. (Show Details)Jul 26 2022, 2:35 PM
gribozavr2 added inline comments.Jul 26 2022, 2:36 PM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
156
166–167
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
225–226
clang/lib/Analysis/FlowSensitive/Transfer.cpp
347

Please add a TODO about modeling NRVO.

samestep updated this revision to Diff 447906.Jul 26 2022, 6:25 PM

Address Dmitri's review

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
156

Done.

166–167

Done.

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
225–226

Done.

clang/lib/Analysis/FlowSensitive/Transfer.cpp
347

Added, thanks! I hadn't realized this would be necessary.

sgatev added inline comments.Jul 26 2022, 10:04 PM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
338–339

Let's make this a FIXME to set a storage location for the outermost context too.

samestep added inline comments.Jul 27 2022, 6:08 AM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
338–339

@sgatev I could add a FIXME for that, or I could just do it in this same patch; do you have a preference between those two options?

samestep updated this revision to Diff 448044.Jul 27 2022, 7:49 AM

Set a storage location for the outermost context too

samestep edited the summary of this revision. (Show Details)Jul 27 2022, 7:51 AM
li.zhe.hua added inline comments.Jul 27 2022, 11:50 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
160

Fix this as well? A reader shouldn't need to root around in private implementation details to understand the requirements for calling a function.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
207–210

I'm a little unclear on what "this" is. Is it this entire function, or just the call to getDirectCallee()? Can this comment be moved somewhere more appropriate, and specifically, so it is touching the code that is most relevant to it? It is currently floating in the middle of the function, and it's unclear to me why new code is being added above it vs. below it.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
3908

Why is this change to the test necessary?

samestep added inline comments.Jul 27 2022, 11:54 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
160

Could you clarify what you mean? I was simply copying the signature and docstring from setThisPointeeStorageLocation.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
207–210

Yes, this comment is not meant to be present in this patch; I am trying to update the parent patch to remove this comment, but am currently unable to do export it to Phabricator for technical reasons. I should be able to do that tomorrow.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
3908

This is mentioned in the patch description (updated earlier today); it's not necessary, but I added it to get a bit of extra coverage for some cases in the VisitReturnStmt method this patch adds.

li.zhe.hua added inline comments.Jul 27 2022, 1:35 PM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
160

You've marked return in backticks. There is no parameter named return and it is unclear what return refers to. My best guess is that this is a typo of ReturnLoc, which is a private data member. So this is a public interface with a requirement that a private data member has some property. This should instead reframe the requirement as properties from the external reader's perspective.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
3908

Please add the coverage as a separate test. Separate behaviors should be tested as separate tests. go/unit-testing-practices#behavior-testing

sgatev added inline comments.Jul 28 2022, 12:56 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
160

That was my guess initially too, but my next best guess is that return in backticks stands for the keyword/AST node. In any case, let's make it less ambiguous and let's not add requirements based on implementation details. How about: The return value must not be assigned a storage location.?

clang/lib/Analysis/FlowSensitive/Transfer.cpp
338–339

Same patch works!

samestep added inline comments.Jul 28 2022, 6:31 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
160

Ah sorry, I understand now; you simply meant that I should make the same change here that @gribozavr2 suggested I make to the other docstrings? I'll go ahead and do that (once I am able to re-export this patch again).

clang/lib/Analysis/FlowSensitive/Transfer.cpp
338–339

Cool, I have an update to this patch which does that, so I'll export it here as soon as I am able to do so.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
3908

Makes sense, I'll do that. Thanks!

samestep updated this revision to Diff 449667.Aug 3 2022, 7:58 AM

Rebase and address all comments

samestep edited the summary of this revision. (Show Details)Aug 3 2022, 8:02 AM
ymandel accepted this revision.Aug 3 2022, 8:03 AM
ymandel added inline comments.
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
156

This looks optional (since it is a pointer). If so, please comment to explain.

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
225–226

This implies a timing/initialization aspect to its state. Is this necessary? If not, can we restructure to avoid it? Alternatively, if it is valid to have it unset, please update the comment to reflect.

This revision is now accepted and ready to land.Aug 3 2022, 8:03 AM

Please ignore my previous comments from an earlier revision.

sgatev added inline comments.Aug 3 2022, 10:48 PM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
345

Let's do if (Val->getKind() == Value::Kind::Reference) return;. Otherwise the framework will be unusable in practice.

348

Let's do if (Loc == nullptr) return;

570

We use stable storage locations to ensure convergence. In that spirit, shouldn't we assign ReturnLoc's value to S's storage location instead of changing the storage location? Alternatively, we can pass S's storage location to pushCall so that it can store it as ReturnLoc.

samestep added inline comments.Aug 4 2022, 5:20 AM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
345

Makes sense, will do.

348

I don't think we want to do that, right? Shouldn't the return storage location always be set? Or is this about the "analyzing fragments rather than full functions" thing we discussed yesterday?

570

Could you clarify how this hurts convergence? My understanding is that ReturnLoc here is already stable, so this would make S's storage location stable too.

sgatev added inline comments.Aug 4 2022, 5:26 AM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
348

I think it's related. If we are going with always initializing the return storage location then I guess at some point we should be able to make Environment::getReturnStorageLocation return a reference? In that case I'm fine with keeping the assert around in the meantime.

570

If I follow correctly, ReturnLoc here is the result of Env.createStorageLocation(ReturnType) which isn't stable. Each call to createStorageLocation returns a fresh storage location.

samestep added inline comments.Aug 4 2022, 6:30 AM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
348

OK; yeah, I think the intention is that we're always initializing it. I'll leave this code as is for now, then.

570

Ah I see, you're right. Is there a way to make a stable storage location for the return? My intuition is that we can't just pass S's storage location to pushCall, because we want the storage location for the return to be the same across analysis of different callsites to the callee (similar to how we currently use the same storage location for a given parameter of the callee, regardless of how many times we analyze it). But maybe it would be fine; @ymandel do you have any thoughts on this?

samestep updated this revision to Diff 449958.Aug 4 2022, 6:43 AM

Don't assert returns to not be references

samestep updated this revision to Diff 449975.Aug 4 2022, 7:20 AM

Use CallExpr location for ReturnLoc

samestep updated this revision to Diff 449976.Aug 4 2022, 7:22 AM

Revert an unnecessary little change

sgatev accepted this revision.Aug 4 2022, 8:55 AM
sgatev added inline comments.
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
208

Let's add a FIXME to support references here.

clang/lib/Analysis/FlowSensitive/Transfer.cpp
543–545

Now there's a hidden connection - ReturnLoc gets assigned a value in Env because we implicitly use the same storage location in Env.pushCall(S). I suggest adding a comment about this.

samestep updated this revision to Diff 450058.Aug 4 2022, 10:41 AM

Add Stanislav's suggested comments

This revision was landed with ongoing or failed builds.Aug 4 2022, 10:42 AM
This revision was automatically updated to reflect the committed changes.