discarded-`reloc` of copy-only class

[ecatmur]

If a class was written before C++11 and thus has a copy constructor but no move constructor, then a discarded reloc may have precisely the wrong effect, constructing and destroying a temporary with overhead and no effect on the source object. This may sound unlikely, but in industry it's not unusual to have to work with vendor code that was written a very long time ago or has to target ancient compilers as well as modern ones.

I think it might be necessary to make discarded-reloc ill-formed if the source object is a function parameter and the constructor selected for relocation would be a copy constructor. Either that or call it out with a strong hint that implementations should issue a warning, but we may as well get this right to start with.

[SebastienBini]

I agree this is an issue. However I don't know if this should be ill-formed, as users may simply want to reduce the scope of the name. The temporary could simply be elided as it would have no effect.

Ideally, this is how I'd like the reloc obj; to behave:

	obj is not an unowned parameter	obj is an unowned parameter
obj has a relocation ctor	Calls the destructor of obj.	A temporary is created from the move constructor, and then destructed. The source object destruction is deferred until the function returns.
obj has a move ctor and no reloc ctor	Calls the destructor of obj.	A temporary is created from the move constructor, and then destructed. The source object destruction is deferred until the function returns.
obj only has a copy ctor	Calls the destructor of obj.	no-op, only prevents from reuse of the name obj. Compilers may emit a warning. But the users may simply want to reduce the scope of the name obj.
obj has no copy, move or reloc ctor	Ideally, calls the destructor of obj. If we can't establish proper rules to enable this, then ill-formed (we don't want implementation-dependent well-formedness).	Ill-formed.

Do we agree on this?

If we reason in terms of value parameter instead of unowned parameter (which is implementation-dependent), then we can make reloc obj; well-formed regardless of its constructors, only calling its destructor in cases where obj is a non-parameter local variable.

Then lock_guard lg{some_mutex}; reloc lg; can work.
And foo(lock_guard{some_mutex}); still gives the guarantee that the lock guard won't be relocated away by foo, still ensuring that the lock is released when foo returns.

[ecatmur]

Uh, I'm not sure.

users may simply want to reduce the scope of the name

If that's a legitimate ask, why wouldn't that also apply to non-relocatable types? I feel this is just too dangerous to allow. To reduce the scope of a name without destroying the object, you can (in most cases) take the parameter by A&& or A const&. Worth adding to discussion, perhaps.

obj has a relocation ctor [...] A temporary is created from the move constructor [...]

What about a type that has a relocation constructor and a copy constructor but no move constructor? Like, uh, gsl::non_null<std::shared_ptr<int>>?

Ideally, calls the destructor of obj.

Yes. I think we can do this by tweaking temporary materialization of discarded-value reloc-expressions:

The temporary materialization conversion ([conv.rval]) is applied if the (possibly converted) expression is a prvalue of object type that is not a (possibly parenthesized) reloc-expression whose id-expression denotes an entity other than a function parameter.
[Note: The temporary materialization conversion in a discarded-value function parameter reloc-expression may nevertheless be elided ([class.reloc.elision]). --end note]

[SebastienBini]

If that's a legitimate ask, why wouldn't that also apply to non-relocatable types? I feel this is just too dangerous to allow. To reduce the scope of a name without destroying the object, you can (in most cases) take the parameter by A&& or A const&. Worth adding to discussion, perhaps.

Indeed. However things still work if the value is not discarded:

void foo(non_null<shared_ptr<int>> ptr) // assuming caller-destroy ABI
{
  if (sometest())
    bar(reloc ptr); // well-formed, calls the copy constructor, ptr destructor is deferred
  else
    reloc ptr; // ill-formed because discarded???
}

I'd prefer to have the compiler emit a warning in that case. Besides if the ABI were callee-destroy, there would be no problem.

Another take would be to have reloc not even trying to call the copy constructor, using only the reloc or move ctor. Hence reloc obj would be ill-formed if relocation were to happen by the copy constructor. I discarded this approach early, as it's quite bad for writing generic code.

Problem is reloc is used for two different things, sometimes to perfect forward, other times to reduce the lifetime. Users should have no expectations on the latter with value parameters (unless the type is relocate-only).

Ideally, calls the destructor of obj.

Yes. I think we can do this by tweaking temporary materialization of discarded-value reloc-expressions:

The temporary materialization conversion ([conv.rval]) is applied if the (possibly converted) expression is a prvalue of object type that is not a (possibly parenthesized) reloc-expression whose id-expression denotes an entity other than a function parameter.
[Note: The temporary materialization conversion in a discarded-value function parameter reloc-expression may nevertheless be elided ([class.reloc.elision]). --end note]

Looks good!

[ecatmur]

void foo(non_null<shared_ptr<int>> ptr) // assuming caller-destroy ABI
{
  if (sometest())
    bar(reloc ptr); // well-formed, calls the copy constructor, ptr destructor is deferred
  else
    reloc ptr; // ill-formed because discarded???
}

