extern(C++) destructor uses wrong ABI on x86 in derived class

[tim-dlang]

import std.stdio;
import core.stdcpp.new_;

extern(C++) class A
{
    int i;
    ~this(){}
}

class B : A
{
    ~this()
    {
        writeln("B.~this ", i);
    }
}

void main()
{
    A obj = cpp_new!B();
    obj.i = 42;
    cpp_delete(obj);
}

Compiling and running the above code using ldc2 -L-lstdc++ -m32 -run test.d prints a wrong result like B.~this -401281917. The same happens when using obj.__dtor(), obj.__xdtor() or obj.destroy().

It only happens, when the base class is extern(C++) and the derived class is extern(D), so a workaround is to also declare class B as extern(C++).

Another workaround is cast the destructor to the used ABI, like (cast(void delegate())&obj.__xdtor)(), but that only works when the used ABI is known.

DMD prints the correct value, so it seems to be a bug in the LDC backend.

[kinke]

Hmm, so this looks like you're overriding a C++ dtor with an extern(D) one - I didn't expect that to work, since the semantics are quite different (e.g., D dtors don't call the base dtors). The opposite case is handled, specifically wrt. ABI differences (edit: wrt. the this pointer) on 32-bit x86, creating an extern(D) dtor shim for extern(C++) dtors (global.params.wrapDtorInExternD), for the D TypeInfo IIRC.

[tim-dlang]

The use case is that the base class is used from C++ and the subclass is defined in D and can have additional methods, which would not be supported by C++.

The destructor could always be extern(C++) internally if the root class is extern(C++).

[kinke]

The destructor could always be extern(C++) internally if the root class is extern(C++).

If it makes it into the vtbl, then it might have to - but probably explicitly, i.e., disallowing (explicit or implicit) extern(D) for such an override. extern(D) ctors can be added to extern(C++) classes, with their own mangling and not stepping on other toes, but there's also no vtbl complication for ctors.

[kinke]

What we could presumably also do is generate dtor shims for extern(C++) classes in general - one that calls the actual dtor (the aggregate dtor IIRC, the one destructing the fields too), and then calls the base dtor - what C++ would do. This would fix that incompatibility (missing base-class destruction when relying on virtual C++ dtors instead of using rt_finalize2(), which calls the dtors in a loop, without the need for a vtbl entry for the D dtors).

Edit: Class B in your snippet is an extern(C++) class too (inherited from the base class A - it cannot magically be a proper D class at the same time); the (implicit) extern(D) AFAICT solely affects the default linkage of the function members.

[kinke]

Oh wait, the missing base-class destruction seems to be fixed nowadays, handled by the generated __aggrDtor (not just additionally destructing the fields, but also calling the base dtor for extern(C++) classes apparently). And after looking at the IR, the problem seems to be that this __aggrDtor (with unconditional extern(D) linkage) is put into the C++ vtbl dtor slots. So if it works with an extern(C++) B.~this(), that's probably just a fluke, since it's not the vtbl entry directly.

[tim-dlang]

Oh wait, the missing base-class destruction seems to be fixed nowadays, handled by the generated __aggrDtor (not just additionally destructing the fields, but also calling the base dtor for extern(C++) classes apparently).

Yes, the generated __aggrDtor of B calls the base class __aggrDtor:

ldc/dmd/clone.d

Lines 1018 to 1021 in 1218a47

	// extern(C++) destructors call into super to destruct the full hierarchy
	ClassDeclaration cldec = ad.isClassDeclaration();
	if (cldec && cldec.classKind == ClassKind.cpp && cldec.baseClass && cldec.baseClass.aggrDtor)
	dtors.push(cldec.baseClass.aggrDtor);

And after looking at the IR, the problem seems to be that this __aggrDtor (with unconditional extern(D) linkage) is put into the C++ vtbl dtor slots. So if it works with an extern(C++) B.~this(), that's probably just a fluke, since it's not the vtbl entry directly.

The vtbl entries look differently if B is declared with extern(C++). With implicit extern(D) the vtbl is (from objdump -r):

RELOCATION RECORDS FOR [.data.rel.ro._D11testldcdtor1B6__vtblZ]:
OFFSET   TYPE              VALUE
00000000 R_386_32          _D11testldcdtor1B10__aggrDtorMFZv
00000004 R_386_32          _D11testldcdtor1B10__aggrDtorMFZv

With extern(C++) for B it is:

RELOCATION RECORDS FOR [.data.rel.ro._D11testldcdtor1B6__vtblZ]:
OFFSET   TYPE              VALUE
00000000 R_386_32          _ZN1BD1Ev
00000004 R_386_32          _ZN1BD1Ev

In the second case it also builds the wrapper _D11testldcdtor1B11__ticppdtorMFZv as you previously said.
I have seen in the frontend, that sometimes dtor._linkage == LINK.cpp is used and sometimes cldec.classKind == ClassKind.cpp. Maybe the frontend should set _linkage for the destructor to LINK.cpp if classKind is ClassKind.cpp.

[kinke]

Yeah exactly, something like this might work (untested):

diff --git a/dmd/clone.d b/dmd/clone.d
index 610dd756ec..d3e5855ab0 100644
--- a/dmd/clone.d
+++ b/dmd/clone.d
@@ -1021,6 +1021,11 @@ void buildDtors(AggregateDeclaration ad, Scope* sc)
             dtors.push(cldec.baseClass.aggrDtor);
     }
 
+    bool landsInCppVtbl;
+    if (auto cldec = ad.isClassDeclaration())
+        if (cldec.classKind == ClassKind.cpp)
+            landsInCppVtbl = true;
+
     // Set/build `ad.aggrDtor`
     switch (dtors.length)
     {
@@ -1030,6 +1035,8 @@ void buildDtors(AggregateDeclaration ad, Scope* sc)
     case 1:
         // Use the single existing dtor directly as aggregate dtor.
         // Note that this might be `cldec.baseClass.aggrDtor`.
+        if (landsInCppVtbl && dtors[0]._linkage != LINK.cpp)
+            goto default;
         ad.aggrDtor = dtors[0];
         break;
 
@@ -1056,6 +1063,8 @@ void buildDtors(AggregateDeclaration ad, Scope* sc)
         auto dd = new DtorDeclaration(declLoc, Loc.initial, stc, Id.__aggrDtor);
         dd.isGenerated = true;
         dd.storage_class |= STC.inference;
+        if (landsInCppVtbl)
+            dd.linkage = LINK.cpp;
         dd.fbody = new ExpStatement(loc, e);
         ad.members.push(dd);
         dd.dsymbolSemantic(sc);

Edit: And yep, the _ZN1BD1Ev is the __aggrDtor with extern(C++) linkage (inherited from the class).

[tim-dlang]

I have found an example, where the problem can also be seen with DMD: dlang/dmd#23605

Here is a PR for DMD: dlang/dmd#23606
I had to set the linkage in the semantic run, because it would otherwise be overwritten there.

[kinke]

Nice, I expected DMD wouldn't handle this in its backend either and only accidentally happen to work for the original test case. So I guess this issue here is safe to close.