Fix issue 20235 - caller destroys arguments on POSIX

[SSoulaimane]

On Windows arguments are destroyed in the callee regardless of which calling convention is used (cdecl, stdcall... etc), 32bits and 64bits alike.

On the other platforms, the caller destroys the arguments if the caller cleans the stack.

[kinke]

Excellent, thanks in advance, another big hurdle for proper C++ interop to be moved out of the way! - Just wanted to say that I really appreciate your work on D and that I find it impressive, given that you're mostly working on pretty hard stuff and have started not that long ago to contribute. It's a pity that your other big ABI PRs (#10200, #10280) are still waiting; I hope it doesn't affect your motivation - things can take time around here. ;)

[SSoulaimane]

My pleasure. Thanks for your motivational words. I'm really just learning and appreciating what you guys have been doing here for so long, this what's really impressive to me. Thank you and thanks to Walter for D and for open sourcing this great piece of engineering which is DMD.

[dlang-bot]

Thanks for your pull request, @SSoulaimane!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20235	normal	C++ ABI doesn't destruct struct arguments in the callee

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10593"

[ibuclaw]

Apart from the divergence in error messages, can't see anything of concern here.

[ibuclaw]

Hmm, I don't think it is particularly good if there's a divergence on this error between posix and non-posix. Consistency should really be upholded, otherwise it'll come back to bite. Particularly as the error message itself is a bit cryptic.

[SSoulaimane]

Agreed. I'm not sure what to do about it.

[ibuclaw]

As the struct has a ~this() function, it is non-pod, therefore going to be passed by reference anyway. So I wonder whether this field is strictly necessary. Perhaps just a check for if (struct && !isPOD) would suffice when it comes to the backend handling in e2ir.

I'm not really that bothered either way.

[SSoulaimane]

The reason there is a check is that if a struct doesn't have the copy constructor the backend does the copying. I find this way of putting a flag on a variable to be passed by ref kludgy and I would prefer a uniform way of copying value arguments (at least non-POD) either the frontend always copies or always leaves it to backend.

[ibuclaw]

If the struct is non-POD, then the backend should never copy it.

[ibuclaw]

There is tmp.type.needsDestruction(). You should be able to replace v.isArgDtorVar with !target.isCalleeDestroyingArgs(tf) && arg.type.needsDestruction()

[SSoulaimane]

If the front-end always copies I guess on windows 32 you're going to find some way to strip off the temporary or place it directly the stack? like in #10510?

[SSoulaimane]

Or since we mixed-in target information in the front-end already, should we check whether the target accepts value arguments by ref then copy? I'm not really satisfied with either solutions tbh.

[ibuclaw]

My own preference (to which I'm happy to admit might be wrong or ill-informed) would be to have an agnostic front-end representation, then let the back-end do generation that both adheres to the target ABI requirements, while respecting the front-end semantics of the operation.

There are definitely many operations that the back-end can do better than what the front-end can generate (or lower) code for, especially around structs and ctor/cpctor/dtor semantics (for instance, GCC has a dedicated tree node - TARGET_EXPR - specifically made for managing temporaries which are initialized from, or destructed by, non-POD [cd]tors, including all EH handling. That the front-end is instead creating its own temporaries and try/finally statements defeats the ability to effectively utilize this). My vision would be to encapsulate such operations in an Expression (existing examples of similar constructs would be: ArrayLengthExp, DelegatePtrExp, DelegateFuncptrExp), then let the back-end do all the lowering, instead of the front-end.

I wouldn't hold my breath on when or if I will ever get time to write a POC implementing the above idea though.

[WalterBright]

As the struct has a ~this() function, it is non-pod, therefore going to be passed by reference anyway.

This could explain a big chunk of the performance difference between DMD and GDC/LDC.

[ibuclaw]

This could explain a big chunk of the performance difference between DMD and GDC/LDC.

!!!

I appear to have slightly misremembered, so I'll correct myself and say that it's only postblits, cpctors and dtors.

/* For structs with a user defined postblit, copy constructor, or a
   destructor, also set TREE_ADDRESSABLE on the type and all variants.
   This will make the struct be passed around by reference.  */
if (t->sym->postblit || t->sym->hasCopyCtor || t->sym->dtor)
  {
    // ...
  }

Wouldn't you need to pass structs by reference anyway to be compatible with C++?

[SSoulaimane]

@ibuclaw I can hardly disagree with your rationale, but I think for better communication there should be a group for discussing work that matters to all backends to find solutions that work for everyone, since any work done on one backend will drag the rest of them willingly or not.

[thewilsonator]

Ping, conflicts resolved.

[thewilsonator]

Hmm, this segfaults Mir's RCPtr, cc @9il

[9il]

The failing test:

///
version(mir_test)
@safe pure @nogc nothrow
unittest
{
    import core.lifetime: move;
    struct S
    {
        double e;
    }
    struct C
    {
        int i;
        S s;
    }

    auto a = createSlimRC!C(10, S(3));
    auto s = a.move.toRCPtr.shareMember!"s";
    assert(s._counter == 1);
    assert(s.e == 3);
}

called functions:

/++
Returns: shared pointer constructed from the slim shared pointer.

The function has zero computation cost.
+/
RCPtr!F toRCPtr(F)(return SlimRCPtr!F contextAndValue) @trusted
{
    typeof(return) ret;
    ret._value = contextAndValue._value;
    ret._context = &contextAndValue.context();
    contextAndValue._value = null;
    return ret;
}

/++
Returns: shared pointer of the member and the context from the current pointer. 
+/
auto shareMember(string member, T, Args...)(return mir_rcptr!T context, auto ref Args args)
{
    import core.lifetime: move;
    void foo(A)(auto ref A) {}
    static if (args.length)
    {
        // breaks safaty
        if (false) foo(__traits(getMember, context._get_value, member)(forward!args));
        return (()@trusted => createRCWithContext(__traits(getMember, context._get_value, member)(forward!args), context.move))();
    }
    else
    {
        // breaks safaty
        if (false) foo(__traits(getMember, context._get_value, member));
        return (()@trusted => createRCWithContext(__traits(getMember, context._get_value, member), context.move))();
    }
}

/++
Returns: shared pointer constructed with current context. 
+/
@system .mir_rcptr!R createRCWithContext(R, F)(return R value, return const mir_rcptr!F context)
    if (is(R == class) || is(R == interface))
{
    typeof(return) ret;
    ret._value = cast()value;
    ret._context = cast(mir_rc_context*)context._context;
    (*cast(mir_rcptr!F*)&context)._value = null;
    (*cast(mir_rcptr!F*)&context)._context = null;
    return ret;
}

///ditto
@system .mir_rcptr!R createRCWithContext(R, F)(return ref R value, return const mir_rcptr!F context)
    if (!is(R == class) && !is(R == interface))
{
    typeof(return) ret;
    ret._value = &value;
    ret._context = cast(mir_rc_context*)context._context;
    (*cast(mir_rcptr!F*)&context)._value = null;
    (*cast(mir_rcptr!F*)&context)._context = null;
    return ret;
}

[9il]

For some reason, shareMember gets a null value argument.

FYI: Mir uses postblit constructors this(this) instead of copy constructors because the last are buggy.

[9il]

So the bug likely appears for move or toRCPtr

[9il]

The unittest tests three kinds of move functions move from core., toRCPtr, and shareMember. It is quite straightforward in terms data ownership. The rc counter should always be equal to 1.

[9il]

Ah! Maybe it is because of a function evaluation order.

createRCWithContext(__traits(getMember, context._get_value, member), context.move)) 

Mir uses the spec rule that arguments are evaluated from left to right.

[thewilsonator]

Thanks for the case reduction.

[12345swordy]

Can you fix the error that 9il has brought up @SSoulaimane ?
cc @thewilsonator

[9il]

Can you fix the error that 9il has brought up @SSoulaimane ?

Looks like @SSoulaimane is offline since NY.

[12345swordy]

Which line of code is the culprit here?

[9il]

Which line of code is the culprit here?

Returns: shared pointer of the member and the context from the current pointer. 
+/
auto shareMember(string member, T, Args...)(return mir_rcptr!T context, auto ref Args args)
{
    import core.lifetime: move;
    void foo(A)(auto ref A) {}
    static if (args.length)
    {
        // breaks safaty
        if (false) foo(__traits(getMember, context._get_value, member)(forward!args));
        return (()@trusted => createRCWithContext(__traits(getMember, context._get_value, member)(forward!args), context.move))();
    }
    else
    {
        // breaks safaty
        if (false) foo(__traits(getMember, context._get_value, member));
        return (()@trusted => createRCWithContext(__traits(getMember, context._get_value, member), context.move))();
    }
}

[9il]

But I have no idea where is the bug in the compiler

[12345swordy]

Excuse me, I was referring to this PR in question here.

[MoonlightSentinel]

The failure in runnable/testassert.d suggests that some destructors are not run if an exception is thrown.

[Geod24]

@WalterBright : Any chance you could take a look at this ? This issue makes it impossible to pass anything by value, e.g. smart pointers. In our case, we're using a C++ library which just happens to do this in the newest version, preventing us from updating. We're going to work around it by moving the problematic overrides to C++ for the time being, but it's a major hurdle when dealing with C++.

[12345swordy]

We have no way of contacting op?

[WalterBright]

Huh, I followed the "Use the command line to resolve conflicts before continuing" exactly and the changes do not show up here. Where did they go?

[thewilsonator]

I usually use

git checkout callerdestroy
git rebase master
#resolve merge conflicts
git push [-f] SSoulaimane callerdestroy

Forcing the push after making sure its going to the right repo.

The instructions seem to be telling you to push the change to a branch on your GH repo, rather than OPs.
instead of git push origin master try git push SSoulaimane callerdestroy (or git push SSoulaimane/callerdestroy I can never remember which), check its going to the right place and then repeat the command with -f.

[WalterBright]

I created a new PR instead #12012

I still don't know where my rebase went following the instructions.

[WalterBright]

Please follow up to #12012