Yeah, if the latter is ill-formed, the former should be as well. The former has its own issues; someone passing in a make_shared prvalue (so with refcount 1) may expect bar to be able to cause it to be freed. But that's a general problem with caller-destroy ABI.

I discarded this approach early, as it's quite bad for writing generic code.

Yes, good point. However, there is the option to have different behavior for dependent and non-dependent types, making the above ill-formed (on both lines, perhaps) while making it allowed if foo were a function template. This might sound crazy, but we have it in other places (e.g. return void();).

[SebastienBini]

Yeah, if the latter is ill-formed, the former should be as well.

Agreed.

Yes, good point. However, there is the option to have different behavior for dependent and non-dependent types, making the above ill-formed (on both lines, perhaps) while making it allowed if foo were a function template. This might sound crazy, but we have it in other places (e.g. return void();).

I fail to see how would that look. So reloc on copy-only types would only be allowed on template functions? That looks a bit weird. If the behavior of reloc is acceptable for template functions, why is it not in non-template functions? Stubborn developers may simply make their non-template functions call a dedicated template-function just to freely use reloc in them.

To be honest, I don't see what the real danger is here. I get that this behavior is not ideal, but I don't see it as a blocking issue. I doubt developers are really concerned on when copy-only types are destroyed, as their destructors rarely have desirable side effects.

The same issue, in some extent, exists today with move:

template <class T> void bar(T);
template <class T> void foo(T x) { bar(std::move(x)); do_somestuff(); }

If x has no move constructor (and not defined as deleted) then foo calls the copy constructor. x is left untouched and not in a moved-from state.

Again, users should have no expectations on when non-relocate-only value-parameters are destructed with reloc.

What really annoys me is that reloc can only be used in new/updated functions, and as such, those functions should take advantage of the fact that they are recompiled to be turned to the callee-destroy ABI or equivalent, (which would close the issue). However I can't find a way to achieve this:

• explicitly mark value parameters as callee-destroy in the signature: void foo(reloc T x); this would be quite ugly, and will end up being a mess with function pointers and such.
• emitting two functions with different mangling, like in the assignment operator, to preserve the ABI. However, it comes at a non-acceptable cost: for the problem we are trying to solve (copy-only parameters) the non-eliding function will need to make a deep copy of the parameter and pass it as if by reference to the eliding one. The eliding one may not even perform the relocation (e.g. exception is thrown before, or reloc is used in a condition), and the expensive copy will have been wasted.

[ecatmur]

To be honest, I don't see what the real danger is here. I get that this behavior is not ideal, but I don't see it as a blocking issue. I doubt developers are really concerned on when copy-only types are destroyed, as their destructors rarely have desirable side effects.

Yes, but they may release resources, e.g. a refcnt'ed database cursor. (Or, worse, an auto-commit transaction object - though that would be pretty bad code.) OTOH since this only affects weird/old classes, and slightly odd code, and only certain platforms and only if the implementer and user have chosen the path of full backwards compatibility, I think it's enough to call this out in discussion; I'll get writing it up.

The same issue, in some extent, exists today with move:

This is a clang-tidy warning: https://clang.llvm.org/extra/clang-tidy/checks/performance/move-const-arg.html It's a little different, though, since that's always got that behavior (so will be detected in testing) whereas for reloc x it's implementation-defined, meaning you could see it as a latent bug to come out during porting.

• explicitly mark value parameters as callee-destroy in the signature: void foo(reloc T x); this would be quite ugly, and will end up being a mess with function pointers and such.

Yes; the beauty of this proposal is that it has zero impact on the type system and indeed can be regarded as completing it.

• emitting two functions with different mangling

