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

[Ted Kremenek]

Hi Lei,

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
>  
> <VirtualCallinCtorOrDtor.patch>

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