Skip to content

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

Open
aaronj0 wants to merge 1 commit into
compiler-research:mainfrom
aaronj0:win-x86-vtable-overlay
Open

Fix VTableOverlay dtor wrapper & test bitcasts on 32-bit MSVC#1048
aaronj0 wants to merge 1 commit into
compiler-research:mainfrom
aaronj0:win-x86-vtable-overlay

Conversation

@aaronj0

@aaronj0 aaronj0 commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Add __thiscall to the bitcast type on _M_IX86 so the caller reproduces the compiler's virtual-dispatch ABI.
Fixes all failing tests of the VTableOverlay on 32-bit MSVC:

Test Assert Got Expected
ReplacesSlotPreservingOthers call_slot(inst, kBeta, 5) -1211418038 -5
ReplacesMultipleSlots call_slot(inst, kAlpha, 5) -1211418038 -5
OverlayIsPerInstance call_slot(a, kBeta, 5) -1211418038 -5
ThunkReadsThisAndDataMember SEH 0xc0000005 105
DerivedClassWithOverride call_slot(inst, kAlpha, 5) -1211418038 -5
MultiLevelInheritance call_slot(inst, kAlpha, 5) -1872131220 10
OverlayThroughHierarchyAccessesDataMembers call_slot_no_arg(&a/&b, kAlpha) 826692698 / 826692688 15 / 10
SetsExtraPrefixSlots call_slot(inst, kBeta, 5) -1211418038 -5

🤖 AI-assisted

Add __thiscall to the bitcast type on _M_IX86 so the caller reproduces the compiler's virtual-dispatch ABI
@aaronj0 aaronj0 requested a review from vgvassilev July 3, 2026 14:42
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.73%. Comparing base (e3fc693) to head (27c51ab).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1048   +/-   ##
=======================================
  Coverage   86.73%   86.73%           
=======================================
  Files          23       23           
  Lines        5970     5972    +2     
=======================================
+ Hits         5178     5180    +2     
  Misses        792      792           
Files with missing lines Coverage Δ
lib/CppInterOp/CppInterOp.cpp 89.32% <100.00%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
lib/CppInterOp/CppInterOp.cpp 89.32% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot left a comment

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.

clang-tidy made some suggestions

static VTableOverlayDtorSlotFn SlotToDtorFn(void* slot) {
static_assert(sizeof(VTableOverlayDtorSlotFn) >= sizeof(void*));
VTableOverlayDtorSlotFn fn{};
std::memcpy(&fn, &slot, sizeof(slot));

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: multilevel pointer conversion from 'void **' to 'const void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

n fn{};
                           ^

}
static void* DtorFnToSlot(VTableOverlayDtorSlotFn fn) {
void* slot;
std::memcpy(&slot, &fn, sizeof(slot));

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: multilevel pointer conversion from 'void **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

* slot;
                      ^

static_cast<B*>(self)->m_d);
// Replacement functions installed into vtable slots.
struct Repl {
int negate(int x) { return -x; }

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: method 'negate' can be made static [readability-convert-member-functions-to-static]

Suggested change
int negate(int x) { return -x; }
static int negate(int x) { return -x; }

// Replacement functions installed into vtable slots.
struct Repl {
int negate(int x) { return -x; }
int twice(int x) { return x * 2; }

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: method 'twice' can be made static [readability-convert-member-functions-to-static]

Suggested change
int twice(int x) { return x * 2; }
static 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<int*>(reinterpret_cast<char*>(this) +

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) +
                                   ^

// 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) +

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; }

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 foo() { return reinterpret_cast<A*>(this)->m_x + 10; }
int bar() {
B* b = reinterpret_cast<B*>(this);

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);
           ^

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

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: multilevel pointer conversion from 'void **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

  std::memcpy(&addr, &mfp, sizeof(addr));
              ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant