Skip to content

Fix VTableOverlay dtor wrapper & test bitcasts on 32-bit MSVC #1048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aaronj0 merged 1 commit into from
Jul 8, 2026
+108 −54
Merged

Fix VTableOverlay dtor wrapper & test bitcasts on 32-bit MSVC #1048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 50 additions & 12 deletions lib/CppInterOp/CppInterOp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2133,6 +2133,38 @@ constexpr int kABIPrefixSize = 2;
static_assert(detail::kVTableOverlayPrefixSize == kABIPrefixSize + 1,
"public prefix size must equal ABI prefix + 1 hidden slot");

// Vtable slots hold member functions: on 32-bit MSVC that means __thiscall,
// which a free function cannot be declared as (C3865). Use a member of a
// base-less class instead; `this` is the object being destroyed. MSVC's
// deleting dtor takes (this, int flags), Itanium's D0 only `this`.
struct VTableOverlayDtorHost {
#ifdef _WIN32
void Wrapper(int flags);
#else
void Wrapper();
#endif
};
#ifdef _WIN32
using VTableOverlayDtorSlotFn = void (VTableOverlayDtorHost::*)(int);
#else
using VTableOverlayDtorSlotFn = void (VTableOverlayDtorHost::*)();
#endif

// A non-virtual mfp of a base-less class keeps the entry point in its first
// pointer-sized word on every ABI we target (MSVC: just the address;
// Itanium: {fnptr, adj} with adj = 0).
static VTableOverlayDtorSlotFn SlotToDtorFn(void* slot) {
static_assert(sizeof(VTableOverlayDtorSlotFn) >= sizeof(void*));
VTableOverlayDtorSlotFn fn{};
std::memcpy(&fn, static_cast<void*>(&slot), sizeof(slot));
return fn;
}
static void* DtorFnToSlot(VTableOverlayDtorSlotFn fn) {
void* slot;
std::memcpy(static_cast<void*>(&slot), &fn, sizeof(slot));
return slot;
}

