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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Sam, this is a great start! I'm really excited to see that you have a core working so quickly.



================
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
----------------



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


================
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);
----------------
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.


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


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:524
+      auto CalleeEnv = Env.pushCall(S);
+      auto Analysis = NoopAnalysis(ASTCtx, /*ApplyBuiltinTransfer=*/true);
+
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130306