RefCountersImpl is destroyed in a different execution unit than the one in which it was created

[hzqst]

As we knows m_pRefCounters can be allocated from different execution units (my dll and Archiver dll in this case)

However MSVC compiles the following virtual function call into direct function call:

//RefCntAutoPtr.hpp

void Release() noexcept
{
    if (m_pRefCounters)
        m_pRefCounters->ReleaseWeakRef(); //will be optimized by MSVC 
    m_pRefCounters = nullptr;
    m_pObject      = nullptr;
}

This can lead to crash when my dll uses custom global operator_new override (mi_new to override pNewRefCounters allocation in this case) :

//RefCountedObjectImpl.hpp

pNewRefCounters = new RefCountersImpl{}; //this goes with `mi_new` from my dll and with ms crt `malloc` from archiver dll

Possible fix A:

Use custom allocator m_pAllocator to allocate RefCountersImpl when available.

Possible fix B:

Tell MSVC not to optimize the virtual function call

#pragma optimize( "", off )
void Release() noexcept
{
    if (m_pRefCounters)
        m_pRefCounters->ReleaseWeakRef(); //call archiver one when optimization off
    m_pRefCounters = nullptr;
    m_pObject      = nullptr;
}
#pragma optimize( "", on )

[TheMostDiligent]

ReleaseWeakRef() is labeled as final, so removing virtual call is totally correct and intended. MSVC is doing exactly what it should.

Using custom allocator m_pAllocator to allocate RefCountersImpl seems to be the right solution here.

[hzqst]

ReleaseWeakRef() is labeled as final, so removing virtual call is totally correct and intended. MSVC is doing exactly what it should.

Using custom allocator m_pAllocator to allocate RefCountersImpl seems to be the right solution here.

Is there any special reason we have final there in void ReleaseWeakRef()? final breaks the principle that one should always be released by the execution unit it was allocated from.

[TheMostDiligent]

Does this work properly if you remove final?

[hzqst]

Does this work properly if you remove final?

no, it has nothing to do with final

It works after changing RefCountersImpl* m_pRefCounters to IReferenceCounters* m_pRefCounters, which make it possible to store object shared across execution units.

[TheMostDiligent]

This change will introduce a very high overhead.
Any time Release is called, there will be a virtual call. The cost of Release must be one interlocked decrement unless the object needs to be destroyed.

[TheMostDiligent]

Try this branch: https://github.com/DiligentGraphics/DiligentCore/compare/ref_counters_fix

[hzqst]

Try this branch: https://github.com/DiligentGraphics/DiligentCore/compare/ref_counters_fix

No it doesn't work.

What about inheriting like this :

RefCountersAllocationImpl -> RefCountersImpl -> IReferenceCounters ,

Add virtual void SelfDestroy() = 0 to RefCountersImpl as an interface,

Add void SelfDestroy() override { delete this; } to RefCountersAllocationImpl

There will be no overhead except when calling SelfDestroy

[TheMostDiligent]

What is the issue with my solution - it still calls new from one module, but delete from another?

[hzqst]

What is the issue with my solution - it still calls new from one module, but delete from another?

yes, still got the exact same result as this one

[TheMostDiligent]

Did you do clean rebuild?

[TheMostDiligent]

Actually, this will not help.
Try making SelfDestroy virtual function.

[hzqst]

Actually, this will not help. Try making SelfDestroy virtual function.

I've pushed a pr to make SelfDestroy virtual function. and it works without problem on my end.