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

[章磊]

Hi Ted,

I re-implement this checker again.

This time i introduce several data structure to do DP and the bugreport.

   1. I modified the worklist a bit, now its unit is a <FunctionDecl *,
   CallExpr *> pair from the caller's function decl to the call expr.
   2. RuntimeStack(vector of CallExpr) to simulate the call graph
   dynamiclly. This is for report the entire chain of the call path.
   3. A DenseMap from a FunctionDecl to all call paths in its body.
   4. A SmallPtrSet to  Record which functions have be fully handled. A
   function fully handled means that all function bodies it called have been
   handled.

I could not find a right way for this kind of bug report, it's not path
diagnostic and we need to annotate the call expr in the call path. So i
simply output the name with " <-- " to show the call-called relationship.
It will crash on some code because "Name is not a simple identifier"? But
if we use the SourceRange, there would not be any crash like this, right?

And i left several things unconsidered.

   1. The memory consume and reclaim.
   2. The readabiliy of the code. It's a little hard to read now.

Do you have any advices on these?

More comments inline, and looking forward to your reply.

在 2011年10月27日 下午12:20，Ted Kremenek <kremenek at apple.com>写道：

> 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>
>
>
>


-- 
Best regards!

Lei Zhang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111111/cfda4616/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: VirtualCall.patch
Type: application/octet-stream
Size: 12931 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111111/cfda4616/attachment.obj>