[cfe-dev] Check virtual calls in ctor or dtor
kremenek at apple.com
Wed Oct 26 21:20:56 PDT 2011
This is looking good.
I think one way to do DP is by modifying CheckedFunctions slightly. Instead of CheckedFunctions just recording whether or not a function has been put on the worklist, you record whether or not it calls a virtual method (perhaps by registering the callsite?). You could thus have three possibilities:
(1) Function hasn't been analyzed (if not, enqueue on the worklist)
(2) Function has been analyzed, and calls a virtual function.
(3) Function has been analyzed, and doesn't call a virtual function.
This should be easy to model with a DenseMap, mapping from the Decl* to the appropriate information. You can consult the information for 1-3 before deciding the "recurse" into analyzing a called method.
As for propagating the information via dynamic programming, since your worklist is a stack, you know the immediate caller. Once you see that a virtual method is called, essentially that means the entire chain of calls results in the call of a virtual method. You can then annotate them all in one swoop. This is essentially what you are assuming when you emit a warning.
On Oct 25, 2011, at 11:50 PM, 章磊 wrote:
> Hi Ted,
> I re-implement this check as a static analyzer checker, and i make the recursion a work list as you suggested.
> Currently I'm not using dynamic programming but introduced a SmallPtrSet to avoid analyzing the same methods again. It's a top-down analysis, how could i introduce dynamic programming? By adjust the work list? It seems that if we introduce DP we still need a set to record those method we have analyzed.
> What you Say?
> 2011/10/4, Ted Kremenek <kremenek at apple.com>:
> > Hi Lei,
> > The part that scares me is the recursive walk of the callee's body. That's
> > not likely to have good enough performance for a compile-time warning. For
> > a compile-time warning, I think it is fine to just check the body of the
> > constructor, but if you want to analyze the bodies of called functions you'd
> > need to be extremely clever to do it efficiently enough for it to be a
> > compile-time warning. We really like compiler warnings to have highly
> > predictable performance, and recursively walking the entire classes's method
> > bodies is not really amendable to that.
> > If you think walking through all the bodies is necessary, then I'd leave it
> > as a static analyzer checker. You'd certainly get better coverage. To
> > handle the recursion, I'd probably make it a worklist (i.e., data recursive)
> > to avoid stack blowup and to keep your memory footprint small. You should
> > also employ dynamic programming to avoid analyzing the same methods again.
> > The other nice thing about this approach is that it probably can be nicely
> > generalized to global IPA once we have it.
> > One caveat about doing a recursive analysis of method bodies without any
> > path-based analysis is that you are introducing additional risk of a false
> > positive. There could be unreachable paths where a constructor can't
> > actually transitively trigger a call to a virtual function because of the
> > context, but your check may report an issue. I don't think this will
> > typically be a problem, but it is something to consider.
> > Cheers,
> > Ted
> > On Sep 27, 2011, at 1:39 AM, 章磊 wrote:
> >> Hi Ted,
> >> My goal is to check whether there is a virtual call (directly or
> >> indirectly) in construction or destruction.
> >> In my former patch i do this check to all the constructors and the
> >> destructor in checkASTDecl(const CXXRecordDecl ...). My way is walk
> >> through the ast body of ctor/dtor: if there is virtual call, warn it; if
> >> there is a call, walk through the callee's body recursively.
> >> I think it's ok to do this check without the analysis engine and the
> >> inline IPA.
> >> What you say?
> >> 在 2011年9月26日 下午9:37，Ted Kremenek <kremenek at apple.com>写道：
> >> On Sep 25, 2011, at 2:32 AM, 章磊 wrote:
> >>> Hi Ted,
> >>> I tried to implement this as a compiler warning. But i had some
> >>> problem in how to get the body of the ctor or dtor.
> >>> I implemented this in Sema::CheckConstructor and
> >>> Sema::CheckDestructor, but i can not get the body of them when there
> >>> is no body with the declaration yet.
> >> The idea how I thought this would be implemented was to do the analysis
> >> when processing CallExprs as we are building them. We should know up
> >> front when we are about to process a constructor body. Simply set/clear a
> >> Sema flag when processing the constructor body, and your check can consult
> >> that flag when processing a CallExpr to see if it needs to do the check.
> >> Alternatively, checking the DeclContext may be all that is needed to see
> >> if we are currently in a constructor body.
> >> The advantage of this approach is that it keeps all the checking local.
> >> Walking the AST of a constructor body *after* we have constructed it is
> >> slow, and not really a great approach for doing compiler warnings in the
> >> frontend (if we can at all help it).
> >>> And i also need the function body
> >>> of the CallExpr in the ctor/dtor to do recursive analysis.
> >> Interesting. If your goal is to do inter procedural analysis, then this
> >> should remain a static analyzer check, or that should be a case caught
> >> beyond what a compiler warning should do. That wasn't clear to me before
> >> that this was something you wanted to do.
> >> The tradeoffs between compiler and static analyzer warnings are fairly
> >> simple:
> >> 1) Compiler warnings have higher visibility because all users see them all
> >> the time when building their code.
> >> 2) The logic behind compiler warnings must be as fast as possible.
> >> Inter-procedural analysis is usually a show stopper, and ideally the
> >> checks are highly local. Sema is already "walking" all of the code as it
> >> builds the AST, so often the warning logic can be put in key places,
> >> essentially where the warning would be issued.
> >> --
> >> Best regards!
> >> Lei Zhang
> Best regards!
> Lei Zhang
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-dev