Fix scoped_ptr::Pass to not rely on undefined behavior

Fix scoped_ptr::Pass to not rely on undefined behavior

Invoking "other.release()" where other is of type RValue invokes undefined behavior, because the given pointer is actually of type scoped_ptr<C>. With the NaCl toolchain, this doesn't work as intended, and the pointed-to object gets destructed twice.

BUG=116317
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143150

[dmichael (off chromium), 2012-06-19 22:43:51]

(see also http://code.google.com/p/nativeclient/issues/detail?id=2859 )

[awong, 2012-06-19 22:53:36]

The fix as is looks okay, but I didn't think it was necessary.  Are we sure it's
undefined behavior?

RValue is a child of Foo, so I would have expected that executing a non-virtual
method of Foo via an RValue would work.

[awong, 2012-06-19 23:37:06]

lgtm with one comment.

dmichael: Can you add a comment saying that these are needed to "undo" the
reinterpret_cast inside move.h?  Otherwise looks good to me.

jyasskin helped hash this out on the #llvm channel.  It does seem like truly
undefined behavior.

Let's put this patch in for now.  I will need to go fix move.h later because as
is, all users must remember to use reinterpret_cast.  That should be a separate
CL though.


On 2012/06/19 22:53:36, awong wrote:
> The fix as is looks okay, but I didn't think it was necessary.  Are we sure
it's
> undefined behavior?
> 
> RValue is a child of Foo, so I would have expected that executing a
non-virtual
> method of Foo via an RValue would work.

[dmichael (off chromium), 2012-06-20 02:33:53]

On 2012/06/19 22:53:36, awong wrote:
> The fix as is looks okay, but I didn't think it was necessary.  Are we sure
it's
> undefined behavior?
From C++11, 5.2.10.7:
"""
7 A pointer to an object can be explicitly converted to a pointer to a different
object type.
69 When a
prvalue v of type “pointer to T1” is converted to the type “pointer to cv T2”,
the result is static_cast<cv
T2*>(static_cast<cv void*>(v)) if both T1 and T2 are standard-layout types (3.9)
and the alignment
requirements of T2 are no stricter than those of T1. Converting a prvalue of
type “pointer to T1” to the
type “pointer to T2” (where T1 and T2 are object types and where the alignment
requirements of T2 are no
stricter than those of T1) and back to its original type yields the original
pointer value. The result of any
other such pointer conversion is unspecified.
"""
Casting back to the original pointer type must yield the original pointer value,
but the pointer value of the RValue pointer in this case is unspecified. At
least, that's how I read it.

> 
> RValue is a child of Foo, so I would have expected that executing a
non-virtual
> method of Foo via an RValue would work.
Right; the problem is the pointer is not really to an RValue; that's just its
static type. But I agree, I would have expected it to work in practice. I'm kind
of curious what the NaCl toolchain was doing to make this happen (and only at
-O2 and higher). But I didn't bother looking at disassembly :-). Maybe the NaCl
toolchain guys will shed some light in:
http://code.google.com/p/nativeclient/issues/detail?id=2859

[dmichael (off chromium), 2012-06-20 03:02:03]

On 2012/06/19 23:37:06, awong wrote:
> lgtm with one comment.
> 
> dmichael: Can you add a comment saying that these are needed to "undo" the
> reinterpret_cast inside move.h?  Otherwise looks good to me.
Done. Let me know if you don't like the wording and I can circle back and
improve it.

> 
> jyasskin helped hash this out on the #llvm channel.  It does seem like truly
> undefined behavior.
> 
> Let's put this patch in for now.  I will need to go fix move.h later because
as
> is, all users must remember to use reinterpret_cast.  That should be a
separate
> CL though.
There may be cleaner ways, but a quick-n-dirty way to force it would be to
remove the inheritence, so there's no way to accidentally call release() without
casting. Might be better though to make RValue just store the pointer (so we
don't have to do messy reinterpret_cast at all). But I haven't thought hard
about it.

> 
> 
> On 2012/06/19 22:53:36, awong wrote:
> > The fix as is looks okay, but I didn't think it was necessary.  Are we sure
> it's
> > undefined behavior?
> > 
> > RValue is a child of Foo, so I would have expected that executing a
> non-virtual
> > method of Foo via an RValue would work.

[commit-bot: I haz the power, 2012-06-20 03:02:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10579030/2002

[commit-bot: I haz the power, 2012-06-20 03:02:29]

Presubmit check for 10579030-2002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base

[brettw, 2012-06-20 03:22:40]

lgtm

[commit-bot: I haz the power, 2012-06-20 03:26:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10579030/2002

[commit-bot: I haz the power, 2012-06-20 05:47:15]

Change committed as 143150