[cfe-dev] CFG support for temporary object desctructors

[Pavel Labath]

Hello,

thank you for the response. It makes things much clearer. However, I still
have some questions.


On 25 June 2013 04:03, Jordan Rose <jordan_rose at apple.com> wrote:

> Hi, Pavel. It's great to hear that you're interested in this.
>
> As I remember from last time I looked at this, there are three things
> holding us back from turning on support for temporary destructors in the
> analyzer. The first is that the analyzer is not wired up to handle
> destructor CFG nodes for arbitrary regions. I don't think this is a major
> blocking issue, but it might take a bit of threading through all the code.
>
After turning on the desctructors i've found some unimplemented functions
and asserts in the code which were stopping me. I have replaced them with
some code, which I cargo-culted from functions dealing with other types of
destructors.

And this actually seems to work fairly well. All my null pointer
dereferences have disappeared. At this moment, I am having only two issues
with this fix.

The first happens in patterns like:
 const Object& foo() { return Object(); }
Here, the constructed CFG looks like:

ConstructCXXExpr
BindTemporary
ImplicitCastExpr
MaterializeTemporaryExpr
Temporary destructor call

I believe here we should put an Implicit destructor call, given that the
object is materialized, as it is done in the case of
const Object &foo = Object();
Is that correct? If so, I can make a patch for that. It shouldn't be too
difficult.

The second issue happens with code patterns like:
Object *o;
...
foo(&o);
...
o->bar();

Here, I get a "dereferencing undefined pointer" warning on the last line,
but it only happens for some values of "...". I am still investigating how
is this related to the destructor problem and what exactly is going on
there.



> The second is a lot more difficult. When you pass temporaries to a
> function, what happens when they are destroyed?
>
> void byRef(const Object &ref);
> void byVal(Object val);
>
> byRef(Object())
> byVal(Object())
>
> The destructor here has to run at the end of the full-expression
> containing the call (roughly, at the outermost expression). However, the
> function call may have changed some of the object's contents (even 'byRef',
> if the object has fields marked 'mutable'). We need to make sure that's
> reflected when the chosen destructor is inlined.
>
> The inlining problem is compounded by the fact that we decay structs to a
> collection of values (nonloc::LazyCompoundVal) whenever we need to treat
> them as rvalues. This is usually the right thing to do, but has very
> confusing results for temporaries being copied in and out of functions.
> According to the standard, the copy constructor happens in the caller (and
> that's how it appears in the AST), but the region it's being copied into is
> based on a ParmVarDecl that's part of a StackFrameContext for inlining the
> function...which we may not decide to inline after all. Ignoring temporary
> destructors our current behavior is indistinguishable from the standard,
> but as soon as we start claiming to support temporary destructors we're
> going to hit this problem.
>
I find this a bit strange. I can't say I know all the quirks of the c++,
but to me the first case looks very similar to:
{ Object foo; byRef(foo); }
You still need to construct the object, pass a reference to the function
and then call the destructor. Obviously, the AST for the two fragments
looks very differently, and we need to take care when parsing it to
construct a CFG. However, once we already have a CFG, processing it should
not depend (too much) on whether we are working with a temporary or a
normal object. Specifically, I do not see why internal implementation
details (LazyCompoundVal, et al.) should cause problems in one case but not
the other. Even in case of normal objects, you still need to pass the
reference to a function and then reflect those changes when you inline the
destructor.

What am I missing?



>
> The third problem is that we simply haven't put time into qualifying and
> validating the temporary destructor logic. Marcin implemented it a long
> time ago in the CFG, then Chandler and Doug made sure it was in good enough
> condition to use for the analysis-based warnings, but we haven't actually
> tested it in the analyzer.
>

> Now, all of that said, you're only interested in making 'noreturn' work
> right now, so for that it seems reasonable to treat the destructors for
> temporary objects as opaque in the analyzer. It may turn out that we were
> properly conservative in everything we've done so far that turning it on
> will just work, but I'd like to see a fair number of test cases before we
> start shipping that.
>
> 'noreturn' is actually the simplest thing here: that's just a change in
> the shape of the CFG, not necessarily in its use. It's all the other
> destructors that come along for the ride that worry me. (And the setup of
> the call to the noreturn destructor as well.)
>
> What do you think?
> Jordan
>
> P.S. I asked "what do you think?", but I'm going to be on vacation for the
> next week, so please don't expect an immediate response. Sorry about the
> time for *this* response.
>
>
After reading this email, I took a look if it is possible to make the
analyzer not inline temporary destructors. But I couldn't find a _nice_ way
to do that. At the point where the inlining decision is made, I don't have
access to the type of the destructor call. Obviously, I could  push
information through somehow, but instead I decided to take another look at
what happens when I actually do inline the destructors, since it already
seems to be mostly working. I will try to fix the two issues I mentioned
above (which means the CFG will get more testing and validation) and then
you can decide whether the code is stable enough to be switched on. If I
fail at that, I can always make the destructors opaque, as you suggested.

Is that ok with you?

regards, and enjoy the vacation,
pavel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130626/b53aaf09/attachment.html>