Page MenuHomePhabricator

[analyzer] Improve the accuracy of the Clang call graph analysis
Needs ReviewPublic

Authored by jcranmer-intel on Tue, Jul 30, 8:31 AM.

Details

Reviewers
dcoughlin
NoQ
Summary

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.

Diff Detail

Event Timeline

jcranmer-intel created this revision.Tue, Jul 30, 8:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jul 30, 8:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
NoQ accepted this revision.Fri, Aug 9, 12:49 PM
NoQ edited reviewers, added: NoQ; removed: dergachev.a.
NoQ added a subscriber: NoQ.

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.

NoQ added a comment.Fri, Aug 9, 12:50 PM

Do you have commit access or should i commit this for you?

No, I do not have commit access, so if you could commit it, it would be greatly appreciated.

NoQ added a comment.Tue, Aug 13, 3:39 PM

Hmm, the patch doesn't pass its own test for me, could you double-check?

The test has been passing for me. What error are you seeing?

NoQ added a comment.Wed, Aug 14, 1:39 PM

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.

NoQ added inline comments.Wed, Aug 14, 1:42 PM
clang/lib/Analysis/CallGraph.cpp
81–116

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.

jcranmer-intel marked an inline comment as done.Thu, Aug 15, 1:30 PM
NoQ added a comment.Thu, Aug 15, 3:02 PM

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.

Szelethus retitled this revision from Improve the accuracy of the Clang call graph analysis to [analyzer] Improve the accuracy of the Clang call graph analysis.Fri, Aug 16, 1:40 PM

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.