On Wed, Dec 21, 2022 at 9:19 PM Edward Catmur <ecatmur@googlemail.com> wrote:

We could simply resolve this issue by stating that no relocation constructor will be added to lock_guard,

Yes, I think that in this specific case it can be argued that std::lock_guard should not be made relocatable, since at present one knows that if one passes a prvalue std::lock_guard to a function, the mutex is necessarily unlocked at the time the function returns, which would no longer be the case if it were relocatable. (Even if one doesn't know whether the mutex is unlocked at the point of returning or at the end of the containing expression, the caller can ensure those two points are the same.)

That's a compelling argument not to make lock_guard relocate-only. I guess we could be satisfied by simply adding a relocation ctor to unique_lock. I feel we have so many guards already, that adding a new one just to be relocate-only, while having the same purposes of unique_lock would be a bit overkill.

That makes me wonder what would happen in the following code (I don't recall we ever took a definitive decision on that point):

void do_something(std::lock_guard<std::mutex> guard)
{
    if (!some_test())
    {
        reloc guard; /* attempt to release the lock early
            as the log function doesn't need it  */
        log("thread " << std::this_thread::get_id() << " failed");
        return;
    }
    bar();
}

If the ABI is caller-destroy then `guard` destructor will forcibly be called at function exit.

What would happen if `guard` were a non-function-parameter local variable, or the ABI callee-destroy? In other terms, should `reloc` offer any guarantee on when the destructor is called?

If we want to keep `reloc` consistent in all situations, then `reloc x;` should never call the destructor of x (which will be destroyed normally at its end of scope). If this approach is taken then everyone is dragged down because of that ABI issue which only some have and that may be resolved in the future.

This is a missed opportunity in my opinion. `reloc x;` should, when possible, call the destructor right away. That would allow developers to preemptively call the destructor of an object, without wrapping the object in an optional (or use unique_lock in our case). The language will keep track of the destruction state for us (especially is used in conditional branches), and this would no longer be a burden for the developer (which means less bugs).

I believe that `reloc src`:

• if `src` is a local object and not a function parameter, then `src` must be left in a destructed state (either because it was passed to a relocation constructor or by a direct call to its destructor) at the end of the expression evaluation.
• otherwise (`src` is a function parameter passed by value) then when `src` is destroyed is implementation-defined. Typically, as soon as the ABI permits.

In the last case, if a `reloc src;` statement is used and the ABI does not allow to call the destructor right-away, then compilers can still emit a warning.

but that does not fix more pernicious cases, like functions with signature: void foo(non_null<unique_ptr<int>>); non_null and unique_ptr will get a relocation ctor, and then non_null<unique_ptr> will be automatically relocate-only, potentially changing the ABI of foo.

I don't know to what extent we can simply ditch those cases by saying they are bad code (really, who would pass a non_null<unique_ptr> by value today?), and the ABI break is the price to pay for writing such bad code in the first place. Maybe an ABI break on such functions is acceptable. I doubt any major libraries provide such APIs?

Right, there's no reason to have a parameter of type non_null<unique_ptr<T>>, since it could just be T&.

Still, this does mean that such ABI breakage does need to be detectable; I think it might be necessary to propose different mangling for callee-destroy parameters. One could even have a scheme where up to two symbols are emitted, where the old (caller-destroy) symbol is emitted only if the function does not in actual fact relocate from its parameters, in which case the new (callee-destroy) symbol forwards to the old and then destructs its relocatable parameters on exit. (I think there was a similar proposal - not mine - in the original thread.)

 Sure, that indeed rings a bell. That would only help in detecting ABI incompatibilities (as link errors instead of silent crashes), or am I missing something? But good point!