[cfe-dev] Check virtual calls in ctor or dtor

[Ted Kremenek]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111003/5e086200/attachment.html>