It's not so bad, if you imagine that most of the time you're calling the eliding version. The problem is when calling through a pointer, or a virtual function, or a DSO (assuming that that can't add extra shadow symbols). As for performance, you could have both functions calling a third, hidden function with a flag saying which ABI is in use.

[SebastienBini]

To be honest, I don't see what the real danger is here. I get that this behavior is not ideal, but I don't see it as a blocking issue. I doubt developers are really concerned on when copy-only types are destroyed, as their destructors rarely have desirable side effects.

Yes, but they may release resources, e.g. a refcnt'ed database cursor. (Or, worse, an auto-commit transaction object - though that would be pretty bad code.) OTOH since this only affects weird/old classes, and slightly odd code, and only certain platforms and only if the implementer and user have chosen the path of full backwards compatibility, I think it's enough to call this out in discussion; I'll get writing it up.

+1. And compilers will emit warnings, and I hope people compile with Werror.

It's not so bad, if you imagine that most of the time you're calling the eliding version. The problem is when calling through a pointer, or a virtual function, or a DSO (assuming that that can't add extra shadow symbols).

Indeed.

As for performance, you could have both functions calling a third, hidden function with a flag saying which ABI is in use.

Hmm, what do you have in mind? I don't see the performance benefits it brings.

[ecatmur]

The hidden function would take a flag to say whether the (non-reloc-only prvalue) parameters are passed caller- or callee-destroy. At each (discarded or non-discarded) reloc it would consult the flag and, if caller-destroy, would move/copy a temporary, or if callee-destroy, would mark that parameter destroyed. On exit, if callee-destroy, it would destroy all those parameters not already marked as destroyed. So this is a couple of flag checks / conditional jumps to save the cost of creating possibly unnecessary temporaries.

[SebastienBini]

Oh ok. But I take we know at compile-time whether the ABI is callee-destroy or caller-destroy, so why not generate two function bodies with the different reloc semantics? Do you foresee it would explode code size?

Also, I don't see the problem this is solving. If we emit two functions, an eliding one and non-eliding one, then we get the benefit that reloc will only be used in callee-destroy convention, which simplifies the rules and gives stronger guarantees on destruction time. I don't see this benefit in your solution, unless I am missing something?

Maybe is it time to involve the ABI people :o ?

[ecatmur]

Yes, it would potentially be bloaty, also the two function bodies would need to coordinate to share nested statics and types. So we can expect the optimizer to merge the common sections anyway.

Let's step back so we're talking about the same thing. We've on Itanium and we have a class A that has caller-destroy ABI. We have a function f calling a function g (we assume that g is called by pointer, vtable, DSO etc, so it has to be called with caller-destroy ABI and cannot be inlined):

void f(A a, bool b) { if (b) g(reloc a); h(); }

For purposes of ABI we need to emit a function:

void f.classic(A* a, bool b) {  // callee-destroy
    if (b) {
        A __tmp(static_cast<A&&>(*a));
        g.classic(&__tmp);  // callee-destroy
    }
    h();
}

For efficiency we also want to emit:

void f.eliding(A* a, bool b) {  // caller-destroy
    bool __a_destroyed = false;
    if (b) {
        g.classic(a);  // callee-destroy
        a->~A();
        __a_destroyed = true;
    }
    h();
    if (not __a_destroyed)
        a->~A();
}

To avoid bloat (also for local types, statics etc.), we could have f.classic call f.eliding:

void f.classic(A* a, bool b) {
    union { A __tmp } = { .tmp = A(static_cast<A&&>(*a))};
    f.eliding(&__tmp, b);
}

But this is inefficient if b is false, so instead I would generate an internal function that both call:

void f.classic(A* a, bool b) {
    f.internal(false, a, b);
}
void f.eliding(A* a, bool b) {
    f.internal(true, a, b);
}
void f.internal(bool __eliding, A* a, bool b) {
    bool __a_destroyed = false;
    if (b) {
        if (__eliding) {
            g.classic(a);
            a->~A();
            __a_destroyed = true;
        } else {
            A __tmp(static_cast<A&&>(*a));
            g.classic(&__tmp);
        }
    }
    h();
    if (__eliding and not __a_destroyed)
        a->~A();
}

[SebastienBini]

Yes, I get what you mean.

we could have f.classic call f.eliding:

void f.classic(A* a, bool b) {
    union { A __tmp } = { .tmp = A(static_cast<A&&>(*a))};
    f.eliding(&__tmp, b);
}

It might be inefficient, but if f.classic calls f.eliding then we know that a is destroyed at the end of the g(reloc a); expression evaluation. What I mean is if the rule for the classic version is to always call the eliding one, then we have the guarantee that reloc expressions will never be evaluated with unowned parameters, and in particular, we no longer need to move or copy construct temporaries and defer destructor calls for value parameters.

Having said that, I don't know if it's really worth it. Copy-only types rarely have a destructor with desirable side-effects. If they have one, then the side effect will likely get triggered when a ref-count reaches 0.
Consider this:

void foo()
{
  A a;
  f(reloc a, true); /* calls f.classic, and under the hypothesis that 
    it would call the eliding version.
    In particular, because f is caller-destroy, `a` still lives until f returns, 
    and its destructor is called
    at the end of the full-expression evaluation. */
  k();
}
void f.classic(A* a, bool b) {
  union { A __tmp } = { .tmp = A(static_cast<A&&>(*a))};
  f.eliding(&__tmp, b);
}
void f.eliding(A* a, bool b) {
    bool __a_destroyed = false;
    if (b) {
        g.classic(a);
        a->~A(); /* we are guaranteed that the destructor
         is called after the reloc call...
         however if ref-counted, despite what users might expect,
         the ref count won't reach 0 as foo still holds a reference.
       */
        __a_destroyed = true;
    }
    h();
    if (not __a_destroyed)
        a->~A();
}

In conclusion, I don't think we need to fiddle with the ABI at this point. Stating that users must have no expectations on when the lifetime of value parameters ends seems the best approach.

[ecatmur]

Ah, of course - if there's a question over whether reloc x; will be a (copy-and-destroy) no-op (other than removing the name from scope), that's because x is a by-value function parameter, which means that if the destructor does something useful despite it being copyable, it's probably refcounted, but the caller holds a refcount, so it won't reach zero anyway!

Thanks for working through this with me, I think there's some valuable stuff to write into the proposal. I'll get on it.