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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a subscriber: gribozavr2.
xazax.hun added a comment.

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



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


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:341
+  // Canonicalize the key, if possible.
+  if (auto *C = F->getCanonicalDecl())
+    F = C;
----------------
ymandel wrote:
> xazax.hun wrote:
> > Out of curiosity, I'd expect getCanonicalDecl to always succeed. Do you know cases where it would not?
> Hah. No, I don't but I also didn't find any mention in the headers guaranteeing otherwise, so I added this. I'm fine removing but I feel a little uncomfortable with the fact that it's not documented.
I see other places where we don't check for nullness, e.g.: https://github.com/llvm/llvm-project/blob/bb297024fad2f6c3ccaaa6a5c3a270f73f15f3ac/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp#L83

I think the CanonicalDecl is defined as the first declaration for redeclarable entities. I think getting the first element of a non-empty list should always succeed. But I definitely agree, this fact should be documented, or even better, this should probably return a reference. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131280