Two object constructed at the same location in stmt-expr in a return stmt

[higher-performance]

It seems #63818/#89154 haven't been quite fixed yet. In some sense, I think the fix actually made things worse, because we now get a double-destruction (which is more likely to cause a security issue) instead of neglecting to run a destructor (which would've been more likely to cause a leak instead).

Repro:

#include <stdio.h>
struct D {
    ~D()              { printf("[%p] D::~D()\n"           , this); }
    D()               { printf("[%p] D::D()\n"            , this); }
    D(int x)          { printf("[%p] D::D(int %d)\n"      , this, x); }
    D(D const &other) { printf("[%p] D::D(D const & %p)\n", this, &other); }
};
struct S { D d; int i; };
static S f() { return S{ D(1), ({ return S(); 0; }) }; }
int main() { return f().i; }

Clang (trunk):

[0x7ffdfbc4df68] D::D(int 1)
[0x7ffdfbc4df68] D::D()
[0x7ffdfbc4df68] D::~D()
[0x7ffdfbc4df68] D::~D()

Clang 18.1.0:

[0x7fffa42f9e60] D::D(int 1)
[0x7fffa42f9e60] D::D()
[0x7fffa42f9e60] D::~D()

Expected behavior: The D(1) subobject should be destroyed before S() constructs an object on top of it.

Actual behavior: The object is constructed twice at the same location, then destroyed twice at the same location.

[usx95]

I can come back with a more detailed analysis later as I am traveling rn.
But this looks like it's working as intended.
The first D() is extended till the end of expression (probably even lifetime extended to the function scope).

D() would be constructed twice because of first D() and then a different instance in S().

Can you give a different example where u see things not working as intended specially with coroutines?

[higher-performance]

They are all in the same memory location though. So the second construction overwrites the first one. Is that not a bug?

[efriedma-quic]

Yes, it's a bug, but the bug is that they're in the same memory location, not that we're destroying both objects. And that's specific to nested "return" statements.

I'm not sure there's any reasonable way to lower this case: the only way to make it work right is to insert copy-constructor calls in places where they wouldn't normally exist. So probably this is just a missing diagnostic.

[higher-performance]

By "double-destruction" I meant of the same object. (IIUC destruction of 2 different objects isn't referred to as double-destruction.) Sorry if this was unclear, hope this clarifies.

Why can't you lower this per the "Expected behavior" in the post:

The D(1) subobject should be destroyed before S() constructs an object on top of it.

[efriedma-quic]

By "double-destruction" I meant of the same object.

Right, but my point is that from the compiler's perspective, it's not the same object; it's two completely unrelated objects that are forced to the same address due to the way "return" works.

Why can't you lower this per the "Expected behavior" in the post:

There's a few ways we could handle this case... we could do that, or we could force the return value to be copy-constructed. Either way, we're way outside normal C++ rules; it's not worth implementing just to allow people to do funny stuff with statement expressions.

[higher-performance]

Copy-construction would fail if the object isn't copyable though. And would break RVO. The only solution would seem to be to destroy the previously-constructed object prior to constructing a new one.

And of course; there was no claim that this is in the C++ rules. But Clang advertises support for extensions to C++ like statement-expressions, so I presume it would be interested in fixing bugs in them.

[usx95]

Sorry, I missed the part that these are constructed at the same location. As @efriedma-quic pointed out, this is the bug here and is not related to double or missing destructors.
(I was not aware of this issue with stmt-expr in a return statement and thought missing dtors was the only piece missing in the support of stmt-expr)

[efriedma-quic]

The primary issue with trying to "undo" the return is that it involves destroying objects out of order: normally, temporaries are either not destroyed, or destroyed in reverse order of construction. But your proposed semantics break that ordering: we can't destroy all temporaries in reverse order because some of them are part of the statement expression itself... so we'd have to specifically pick and choose which temporaries to destroy, and whatever we choose is likely to be surprising.

Given that, I'd rather just tell the user "please write your code differently".

[higher-performance]

The primary issue with trying to "undo" the return is that it involves destroying objects out of order: normally, temporaries are either not destroyed, or destroyed in reverse order of construction.

I don't believe this is true. RVO already violates that, IIUC. As an example, the following only has temporaries and they are destroyed in the same order of construction rather than the reverse:

static D f() {
    return (D(1), D());
}
int main() {
    f();
}

Output:

[0x7ffc5aacde9f] D::D(int 1)
[0x7ffc5aacde9e] D::D()
[0x7ffc5aacde9f] D::~D()
[0x7ffc5aacde9e] D::~D()

whatever we choose is likely to be surprising.

Again, I don't believe this is a concern. Both because RVO already violated that (per the above), and also because even if it were surprising, it would be only very mildly, vs. the current situation which seems far more dangerous.