Translation is incorrect for destructors on Microsoft ABI

[PathogenDavid]

Found this while investigating an issue with calling the destructor of PxDefaultFileOutputStream with Mochi.PhysX. (Ironically I apparently knew about this when I filed https://github.com/MochiLibraries/Biohazrd/issues/210 but never put 2 and 2 together.)

The ABI of (virtual?) destructors should include an int parameter that determines whether the destructor will call operator delete or not. This isn't being emitted so it's receiving whatever garbage is in edx. Additionally this is making it impossible to properly call the destructor for stack-allocator objects (which obviously don't need to be deleted.)

Godbolt for the below code.

Destructors in the Microsoft ABI

Consider the type below:

class Hello
{
public:
    Hello()
    {
        puts("Hello");
    }

    virtual ~Hello()
    {
        puts("Goodbye");
    }
};

Currently Biohazrd exposes this constructor as delegate* unmanaged<Hello*, void>, but as mentioned above it's actually delegate* unmanaged<Hello*, int, void>.

Expressed as a C function, it would be something like void DestroyHello(Hello* _this, int shouldDelete);

(Technically it returns this too. Although I'm not really sure why that's useful considering that pointer is now invalid.)

The implementation of this destructor doesn't show up on Godbolt, so here it is for PxDefaultFileOutputStream:

Mochi.PhysX.Native.dll!physx::PxDefaultFileOutputStream::`scalar deleting destructor'(unsigned int):
00007FFBCC20D2A0 89 54 24 10          mov         dword ptr [rsp+10h],edx  
00007FFBCC20D2A4 48 89 4C 24 08       mov         qword ptr [rsp+8],rcx  
00007FFBCC20D2A9 48 83 EC 28          sub         rsp,28h  
00007FFBCC20D2AD 48 8B 4C 24 30       mov         rcx,qword ptr [this]  
00007FFBCC20D2B2 E8 59 FB FF FF       call        physx::PxDefaultFileOutputStream::~PxDefaultFileOutputStream (07FFBCC20CE10h)  
00007FFBCC20D2B7 8B 44 24 38          mov         eax,dword ptr [rsp+38h]  
00007FFBCC20D2BB 83 E0 01             and         eax,1  
00007FFBCC20D2BE 85 C0                test        eax,eax  
00007FFBCC20D2C0 74 0F                je          physx::PxDefaultFileOutputStream::`scalar deleting destructor'+31h (07FFBCC20D2D1h)  
00007FFBCC20D2C2 BA 10 00 00 00       mov         edx,10h  
00007FFBCC20D2C7 48 8B 4C 24 30       mov         rcx,qword ptr [this]  
00007FFBCC20D2CC E8 AF E0 E2 FF       call        operator delete (07FFBCC03B380h)  
00007FFBCC20D2D1 48 8B 44 24 30       mov         rax,qword ptr [this]  
00007FFBCC20D2D6 48 83 C4 28          add         rsp,28h  
00007FFBCC20D2DA C3                   ret   

Basically this boils down to:

1. Call destructor
2. if (shouldDelete == 1) operator_Delete(this);

There are basically three ways to call the destructor from C++:

How destructors are called in Microsoft ABI

Implicit call for stack-allocated objects

void Test1()
{
    Hello x;
}

MSVC currently devirtualizes the call to the destructor here (even with optimizations off) and calls ~Hello directly without going through the deleting destructor. This is ideal from a performance perspective for stack-allocated objects since you know for sure what they are. It might not be safe or practical to expose this in C#. (I also don't think we'd be able to export it via an inline export helper in situations where that's needed.)

Implicitish call when deleting heap-allocated objects

void Test2(Hello* x)
{
    delete x;
}

This basically compiles down to if (x) DestroyHello(x, 1); (see below), so the operator_Delete call in the assembly above gets called to deallocate the object. (As you might imagine, this is horribly invalid if x is not actually allocated on the heap controlled by the delete implementation.)

void Test2(Hello *) PROC                     ; Test2, COMDAT
        test    rcx, rcx
        je      SHORT $LN3@Test2
        mov     rax, QWORD PTR [rcx]
        mov     edx, 1
        rex_jmp QWORD PTR [rax]
$LN3@Test2:
        ret     0
void Test2(Hello *) ENDP                     ; Test2

Explicit call

You only ever really see this in C++ when placement new is being used. In the context of Biohazrd, it could be argued that basically all C++ objects allocated by managed code are allocated with placement new. (You can also kind-of look at stack allocation in C++ as a specialized variant of placement new.)

void Test3(Hello& x)
{
    x.~Hello();
}

void Test4(Hello* x)
{
    x->~Hello();
}

Both of these compile down to DestroyHello(x, 0);:

        mov     rax, QWORD PTR [rcx]
        xor     edx, edx
        rex_jmp QWORD PTR [rax]

---

Why function arrangement is failing here

In theory the arranged function should've caught this and emitted it with the int parameter, but it didn't (I double checked we weren't ignoring a special flag or something.)

What I suspect is happening is that we're arranging the Hello::~Hello function rather than the Hello::vector deleting destructor'` function...

Theory confirmed:

https://github.com/MochiLibraries/llvm-project/blob/c41801cb0b02eaed1db8ade850987b5c9f3ea3d6/clang/tools/libclang/PathogenExtensions.cpp#L1894

I believe we actually need to use Dtor_Deleting here.

This does expose a slight conundrum though. How will we call the base destructor for destructors overridden from C#? Obviously we could call the deleting destructor with shouldDelete = 0, but that doesn't seem quite right. One of the destructor types is Dtor_Base, is that what this is?

[PathogenDavid]

As a workaround for now, developers can manually cast the destructor pointer to the correct ABI and call it that way, IE:

((delegate* unmanaged[Cdecl]<PxDefaultFileOutputStream*, int, void>)actorOutputStream.VirtualMethodTablePointer->__DeletingDestructorPointer)(&actorOutputStream, 0);