Page MenuHomePhabricator

[clang][dataflow] Add cache of `ControlFlowContext`s for function decls.
ClosedPublic

Authored by ymandel on Aug 2 2022, 5:55 PM.

Details

Summary

This patch modifies context-sensitive analysis of functions to use a cache,
rather than recreate the ControlFlowContext from a function decl on each
encounter. However, this is just step 1 (of N) in adding support for a
configurable map of "modeled" function decls. The map will go from the actual
function decl to the ControlFlowContext used to model it. Only functions
pre-configured in the map will be modeled in a context-sensitive way.

We start with a cache because it introduces the desired map, while retaining the
current behavior. Here, functions are mapped to their actual implementations
(when available).

Diff Detail

Event Timeline

ymandel created this revision.Aug 2 2022, 5:55 PM
Herald added a project: Restricted Project. · View Herald Transcript
ymandel requested review of this revision.Aug 2 2022, 5:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2022, 5:55 PM
sgatev added inline comments.Aug 2 2022, 10:35 PM
clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h
34

Can we make them references?

55–57

Can we make it a reference?

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

Can we make the keys FunctionDecl * and use FunctionDecl::getDefinition in getControlFlowContext to obtain a canonical pointer?

Looks awesome, thanks @ymandel!

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
23

I'm guessing this change is unrelated to the patch as a whole?

samestep accepted this revision.Aug 3 2022, 6:04 AM
This revision is now accepted and ready to land.Aug 3 2022, 6:04 AM
ymandel marked 4 inline comments as done.Aug 3 2022, 6:11 AM
ymandel added inline comments.
clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h
34

Turns out I was wrong - D can be null. Would that CFG has comments about what its parameters do... We have a test that excercises this (though I can't say why):

https://github.com/llvm/llvm-project/blob/9a976f36615dbe15e76c12b22f711b2e597a8e51/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp#L70

The other two -- yes -- but then we'd have to update the various callers as well. I'd rather not do that in this patch, but I'll add the new overload and we can follow up with cleanups in another patch.

55–57

See above.

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
23

correct. it was a driveby fix (for better or worse).

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

We can for now -- for the purposes of the cache that makes the most sense. I thought of strings because the goal is a map from *specific* (named) functions, for which we won't a priori have the function decls. still, I'll change for now and we can change it back (or to a better implementation) in the follow up patches.

ymandel updated this revision to Diff 449648.Aug 3 2022, 6:12 AM
ymandel marked 4 inline comments as done.

addressed comments

ymandel updated this revision to Diff 449649.Aug 3 2022, 6:19 AM

adusted interface to return (nullable) pointer rather than reference.

sgatev accepted this revision.Aug 3 2022, 6:20 AM
sgatev added inline comments.
clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h
34

Weird. I think we should change the test to pass the decl.

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

Perhaps FunctionContexts?

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
19

Is this still needed?

ymandel updated this revision to Diff 449658.Aug 3 2022, 6:55 AM
ymandel marked 2 inline comments as done.

update.

ymandel updated this revision to Diff 449660.Aug 3 2022, 6:57 AM

tweaks.

clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h
34

I'd like that, but when looking through other uses found a bunch that pass nullptr (and for no apparent reason - they all have the function decl handy).

So, I propose that we mark the new API as such and then in the cleanup we'll make sure to pass a non-null argument.

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
19

Nope.

ymandel updated this revision to Diff 449661.Aug 3 2022, 6:59 AM

comments

This revision was landed with ongoing or failed builds.Aug 3 2022, 8:20 AM
This revision was automatically updated to reflect the committed changes.
xazax.hun added inline comments.Aug 3 2022, 10:33 AM
clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h
38

I think LLVM can use the [[deprecated]] attribute.

63–65

This starts to look more and more similar to AnalysisDeclContext. I wonder if at some point we should factor the common code out into a common base class or something.

ymandel marked 2 inline comments as done.Aug 4 2022, 10:51 AM
ymandel added inline comments.
clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h
38

Thanks -- I verifed and it works. But, it produces warnings which I'm afraid may be a problem with some builds. So, I want to hold off until I can clean up all of our uses.

63–65

Agreed. Filed issue #56932.