From 3e4adffad5a23942a43631c3b8fd24ca6244f17d Mon Sep 17 00:00:00 2001 From: Aaron Jomy Date: Thu, 2 Jul 2026 09:35:02 +0200 Subject: [PATCH] Fix VTableOverlay dtor wrapper & test bitcasts on 32-bit MSVC Add __thiscall to the bitcast type on _M_IX86 so the caller reproduces the compiler's virtual-dispatch ABI --- lib/CppInterOp/CppInterOp.cpp | 62 ++++++++++--- unittests/CppInterOp/VTableOverlayTest.cpp | 100 ++++++++++++--------- 2 files changed, 108 insertions(+), 54 deletions(-) diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index ea08c0470..986e50ce2 100644 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/lib/CppInterOp/CppInterOp.cpp @@ -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(&slot), sizeof(slot)); + return fn; +} +static void* DtorFnToSlot(VTableOverlayDtorSlotFn fn) { + void* slot; + std::memcpy(static_cast(&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: @@ -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 @@ -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; @@ -2200,12 +2232,16 @@ 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( vptr - detail::kVTableOverlayPrefixSize); // Snapshot orig_dtor before user code runs: a misbehaving callback @@ -2213,8 +2249,12 @@ extern "C" void cppinteropVTableOverlayDtorWrapper(void* self) { 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: @@ -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(vptr[kDeletingDtorSlot]); + ov->orig_dtor = SlotToDtorFn(vptr[kDeletingDtorSlot]); ov->cleanup = on_destroy; ov->cleanup_data = cleanup_data; - vptr[kDeletingDtorSlot] = - VTableOverlay::BitCastFn(&cppinteropVTableOverlayDtorWrapper); + vptr[kDeletingDtorSlot] = DtorFnToSlot(&VTableOverlayDtorHost::Wrapper); } return INTEROP_RETURN(ov); diff --git a/unittests/CppInterOp/VTableOverlayTest.cpp b/unittests/CppInterOp/VTableOverlayTest.cpp index a4a41ab0d..848ffdab9 100644 --- a/unittests/CppInterOp/VTableOverlayTest.cpp +++ b/unittests/CppInterOp/VTableOverlayTest.cpp @@ -5,30 +5,17 @@ #include "gtest/gtest.h" #include +#include #include 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(static_cast(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) {} @@ -42,17 +29,45 @@ struct B : A { int method() override { return static_cast(m_d); } }; -extern "C" int foo(void* self) { - return static_cast(self)->m_x + 10; -} -extern "C" int bar(void* self) { - return static_cast(static_cast(self)->m_x + - static_cast(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; } + int twice(int x) { return x * 2; } + // 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(reinterpret_cast(this) + + sizeof(void*)) + + x; + } + int foo() { return reinterpret_cast(this)->m_x + 10; } + int bar() { + B* b = reinterpret_cast(this); + return static_cast(b->m_x + b->m_d); + } +}; +// NOLINTEND(readability-convert-member-functions-to-static, cppcoreguidelines-pro-type-reinterpret-cast) + +template void* MethodAddr(MFP mfp) { + static_assert(sizeof(MFP) >= sizeof(void*), "unexpected MFP layout"); + void* addr; + std::memcpy(static_cast(&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(inst); - return TestUtils::BitCastFn(vptr[slot])(inst); + return TestUtils::BitCastFn( + vptr[slot])(inst); } // OverlayB declared through the interpreter so the overlay runs against @@ -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(inst); - return TestUtils::BitCastFn(vptr[slot])(inst, arg); + return TestUtils::BitCastFn( + vptr[slot])(inst, arg); } #ifdef _WIN32 @@ -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(&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 @@ -125,7 +141,7 @@ TEST(VTableOverlay, PreservesPrefixAndUnrelatedSlots) { #endif void* alpha_slot = aot[kAlpha]; auto ov = Cpp::MakeUniqueVTableOverlay( - inst, B, {{Method(B, "beta"), TestUtils::BitCastFn(&repl_negate)}}); + inst, B, {{Method(B, "beta"), MethodAddr(&Repl::negate)}}); ASSERT_TRUE(ov); void** now = *reinterpret_cast(inst); EXPECT_EQ(now[-1], prefix_m1); @@ -144,7 +160,7 @@ TEST(VTableOverlay, RestoresOnDestroy) { void* aot = *reinterpret_cast(inst); { auto ov = Cpp::MakeUniqueVTableOverlay( - inst, B, {{Method(B, "beta"), TestUtils::BitCastFn(&repl_negate)}}); + inst, B, {{Method(B, "beta"), MethodAddr(&Repl::negate)}}); ASSERT_TRUE(ov); EXPECT_NE(*reinterpret_cast(inst), aot); } @@ -159,8 +175,8 @@ TEST(VTableOverlay, ReplacesMultipleSlots) { ASSERT_NE(inst, nullptr); auto ov = Cpp::MakeUniqueVTableOverlay( inst, B, - {{Method(B, "alpha"), TestUtils::BitCastFn(&repl_negate)}, - {Method(B, "beta"), TestUtils::BitCastFn(&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); @@ -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(&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), @@ -194,7 +210,7 @@ TEST(VTableOverlay, OverlayIsPerInstance) { void* b_vptr_before = *reinterpret_cast(b); auto ov = Cpp::MakeUniqueVTableOverlay( - a, B, {{Method(B, "beta"), TestUtils::BitCastFn(&repl_negate)}}); + a, B, {{Method(B, "beta"), MethodAddr(&Repl::negate)}}); ASSERT_TRUE(ov); EXPECT_NE(*reinterpret_cast(a), b_vptr_before); // a swapped @@ -224,7 +240,7 @@ TEST(VTableOverlay, ThunkReadsThisAndDataMember) { ASSERT_NE(inst, nullptr); auto ov = Cpp::MakeUniqueVTableOverlay( - inst, T, {{Method(T, "frob"), TestUtils::BitCastFn(&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 @@ -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(&repl_negate)}}); + inst, D, {{Method(D, "frob"), MethodAddr(&Repl::negate)}}); ASSERT_TRUE(ov); EXPECT_EQ(call_slot(inst, kAlpha, 5), -5); ov.reset(); @@ -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(&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(); @@ -302,7 +318,7 @@ TEST(VTableOverlay, RejectsMultipleInheritance) { ASSERT_NE(inst, nullptr); auto ov = Cpp::MakeUniqueVTableOverlay( - inst, C, {{Method(C, "af"), TestUtils::BitCastFn(&repl_negate)}}); + inst, C, {{Method(C, "af"), MethodAddr(&Repl::negate)}}); EXPECT_FALSE(ov); // refuses the layout Cpp::Destruct(inst, C); @@ -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(&repl_negate); + void* fn = MethodAddr(&Repl::negate); EXPECT_EQ(Cpp::MakeVTableOverlay(inst, NP, &dummy, &fn, 1), nullptr); Cpp::Destruct(inst, NP); } @@ -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(&repl_negate); + void* fn = MethodAddr(&Repl::negate); EXPECT_EQ(Cpp::MakeVTableOverlay(inst, Small, &v10, &fn, 1), nullptr); Cpp::Destruct(inst, Small); } @@ -395,11 +411,11 @@ TEST(VTableOverlay, OverlayThroughHierarchyAccessesDataMembers) { auto ov_a = Cpp::MakeUniqueVTableOverlay( &a, A_scope, - {{Method(A_scope, "method"), TestUtils::BitCastFn(&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(&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 @@ -431,7 +447,7 @@ TEST(VTableOverlay, RejectsVirtualInheritance) { ASSERT_NE(inst, nullptr); auto ov = Cpp::MakeUniqueVTableOverlay( - inst, D, {{Method(D, "frob"), TestUtils::BitCastFn(&repl_negate)}}); + inst, D, {{Method(D, "frob"), MethodAddr(&Repl::negate)}}); EXPECT_FALSE(ov); // refuses the layout Cpp::Destruct(inst, D); @@ -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(&repl_negate)}}, + inst, B, {{Method(B, "beta"), MethodAddr(&Repl::negate)}}, /*n_extra_prefix_slots=*/2); ASSERT_TRUE(ov); void*& slot0 = Cpp::VTableOverlayExtraSlot(inst, 0);