[PATCH] D131280: [clang][dataflow] Parameterize analysis by explicit map of analyzable functions.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D131280#3706988 <https://reviews.llvm.org/D131280#3706988>, @xazax.hun wrote:

> In D131280#3706915 <https://reviews.llvm.org/D131280#3706915>, @ymandel wrote:
>
>> Sure. This is probably worth some discussion. Fully qualified names, however we define them, will not be enough, since they don't cover overload sets.
>
> I see. This is not a unique problem. I think there were multiple discussions about API Notes and those need to solve the same problem. @gribozavr2 probably has more context on the current status of API Notes. An alternative to fully qualified names is Clang's USR that is often used for cross-referencing functions across translation units. Less user friendly, but will support overloads.
>
>> I'd like some mechanism that matches how identifiers are used. So, for example, inline namespaces should *not* be necessary, since they are an implementation detail from this perspective.
>
> A similar matching is already implemented for the Clang Static Analyzer. See https://clang.llvm.org/doxygen/classclang_1_1ento_1_1CallDescription.html and https://clang.llvm.org/doxygen/classclang_1_1ento_1_1CallDescriptionMap.html
>
> One example use is in the CStringChecker: https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp#L136

Thanks. That looks good, but I'm concerned that it only counts the arguments and doesn't look at their types. I'd imagine this will be a limitation down the line when we want to deal with overload sets w/ the same number of arguments, but different types.

Aside: why the `const char *` interface? Do you think owners would be open to a `llvm::StringRef` overload for the constructor?



================
Comment at: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp:96
+  for (auto It = Unit.top_level_begin(); It != Unit.top_level_end(); ++It) {
+    if (auto *C = dyn_cast<CXXRecordDecl>(*It)) {
+      for (auto *M : C->methods())
----------------
xazax.hun wrote:
> ymandel wrote:
> > xazax.hun wrote:
> > > Do we exclude non-toplevel declarations on purpuse? Or would this work for methods of inline classes, methods of classes defined within a function? 
> > > Do we exclude non-toplevel declarations on purpuse? Or would this work for methods of inline classes, methods of classes defined within a function? 
> > 
> > I think that for the current use case -- models of library types and their methods/functions -- we don't have a good usecase for this. But, I can see this becoming an issue if we want to expand to inlining other declarations. So, I'm inclined to hold off on this for the time being, since that's a larger design discussion.
> > 
> > Yet, I also think this should be generalized to take any decl and extract the functions/methods. I limited to `ASTUnit` for convenience, since that was the immediate need. I'm happy to either:
> > 1. Add a FIXME, and/or,
> > 2. Split this function into two: one that takes two decl iterators (begin, end) and does this work here and another which is just a convenience function for ASTUnit to apply the above to the top-level decls.
> > 
> > WDYT?
> I am fine with the current behavior, but I think the docstring "Builds a map of all declared functions in the given AST" is misleading in this case. Specifying in the docstring that this function will only map the top-level functions and methods of top-level classes sounds good to me.
Thanks, done.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:549
 
-      // Note that it is important for the storage location of `S` to be set
-      // before `pushCall`, because the latter uses it to set the storage
-      // location for `return`.
-      auto &ReturnLoc = Env.createStorageLocation(*S);
-      Env.setStorageLocation(*S, ReturnLoc);
-      auto CalleeEnv = Env.pushCall(S);
+      const FunctionDecl *FuncDecl = CFCtx->getDecl()->getAsFunction();
+      assert(FuncDecl != nullptr && "ControlFlowContexts in the environment "
----------------
sgatev wrote:
> How is that different from `F`? Why not let `Environment::pushCall` get this from the `CallExpr` argument?
Added comment to explain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131280