Page MenuHomePhabricator

[clang][dataflow] Handle multiple context-sensitive calls to the same function
ClosedPublic

Authored by samestep on Jul 28 2022, 12:27 PM.

Details

Summary

This patch enables context-sensitive analysis of multiple different calls to the same function (see the ContextSensitiveSetBothTrueAndFalse example in the TransferTest suite) by replacing the Environment copy-assignment with a call to the new popCall method, which std::moves some fields but specifically does not move DeclToLoc and ExprToLoc from the callee back to the caller.

To enable this, the StorageLocation for a given parameter needs to be stable across different calls to the same function, so this patch also improves the modeling of parameter initialization, using ReferenceValue when necessary (for arguments passed by reference).

This approach explicitly does not work for recursive calls, because we currently only plan to use this context-sensitive machinery to support specialized analysis models we write, not analysis of arbitrary callees.

Diff Detail

Event Timeline

samestep created this revision.Jul 28 2022, 12:27 PM
Herald added a project: Restricted Project. · View Herald Transcript
samestep requested review of this revision.Jul 28 2022, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2022, 12:27 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
samestep edited the summary of this revision. (Show Details)Jul 28 2022, 12:42 PM

Looks great!

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
225

maybe mention in the patch description that it also improves the modeling of parameter initialization?

234

Add an else that calls createValue on the decl's type, and then sets it, like VisitDeclStmt?

247

Maybe add something like "Conceptually, these maps capture information from the local scope, so when popping that scope, we do not propagate the maps".

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

Are there more scenarios testable at this point? e.g

  1. 2 layers of callees
  2. more than one line of code inside the body?
  3. one than one CFG block in the body?

If so, please add tests for those that are supported.

samestep edited the summary of this revision. (Show Details)Jul 28 2022, 2:11 PM
samestep added inline comments.
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
225

Good point; done.

234

Ah yes, I meant to do this before but forgot; will do that now.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
3963
  1. This currently isn't supported.
  2. This should work.
  3. This should work.

For (2) and (3), should I add those tests now, or do that in a followup patch?

samestep updated this revision to Diff 448438.Jul 28 2022, 2:19 PM

Address some of Yitzie's comments

ymandel added inline comments.Jul 28 2022, 2:44 PM
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
3963
  1. This currently isn't supported.

What's limiting us here? Just curious, not asking you to update the comments, etc.

  1. This should work.
  2. This should work.

For (2) and (3), should I add those tests now, or do that in a followup patch?

may as well add them now. thx

3963
  1. This currently isn't supported.
  2. This should work.
  3. This should work.

For (2) and (3), should I add those tests now, or do that in a followup patch?

samestep added inline comments.Jul 29 2022, 6:32 AM
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
3963

What's limiting us here? Just curious, not asking you to update the comments, etc.

Nothing's really limiting us; all I meant was that currently we don't turn on context-sensitivity for the sub-analysis when we analyze the callee, and I don't want to turn that on until I've added support both for limiting the depth and for selecting the set of symbols to analyze using an allowlist.

xazax.hun accepted this revision.EditedJul 29 2022, 9:54 AM

I'd love to see some comments in the source code to make sure one will not use these facilities for other purposes that would not work. Alternatively, ContextSensitive option could be renamed to NonRecursiveContextSensitive.

clang/lib/Analysis/FlowSensitive/Transfer.cpp
512–513

Since we do not support recursion (and only do inlining with the depth of one), I think we should have a check here to avoid a inlining recursive call.

This revision is now accepted and ready to land.Jul 29 2022, 9:54 AM
samestep updated this revision to Diff 448698.Jul 29 2022, 12:19 PM

Add more comments and tests

xazax.hun accepted this revision.Jul 29 2022, 12:35 PM
ymandel accepted this revision.Jul 29 2022, 12:37 PM

Nice work!

This revision was landed with ongoing or failed builds.Jul 29 2022, 12:40 PM
This revision was automatically updated to reflect the committed changes.
sgatev added inline comments.Aug 3 2022, 10:50 PM
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
250

Does std::move help here? CalleeEnv is a const reference.