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

[Sam Estep via Phabricator via cfe-commits]

samestep added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:208
+
+  // TODO: Currently this only works if the callee is never a method and the
+  // same callee is never analyzed from multiple separate callsites. To
----------------
ymandel wrote:
> 
OK, I'll change this; would you like for me to replace all the other `TODO`s with `FIXME`s, as well?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:220
+  assert(Body != nullptr);
+  initGlobalVars(*Body, Env);
+
----------------
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.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:237
+    const Expr *Arg = *ArgIt;
+    // TODO: Confirm that this is the correct `SkipPast` to use here.
+    auto *ArgLoc = Env.getStorageLocation(*Arg, SkipPast::None);
----------------
ymandel wrote:
> I'm pretty sure we want `SkipPast::Reference`. That will ensure that the parameter and argument share the same underlying location. Otherwise, in the case of references, the parameter will point to the reference location object rather than just directly to the location.
OK, thank you! I'll make that change.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:517
+
+      // TODO: Cache these CFGs.
+      auto CFCtx = ControlFlowContext::build(F, F->getBody(), &ASTCtx);
----------------
ymandel wrote:
> here and below: s/TODO/FIXME.
Will do.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:524
+      auto CalleeEnv = Env.pushCall(S);
+      auto Analysis = NoopAnalysis(ASTCtx, /*ApplyBuiltinTransfer=*/true);
+
----------------
ymandel wrote:
> This seems worth a FIXME or, at least, an explanation.  It implies that with the current design, we can't support general-purpose analyses, which we should probably fix. Given our goal of supporting models that don't involve specialized lattices, I think this is a good compromise for the short term, but not a stable solution for the framework (hence  FIXME sounds right).
Good point, and @xazax.hun pointed this out as well. I'll add a `FIXME` here, at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130306