fix bugzilla Issue 24368 - destruction of parameter should be done by…

[WalterBright]

… caller

This is a risky change!

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	24368	normal	destruction of parameter should be done by caller

Testing this PR locally

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

dub run digger -- build "master + dmd#16145"

[thewilsonator]

I feel like this could do with a changelog entry

[WalterBright]

It could, indeed. First I have to get it to work.

[ibuclaw]

I'm on the fence of just calling this wontfix. What's the reason for mimicking C++ here when the D method is perfectly acceptable?

[ibuclaw]

I'd limit this to just tf.linkage == LINK.d, it seems odd to be potentially calling a destructor on a parameter passed from a C/ObjC program.

Suggested change

	return os == Target.OS.Windows ||
	// C++ on non-Windows platforms has the caller destroying the arguments
	tf.linkage != LINK.cpp;
	return os == Target.OS.Windows ||
	// C++ on non-Windows platforms has the caller destroying the arguments
	tf.linkage == LINK.d;

[WalterBright]

What's the reason for mimicking C++ here when the D method is perfectly acceptable?

1. C++ ABI needs to be fixed
2. D ABI has far more tests than the C++ ABI, making it work for D makes it more reliable
3. fewer special cases in the code
4. You mentioned once that gdc and ldc already did this for D ABI
5. You pointed out a problem with the dmd D ABI and and dmd's stack alignment implementation that didn't happen with gdc, because gdc follows the C++ ABI
6. destructors for variadic function calls don't get called (I didn't actually check this)

[ibuclaw]

What's the reason for mimicking C++ here when the D method is perfectly acceptable?

1. C++ ABI needs to be fixed
2. D ABI has far more tests than the C++ ABI, making it work for D makes it more reliable

In what way does it need to be fixed? There's no examples where C++/D interop is broken because extern(D) code does something different.

3. fewer special cases in the code

extern(C++) is what has necessary special cases for ABI compatibility for specific targets, extern(D) has no special case as it's all consistent.

4. You mentioned once that gdc and ldc already did this for D ABI

Maybe that was something else? Both gdc and ldc do callee for extern(D) - same as dmd.

5. You pointed out a problem with the dmd D ABI and and dmd's stack alignment implementation that didn't happen with gdc, because gdc follows the C++ ABI

Stack frames/closures? That's a separate thing - or are you saying that you have an example where a closure variable is (somehow) passed by reference instead of copied and has its destructor called by the callee?

Reflecting some more, maybe you're referring to how non-trivial types are passed around? In gdc it's always by reference (so the front-end better do the copy on the caller side else we're in trouble).

6. destructors for variadic function calls don't get called (I didn't actually check this)

That's illegal.

error: cannot pass types that need destruction as variadic arguments

[ibuclaw]

At best, this is an optimization, consider:

struct Dtor
{
    int field;
    ~this() { }
}

void callee(Dtor param)
{
    // __dtor(param)
}

void caller(Dtor param)
{
    callee(param);  // __copy(param)
    // __dtor(param)
}

void main()
{
    Dtor var;
    caller(var); // __copy(var)
   // __dtor(var)
}

That is an awful lot of copying and destructoring.

If instead we were doing caller destruction.

struct Dtor
{
    int field;
    ~this() { }
}

void callee(Dtor param)
{
}

void caller(Dtor param)
{
    callee(param);  // ref
}

void main()
{
    Dtor var;
    caller(var); // ref
   // __dtor(var)
}

No copying, one destructor call.

[kinke]

Nope, this doesn't affect the number of copies/destructions (since lvalues need to be copied implicitly, and the low-level ref is to that caller-allocated copy, not to the original lvalue).

AFAICT, the only difference is where the cleanup ends up in, in pseudo-code either

• in the caller: try { callee(auto tmp = param, tmp); } finally { tmp.~this(); }; no destruction of param in callee
• in the callee: void callee(Dtor param) { scope(exit) param.~this(); ... }; no destruction of tmp temporary in caller

Doing this in the callee seems more intuitive to me - it's a local by-value param of callee, so naturally destroyed by the callee. And I suspect having the cleanup/finally in the callee might enable better optimizations, as it can be optimized along with the function body, guaranteed in the same translation unit.

[ibuclaw]

Doing this in the callee seems more intuitive to me - it's a local by-value param of callee, so naturally destroyed by the callee. And I suspect having the cleanup/finally in the callee might enable better optimizations, as it can be optimized along with the function body, guaranteed in the same translation unit.

You can't pass a non-trivially copyable type by-value, as that would require a temporary. The front-end then must make an explicit copy (temporary) before passing it.

[kinke]

Sure, allocation + copying (in the lvalue argument case, otherwise just passing a ref to the rvalue for perfect forwarding without moving) needs to be performed by the caller, always. But IMO that doesn't make it more natural to handle its destruction there too. But more importantly, EH overhead can be avoided by letting the callee destruct it; e.g., it might not throw, without explicit nothrow attribute.

[WalterBright]

The testing issue is important. We have a lot of tests in the D test suite to verify that the destructors are working properly. We have pretty much none for C++ destructor semantics. By making the D destructor ABI the same as the C++ destructor ABI, we can have confidence that both are implemented correctly.

EH overhead can be avoided by letting the callee destruct it; e.g., it might not throw, without explicit nothrow attribute.

I hadn't thought of that. A good point. @ibuclaw is there someone you can ask why gdc does caller destruction?

[kinke]

Well one argument against callee destruction is the following example with 2 temporaries:

void callee(Dtor a, Dtor b) {}
void caller() { callee(makeDtor(), makeDtor()); }

