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

[章磊]

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...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111026/25dac648/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: VirtualCallinCtorOrDtor.patch
Type: application/octet-stream
Size: 7545 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111026/25dac648/attachment.obj>