// Single locus for vptr reads/writes and slot arithmetic in this file;
// the public header's VTableOverlayExtraSlot covers the symmetric pun
// thunks need on the read side. The owned block layout is:
Expand All @@ -2143,7 +2175,7 @@ static_assert(detail::kVTableOverlayPrefixSize == kABIPrefixSize + 1,
// address_point
//
// The wrapper at the deleting-dtor slot recovers its VTableOverlay from
// the hidden self-ptr slot via a fixed-offset load from `self`'s vptr.
// the hidden self-ptr slot via a fixed-offset load from `this`'s vptr.
struct VTableOverlay {
void** block; // owned, freed in ~VTableOverlay
void** original_vptr; // restored on caller-driven teardown
Expand All @@ -2152,7 +2184,7 @@ struct VTableOverlay {
bool dtor_fired = false; // wrapper started -- skip vptr restore
// Dtor-hook fields. orig_dtor stays null when the caller passed
// on_destroy = nullptr; the wrapper is then not installed at all.
void (*orig_dtor)(void*) = nullptr;
VTableOverlayDtorSlotFn orig_dtor = nullptr;
VTableOverlayDtorHook cleanup = nullptr;
void* cleanup_data = nullptr;

Expand Down Expand Up @@ -2200,21 +2232,29 @@ struct VTableOverlay {

// Wrapper installed in the deleting-dtor slot when MakeVTableOverlay was
// called with a non-null on_destroy. Recovers the owning VTableOverlay
// from `self`'s vptr (hidden-slot fixed-offset load) and runs the
// callback BEFORE the original destructor: `self` is alive at that
// from `this`'s vptr (hidden-slot fixed-offset load) and runs the
// callback BEFORE the original destructor: the object is alive at that
// point so the callback can inspect it, and after the original
// deleting-destructor returns memory has been freed.
extern "C" void cppinteropVTableOverlayDtorWrapper(void* self) {
void** vptr = VTableOverlay::ReadVPtr(self);
#ifdef _WIN32
void VTableOverlayDtorHost::Wrapper(int flags) {
#else
void VTableOverlayDtorHost::Wrapper() {
#endif
void** vptr = VTableOverlay::ReadVPtr(this);
VTableOverlay* ov = *reinterpret_cast<VTableOverlay**>(
vptr - detail::kVTableOverlayPrefixSize);
// Snapshot orig_dtor before user code runs: a misbehaving callback
// that destroys the overlay must not strand the C++ destructor.
auto orig_dtor = ov->orig_dtor;
ov->dtor_fired = true;
if (ov->cleanup)
ov->cleanup(self, ov->cleanup_data);
orig_dtor(self);
ov->cleanup(this, ov->cleanup_data);
#ifdef _WIN32
(this->*orig_dtor)(flags);
#else
(this->*orig_dtor)();
#endif
}

// Minimum slot count a polymorphic class can have from its address point:
Expand Down Expand Up @@ -2323,12 +2363,10 @@ MakeVTableOverlay(void* inst, ConstDeclRef base, const ConstFuncRef* methods,
// destruction could fire the wrapper with stale state.
if (on_destroy) {
void** vptr = ov->address_point();
ov->orig_dtor =
VTableOverlay::BitCastFn<void (*)(void*)>(vptr[kDeletingDtorSlot]);
ov->orig_dtor = SlotToDtorFn(vptr[kDeletingDtorSlot]);
ov->cleanup = on_destroy;
ov->cleanup_data = cleanup_data;
vptr[kDeletingDtorSlot] =
VTableOverlay::BitCastFn<void*>(&cppinteropVTableOverlayDtorWrapper);
vptr[kDeletingDtorSlot] = DtorFnToSlot(&VTableOverlayDtorHost::Wrapper);
}

return INTEROP_RETURN(ov);
Expand Down
100 changes: 58 additions & 42 deletions unittests/CppInterOp/VTableOverlayTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,17 @@
#include "gtest/gtest.h"

#include <cstdint>
#include <cstring>
#include <vector>

namespace {

// Replacement functions installed into vtable slots. The leading self
// parameter matches the implicit `this` of the virtual they replace.
extern "C" int repl_negate(void* /*self*/, int x) { return -x; }
extern "C" int repl_double(void* /*self*/, int x) { return x * 2; }

// Replacement that reads the object via `this`: layout is { vptr, int value }
// on every ABI we target, so the data member sits at sizeof(void*).
extern "C" int repl_read_value(void* self, int x) {
int v = *reinterpret_cast<int*>(static_cast<char*>(self) + sizeof(void*));
return v + x;
}

// Parallel test-TU definitions of A / B used by the OverlayThroughHierarchy
// test. Layout and ABI come from the host compiler here; the same struct
// shape is declared at interpreter level inside the test so reflection
// computes matching vtable slot indices. foo / bar access the object's
// data members via typed static_cast on the self pointer the vtable slot
// hands them -- exactly the pattern a language binding's adapter writes.
// Virtual destructors keep the vtable layout in line with the other tests
// here (Itanium D1/D0 pair before user virtuals; MSVC single deleting-dtor).
// computes matching vtable slot indices. Virtual destructors keep the
// vtable layout in line with the other tests here (Itanium D1/D0 pair
// before user virtuals; MSVC single deleting-dtor).
struct A {
int m_x;
A(int x) : m_x(x) {}
Expand All @@ -42,17 +29,45 @@ struct B : A {
int method() override { return static_cast<int>(m_d); }
};

extern "C" int foo(void* self) {
return static_cast<A*>(self)->m_x + 10;
}
extern "C" int bar(void* self) {
return static_cast<int>(static_cast<B*>(self)->m_x +
static_cast<B*>(self)->m_d);
// Replacement functions installed into vtable slots. Non-static on purpose
// (vtable slots take __thiscall on 32-bit MSVC); the casts alias the real
// object's layout through an unrelated `this` -- the point of the test.
// NOLINTBEGIN(readability-convert-member-functions-to-static, cppcoreguidelines-pro-type-reinterpret-cast)
struct Repl {
int negate(int x) { return -x; }
Comment thread
aaronj0 marked this conversation as resolved.
int twice(int x) { return x * 2; }
Comment thread
aaronj0 marked this conversation as resolved.
// Reads the object via `this`: layout is { vptr, int value } on every
// ABI we target, so the data member sits at sizeof(void*).
int read_value(int x) {
return *reinterpret_cast<int*>(reinterpret_cast<char*>(this) +

@github-actions github-actions Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]

    return *reinterpret_cast<int*>(reinterpret_cast<char*>(this) +
                                   ^

@github-actions github-actions Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]

    return *reinterpret_cast<int*>(reinterpret_cast<char*>(this) +
            ^

sizeof(void*)) +
x;
}
int foo() { return reinterpret_cast<A*>(this)->m_x + 10; }

@github-actions github-actions Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]

  int foo() { return reinterpret_cast<A*>(this)->m_x + 10; }
                     ^

int bar() {
B* b = reinterpret_cast<B*>(this);

@github-actions github-actions Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]

    B* b = reinterpret_cast<B*>(this);
           ^

return static_cast<int>(b->m_x + b->m_d);
}
};
// NOLINTEND(readability-convert-member-functions-to-static, cppcoreguidelines-pro-type-reinterpret-cast)

template <class MFP> void* MethodAddr(MFP mfp) {
static_assert(sizeof(MFP) >= sizeof(void*), "unexpected MFP layout");
void* addr;
std::memcpy(static_cast<void*>(&addr), &mfp, sizeof(addr));
return addr;
}

#if defined(_MSC_VER) && defined(_M_IX86)
# define CPPINTEROP_VTABLE_SLOT_CC __thiscall
#else
# define CPPINTEROP_VTABLE_SLOT_CC
#endif

int call_slot_no_arg(void* inst, int slot) {
void** vptr = *reinterpret_cast<void***>(inst);
return TestUtils::BitCastFn<int (*)(void*)>(vptr[slot])(inst);
return TestUtils::BitCastFn<int (CPPINTEROP_VTABLE_SLOT_CC *)(void*)>(
vptr[slot])(inst);
}

// OverlayB declared through the interpreter so the overlay runs against
Expand Down Expand Up @@ -85,7 +100,8 @@ Cpp::FuncRef Method(Cpp::DeclRef scope, const char* name) {
// reading the slot tests what the overlay actually installs.
int call_slot(void* inst, int slot, int arg) {
void** vptr = *reinterpret_cast<void***>(inst);
return TestUtils::BitCastFn<int (*)(void*, int)>(vptr[slot])(inst, arg);
return TestUtils::BitCastFn<int (CPPINTEROP_VTABLE_SLOT_CC *)(void*, int)>(
vptr[slot])(inst, arg);
}

#ifdef _WIN32
Expand All @@ -103,7 +119,7 @@ TEST(VTableOverlay, ReplacesSlotPreservingOthers) {
void* inst = Cpp::Construct(B).data;
ASSERT_NE(inst, nullptr);
auto ov = Cpp::MakeUniqueVTableOverlay(
inst, B, {{Method(B, "beta"), TestUtils::BitCastFn<void*>(&repl_negate)}});
inst, B, {{Method(B, "beta"), MethodAddr(&Repl::negate)}});
ASSERT_TRUE(ov);
EXPECT_EQ(call_slot(inst, kBeta, 5), -5); // overlaid
EXPECT_EQ(call_slot(inst, kAlpha, 5), 15); // preserved: alpha -> x+10
Expand All @@ -125,7 +141,7 @@ TEST(VTableOverlay, PreservesPrefixAndUnrelatedSlots) {
#endif
void* alpha_slot = aot[kAlpha];
auto ov = Cpp::MakeUniqueVTableOverlay(
inst, B, {{Method(B, "beta"), TestUtils::BitCastFn<void*>(&repl_negate)}});
inst, B, {{Method(B, "beta"), MethodAddr(&Repl::negate)}});
ASSERT_TRUE(ov);
void** now = *reinterpret_cast<void***>(inst);
EXPECT_EQ(now[-1], prefix_m1);
Expand All @@ -144,7 +160,7 @@ TEST(VTableOverlay, RestoresOnDestroy) {
void* aot = *reinterpret_cast<void**>(inst);
{
auto ov = Cpp::MakeUniqueVTableOverlay(
inst, B, {{Method(B, "beta"), TestUtils::BitCastFn<void*>(&repl_negate)}});
inst, B, {{Method(B, "beta"), MethodAddr(&Repl::negate)}});
ASSERT_TRUE(ov);
EXPECT_NE(*reinterpret_cast<void**>(inst), aot);
}
Expand All @@ -159,8 +175,8 @@ TEST(VTableOverlay, ReplacesMultipleSlots) {
ASSERT_NE(inst, nullptr);
auto ov = Cpp::MakeUniqueVTableOverlay(
inst, B,
{{Method(B, "alpha"), TestUtils::BitCastFn<void*>(&repl_negate)},
{Method(B, "beta"), TestUtils::BitCastFn<void*>(&repl_double)}});
{{Method(B, "alpha"), MethodAddr(&Repl::negate)},
{Method(B, "beta"), MethodAddr(&Repl::twice)}});
ASSERT_TRUE(ov);
EXPECT_EQ(call_slot(inst, kAlpha, 5), -5);
EXPECT_EQ(call_slot(inst, kBeta, 5), 10);
Expand All @@ -174,7 +190,7 @@ TEST(VTableOverlay, RejectsInvalidInput) {
ASSERT_NE(inst, nullptr);
Cpp::ConstFuncRef beta = Method(B, "beta");
Cpp::ConstFuncRef none = nullptr;
void* fn = TestUtils::BitCastFn<void*>(&repl_negate);
void* fn = MethodAddr(&Repl::negate);

EXPECT_EQ(Cpp::MakeVTableOverlay(nullptr, B, &beta, &fn, 1), nullptr); // inst
EXPECT_EQ(Cpp::MakeVTableOverlay(inst, nullptr, &beta, &fn, 1),
Expand All @@ -194,7 +210,7 @@ TEST(VTableOverlay, OverlayIsPerInstance) {
void* b_vptr_before = *reinterpret_cast<void**>(b);

auto ov = Cpp::MakeUniqueVTableOverlay(
a, B, {{Method(B, "beta"), TestUtils::BitCastFn<void*>(&repl_negate)}});
a, B, {{Method(B, "beta"), MethodAddr(&Repl::negate)}});
ASSERT_TRUE(ov);

EXPECT_NE(*reinterpret_cast<void**>(a), b_vptr_before); // a swapped
Expand Down Expand Up @@ -224,7 +240,7 @@ TEST(VTableOverlay, ThunkReadsThisAndDataMember) {
ASSERT_NE(inst, nullptr);

auto ov = Cpp::MakeUniqueVTableOverlay(
inst, T, {{Method(T, "frob"), TestUtils::BitCastFn<void*>(&repl_read_value)}});
inst, T, {{Method(T, "frob"), MethodAddr(&Repl::read_value)}});
ASSERT_TRUE(ov);

// First user virtual lives at kAlpha (Itanium D1/D0 prefix; MSVC single
Expand Down Expand Up @@ -254,7 +270,7 @@ TEST(VTableOverlay, DerivedClassWithOverride) {
EXPECT_EQ(call_slot(inst, kAlpha, 5), 7);

auto ov = Cpp::MakeUniqueVTableOverlay(
inst, D, {{Method(D, "frob"), TestUtils::BitCastFn<void*>(&repl_negate)}});
inst, D, {{Method(D, "frob"), MethodAddr(&Repl::negate)}});
ASSERT_TRUE(ov);
EXPECT_EQ(call_slot(inst, kAlpha, 5), -5);
ov.reset();
Expand All @@ -276,7 +292,7 @@ TEST(VTableOverlay, MultiLevelInheritance) {
EXPECT_EQ(call_slot(inst, kAlpha, 5), 8); // MlC::frob: x+3

auto ov = Cpp::MakeUniqueVTableOverlay(
inst, C, {{Method(C, "frob"), TestUtils::BitCastFn<void*>(&repl_double)}});
inst, C, {{Method(C, "frob"), MethodAddr(&Repl::twice)}});
ASSERT_TRUE(ov);
EXPECT_EQ(call_slot(inst, kAlpha, 5), 10); // overlay: x*2
ov.reset();
Expand All @@ -302,7 +318,7 @@ TEST(VTableOverlay, RejectsMultipleInheritance) {
ASSERT_NE(inst, nullptr);

auto ov = Cpp::MakeUniqueVTableOverlay(
inst, C, {{Method(C, "af"), TestUtils::BitCastFn<void*>(&repl_negate)}});
inst, C, {{Method(C, "af"), MethodAddr(&Repl::negate)}});
EXPECT_FALSE(ov); // refuses the layout

Cpp::Destruct(inst, C);
Expand All @@ -323,7 +339,7 @@ TEST(VTableOverlay, RejectsNonPolymorphicBase) {
void* inst = Cpp::Construct(NP).data;
ASSERT_NE(inst, nullptr);
Cpp::ConstFuncRef dummy = Method(PH, "dummy");
void* fn = TestUtils::BitCastFn<void*>(&repl_negate);
void* fn = MethodAddr(&Repl::negate);
EXPECT_EQ(Cpp::MakeVTableOverlay(inst, NP, &dummy, &fn, 1), nullptr);
Cpp::Destruct(inst, NP);
}
Expand Down Expand Up @@ -357,7 +373,7 @@ TEST(VTableOverlay, RejectsOutOfRangeMethodSlot) {
void* inst = Cpp::Construct(Small).data;
ASSERT_NE(inst, nullptr);
Cpp::ConstFuncRef v10 = Method(Large, "v10");
void* fn = TestUtils::BitCastFn<void*>(&repl_negate);
void* fn = MethodAddr(&Repl::negate);
EXPECT_EQ(Cpp::MakeVTableOverlay(inst, Small, &v10, &fn, 1), nullptr);
Cpp::Destruct(inst, Small);
}
Expand Down Expand Up @@ -395,11 +411,11 @@ TEST(VTableOverlay, OverlayThroughHierarchyAccessesDataMembers) {

auto ov_a = Cpp::MakeUniqueVTableOverlay(
&a, A_scope,
{{Method(A_scope, "method"), TestUtils::BitCastFn<void*>(&foo)}});
{{Method(A_scope, "method"), MethodAddr(&Repl::foo)}});
ASSERT_TRUE(ov_a);
auto ov_b = Cpp::MakeUniqueVTableOverlay(
&b, B_scope,
{{Method(B_scope, "method"), TestUtils::BitCastFn<void*>(&bar)}});
{{Method(B_scope, "method"), MethodAddr(&Repl::bar)}});
ASSERT_TRUE(ov_b);

EXPECT_EQ(call_slot_no_arg(&a, kAlpha), 15); // foo: A::m_x(5) + 10
Expand Down Expand Up @@ -431,7 +447,7 @@ TEST(VTableOverlay, RejectsVirtualInheritance) {
ASSERT_NE(inst, nullptr);

auto ov = Cpp::MakeUniqueVTableOverlay(
inst, D, {{Method(D, "frob"), TestUtils::BitCastFn<void*>(&repl_negate)}});
inst, D, {{Method(D, "frob"), MethodAddr(&Repl::negate)}});
EXPECT_FALSE(ov); // refuses the layout

Cpp::Destruct(inst, D);
Expand Down Expand Up @@ -576,7 +592,7 @@ TEST(VTableOverlay, SetsExtraPrefixSlots) {
void* inst = Cpp::Construct(B).data;
ASSERT_NE(inst, nullptr);
auto ov = Cpp::MakeUniqueVTableOverlay(
inst, B, {{Method(B, "beta"), TestUtils::BitCastFn<void*>(&repl_negate)}},
inst, B, {{Method(B, "beta"), MethodAddr(&Repl::negate)}},
/*n_extra_prefix_slots=*/2);
ASSERT_TRUE(ov);
void*& slot0 = Cpp::VTableOverlayExtraSlot(inst, 0);
Expand Down
Loading