This patch improves Clang call graph analysis by adding in expressions that are not found in regular function bodies, such as default arguments or C++ member initializers.
Details
Diff Detail
Event Timeline
This will slightly skew Static Analyzer exploration order, not necessarily in a good way, as default arguments and (sometimes) default initializers are not modeled properly. But this doesn't sound dangerous, because whether functions will be analyzed at top level doesn't depend on where they're positioned in the CallGraph, but instead on whether they've been actually visited during symbolic execution. So it's mostly about order rather than coverage. So i'm not worried.
No, I do not have commit access, so if you could commit it, it would be greatly appreciated.
For me ddd() doesn't call c::c(). I can fix it by adding the following code:
void VisitCXXConstructExpr(CXXConstructExpr *CE) { addCalledDecl(CE->getConstructor()); VisitChildren(CE); }
I don't see why it would work without that code, as CXXConstructExpr isn't a sub-class of CallExpr. So that's another obvious omission in the CallGraph.
Might it be that you have it locally but forgot to include in the patch? It also fails a couple more unrelated tests when i add it because it affects analysis order.
clang/lib/Analysis/CallGraph.cpp | ||
---|---|---|
93 | Yes, there it is. Where does it come from? I don't have it in master. |
Okay, I see the issue now. I originally developed this patch on a fork with a whole lot of extra changes, and that fork included some extra modifications to the callgraph that I had missed: https://github.com/intel/llvm/commit/971fecdc316930c0c1c79283d1094ee4c4ca41e0#diff-cae4e2b4043cd0f49ce29e77de22a5a5. I'll merge the callgraph-related changes in that patch back onto a clean patch for upstream.
I've rolled the relevant call graph analysis changes from the prior commit into this updated patch.
Thanks! This looks very useful.
clang/lib/Analysis/CallGraph.cpp | ||
---|---|---|
97–102 | Ugh this is questionable. The object may easily outlive the function, so the destructor may be called from another function. I see where this was supposed to go (i.e., this is perfectly correct when the constructor is constructing a non-RVO'd temporary or a non-NRVO'd local variable), but in the general case it's not true. The really nice way to implement this functionality would be to rely on the constructor's ConstructionContext, but as of now it's only available in the CFG and there's no easy way to retrieve it by looking at the AST node (and if you have a CFG you might as well look at destructors directly, as they're listed there explicitly). It should also be possible to do that by hand by matching DeclStmts and CXXBindTemporaryExprs. Let's omit this part for now because currently the analysis seems to be conservative in the sense that it doesn't add calls when it's not sure. |
I hope you dont mind me changing the revision title -- many of us are subscribed to the "analyzer" tag and like learning about whats changing :)
This is not an objection to the patch or anything of course, just adding some lurkers.
I think there are use cases for having a callgraph that errs on the side of adding edges that might not exist, but I'm happy enough to leave that for a later patch.
This does remind me that we need a real clang IR that we can leverage for this sort of thing.
Yes, there it is. Where does it come from? I don't have it in master.