What we do is create 2 temporaries, and if makeDtor() may throw, the caller still needs to destruct the first temporary in case the 2nd makeDtor() threw (but with callee-destruction we can elide a cleanup for the 2nd temporary, as it's the last argument expression that may throw, so if constructed successfully, callee is definitely called and will destruct it). But not if callee was indeed called, then ownership (incl. duty to destruct) was transferred to the callee. So CallExpressions are complex; we work with a gate variable in edtor to gate the destruction of these temporaries ('skip if callee was called') etc. So in the worst case, we have an EH cleanup for the 1st temporary in the caller, and another EH cleanup in callee for the same temporary.

[ibuclaw]

@ibuclaw is there someone you can ask why gdc does caller destruction?

It isn't. Maybe the example was confusing by using the same name everywhere.

[WalterBright]

g++ certainly does caller destruction.

[ibuclaw]

g++ certainly does caller destruction.

Yes, and that is supported by this target hook.

Did you mean for me to find out why g++ does caller destruction? I can certainly have a dig around and see what I can come up with.

[ibuclaw]

The obvious/benign reason is that it's just easier to represent lifetimes if the caller does destruction.

Consider the following transformation sequence:

// User code
fn(obj);

// Compiler generates AST for call expr.
// There's a temporary with a cleanup, but no information about when the cleanup should run.
fn(TEMP[temp = obj, dtor(temp)]);

// Compiler generates AST for exp statement
// Now there is a cleanup point.
CLEANUP[fn(TEMP[temp = obj, dtor(temp)])];

// Compiler lowers AST to SSA form.
try
{
    temp = {BEGIN};
    try
    {
        temp = obj;
        fn(&temp);
    }
    finally
    {
        dtor(&temp);
    }
}
finally
{
    temp = {END};
}

[WalterBright]

Did you mean for me to find out why g++ does caller destruction?

Yes, I think that would be valuable information.

[WalterBright]

Thanks for the example. I think it does make the code easier to reason about, and as @kinke mentioned, there wouldn't be the complexity of what if a following argument threw an exception and the callee was never called, then the caller would still have to do the destruction.

[WalterBright]

Getting this mysterious error:

std\file.d(5039): Error: `@safe` function `std.file.__unittest_L5020_C7.main` cannot call `@system` function `std.file.__unittest_L5020_C7.listdir`
std\algorithm\iteration.d(1311):        which calls `std.file.__unittest_L5020_C7.listdir.filter!(_DirIterator!true).filter`
std\algorithm\iteration.d(1313):        which wasn't inferred `@safe` because of:
std\algorithm\iteration.d(1313):        scope variable `__pfx2089` may not be returned
std\file.d(5022):        `std.file.__unittest_L5020_C7.listdir` is declared here

__pfx is declared in expressionsem.d and goes down a different path when needsDtor is true.

https://github.com/dlang/dmd/blame/master/compiler/src/dmd/expressionsem.d#L3498

[WalterBright]

@dkorpel do you have any insight on this problem?

[dkorpel]

I don't know what causes the error yet, but with the help of dustmite I made this reduced test case:

@safe:

SafeRefCounted map(return scope SafeRefCounted r)
{
    return r;
}

struct SafeRefCounted
{
    ~this() scope { }
    int* p;
}

SafeRefCounted listdir()
{
    return SafeRefCounted().map;
}

[WalterBright]

Wow, nice work, @dkorpel

[WalterBright]

@dkorpel your reduced test case made for an easy fix. Thanks!

[WalterBright]

The latest problem is another __pfx one:

__pfx10801 = ((ref Probe[1] a = __pfx10800;) , a)

which is triggering:

Trying reference _d_arrayctor, this should not happen!

in e2ir.d. Apparently the comma-expression is causing the failure.

The trigger is compiling phobos/std/typecons.d:

~/forks/dmd/generated/linux/release/64/dmd -conf=/home/walter/forks/dmd/compiler/src/bug2/dmd.conf -I/home/walter/forks/dmd/druntime/import -w -de -preview=dip1000 -preview=dtorfields -preview=fieldwise -m64 -fPIC -unittest -version=StdUnittest -c std/typecons.d

The __pfx variables are only generated in expressionsem.d for destructor handling.

[WalterBright]

Trying dustmite on it ...

[WalterBright]

The miracle of dustmite reduces it to:

struct Probe
{
    ~this() { }
}

void test()
{
    staticArrayX([Probe()]);
}

pragma(inline, true)
Probe[1] staticArrayX(Probe[1] a)
{
    return a;
}

[WalterBright]

D:/a/1/s/generated/Windows/RelWithAsserts/Win32/dmd.exe -m32  -w -I../../src -I../../import -Isrc -defaultlib= -preview=dip1000  -LD:/a/1/s/generated/windows/release/32/druntime.lib -O -release -profile=gc -of./generated/windows/release/32/profilegc src/profilegc.d
****** FAIL debug32 core.internal.container.array
core.exception.AssertError@src\core\internal\container\array.d(200): 0 != 1

[WalterBright]

Another new one:

std/algorithm/iteration.d(2215): Error: `@safe` function `std.algorithm.iteration.__unittest_L2208_C7` cannot call `@system` function `std.algorithm.iteration.__unittest_L2208_C7.map!(ChunkByImpl!(__lambda2, __lambda3, GroupingOpType.unary, int[])).map`
--
  | std/algorithm/iteration.d(479):        which wasn't inferred `@safe` because of:
  | std/algorithm/iteration.d(479):        scope variable `__pfx2180 = r` calling non-scope member function `ChunkByImpl.__postblit()`
  | std/algorithm/iteration.d(446):        `std.algorithm.iteration.__unittest_L2208_C7.map!(ChunkByImpl!(__lambda2, __lambda3, GroupingOpType.unary, int[])).map` is declared here

[WalterBright]

Blocked by dlang/phobos#8909