[PATCH] D130306: [clang][dataflow] Analyze calls to in-TU functions

Sam Estep via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jul 22 11:34:27 PDT 2022


samestep added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:220
+  assert(Body != nullptr);
+  initGlobalVars(*Body, Env);
+
----------------
ymandel wrote:
> samestep wrote:
> > ymandel wrote:
> > > samestep wrote:
> > > > ymandel wrote:
> > > > > I wonder how this will work between caller and callee. Do we need separate global var state in the frame? If so, maybe mention that as well in the FIXME above.
> > > > Could you clarify what you mean? Perhaps I just don't understand exactly what is meant by "global vars" here.
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp#L131-L135
> > > 
> > > ```
> > > /// Initializes global storage values that are declared or referenced from
> > > /// sub-statements of `S`.
> > > // FIXME: Add support for resetting globals after function calls to enable
> > > // the implementation of sound analyses.
> > > ```
> > > Since this already mentions a need to reset after function calls, seemed relevant here.
> > Hmm, OK. I pretty much just pattern-matched from the `Environment` constructor right above this method implementation. Would it be better for me to instead just remove this `initGlobalVars` call for now and replace it with a `FIXME` saying that we'll probably need to call this but it's unclear how exactly to do so?
> SGTM. We can note this constraint on models' reference implementations. Specifically, that they cannot reference globals.
Done.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:231
+  // parameters still need to be given `StorageLocation`s anyway, so this code
+  // will need to be generalized later.
+  for (; ArgIt != ArgEnd; ++ParamIt, ++ArgIt) {
----------------
gribozavr2 wrote:
> The Clang AST includes argument expressions for defaulted arguments, so I believe there shouldn't be anything left to do here, it should just work.
Oh nice! I'm updating this comment, thanks.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:3896
+                    *cast<BoolValue>(Env.getValue(*FooDecl, SkipPast::None));
+                EXPECT_FALSE(Env.flowConditionImplies(FooVal));
+              },
----------------
gribozavr2 wrote:
> 
Good idea, done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130306/new/

https://reviews.llvm.org/D130306



More information about the cfe-commits mailing list