Fix cross-module class extends crash and add regression coverage#274
Merged
Conversation
ClassMethodAccess's isDynamicImport branches (called for a class whose base lives in another module) fed an unchecked, possibly-null Value into CreateBoundFunctionOp when the target method's global variable couldn't be resolved by name - an intermittent, heap-layout-sensitive access violation (masked whenever a debugger happened to be attached, which made it look like a flaky Heisenbug). Adds a null-check that turns the failure into a clean compile error, plus a theModule.lookupSymbol fallback since compiler- synthesized methods (e.g. .instanceOf) are registered as real FuncOps rather than the dlsym-style global variables regular imported methods use. Adds the project's first regression coverage for cross-module class inheritance: basic 2-level extends, a 3-level chain, and extends+implements combined. The non-shared-lib variants pass and are enabled; the -shared (AOT/JIT) variants hit a separate, deeper pre-existing bug in when .instanceOf gets synthesized for a class compiled across a real DLL boundary - left disabled with a detailed comment rather than patched further, since two targeted fix attempts each introduced a worse regression (an infinite loop in unrelated same-module class tests). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 task
ASDAlexander77
added a commit
that referenced
this pull request
Jul 22, 2026
…vestigation (#276) * Fix CRT assert dialogs blocking debug tslang.exe runs; document cross-module .instanceOf investigation _CrtSetReportFile(_CRT_ASSERT, ...) has no effect unless the report mode for that category is also set to _CRTDBG_MODE_FILE - without it, the default mode (_CRTDBG_MODE_WNDW) still pops a blocking "Assertion failed" MessageBox on every assert(), which looks like a hang in a non-interactive session. One missing _CrtSetReportMode call fixes it. Also documents a three-attempt investigation into the disabled cross-module `-shared` class-extends tests (PR #274's known issue): root-caused why .instanceOf can't be resolved (two real bugs found and fixed in isolation), but every attempt to fix it regressed unrelated tests project-wide (70+ tests, then 10 more on a narrower retry) via a fragile scoped-hash-table interaction that isn't safe to patch blindly. All three attempts reverted; the actual fix is left to a future, more careful session per the recommendation in the new doc. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * Fix cross-module class extends under -shared: inline dlsym resolution + decl-printer fixes (#277) Makes a derived class extending a base from a dynamically imported module (-shared, real DLL boundary) work end-to-end - all six formerly disabled tests (basic/multilevel/diamond x compile/JIT) now pass and are enabled. Three co-operating fixes, each exposing the next: 1. ClassMethodAccess (isDynamicImport instance branch): when a member has neither a locally defined FuncOp (bodyless declarations are now explicitly excluded - they lower to unlinkable external references) nor a registered dlsym-global, resolve it in place via SearchForAddressOfSymbolOp + cast - no global registration at all, sidestepping the fullNameGlobalsMap scope fragility that sank the previous session's three attempts (see docs/cross-module-dynamic-import-instanceof-design.md, now updated with the resolution in section 10). 2. mlirGenClassVirtualTableDefinition: vtable slots whose symbols are owned by a dynamic-import base (inherited virtual methods; .rtti/.size statics under ADD_STATIC_MEMBERS_TO_VTABLE) emit runtime symbol resolution instead of constant SymbolRefOp - a link-time address of a DLL-resident symbol does not exist without an import library. GlobalOpLowering already routes such initializers through the __cctor path. 3. DeclarationPrinter: the extends clause printed the class's OWN name instead of the base's (unused loop variable - "class B extends M.B"), which made the importer build a self-cycle in baseClasses and stack-overflow in getVirtualTable's unguarded recursion; and the synthetic base-class storage field (memory layout, not a source member) leaked into the printed decl as a real field, shifting every subsequent field's offset in the importer - the DLL's methods saw b=22 while the importer read c.b==0 from one slot past it. Full suite green: 767/767 (761 prior + 6 newly enabled). Co-authored-by: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ClassMethodAccess'sisDynamicImportbranches (hit when a derived class' base class lives in another module) fed an unchecked, possibly-nullValueintoCreateBoundFunctionOpwhenever the target method's global couldn't be resolved by name — an intermittent, heap-layout-sensitive access violation. It was masked whenever a debugger happened to be attached (ProcDump/WinDbg), which is why it initially looked like a flaky Heisenbug rather than a deterministic bug.theModule.lookupSymbolfallback — compiler-synthesized methods (e.g..instanceOf, which every class gets) are registered as realFuncOps rather than the dlsym-style global variables that regular imported/declared methods use, so the original single-mechanism lookup could never find them.class extends: basic 2-level extends, a 3-level chain, and extends+implements combined (mirroring the interface-extends coverage added in Fix cross-module diamond-extends interface silently corrupting data #268-Extend interface extends coverage to 3-target extends clauses #270). The non-shared-lib variants pass and are enabled.Known issue (documented, not fixed here)
The
-shared(AOT and JIT) variants of the same class hierarchies hit a separate, deeper pre-existing bug:.instanceOfisn't always reliably synthesized/registered when a class is compiled across a real DLL boundary (order-dependent on discovery/partial-resolve pass sequencing). Two targeted fix attempts were tried and reverted:.instanceOf/.newsynthesis until a non-speculative compiler pass caused an infinite loop in unrelated same-module class tests (their own discovery retry loop can also always run underallowPartialResolve).Both were reverted in favor of leaving a clean, documented known issue rather than risking a worse regression. The 5 affected
-sharedtest entries are commented out inCMakeLists.txtwith a full explanation.Test plan
ctestsuite (761 tests): 760/761 passing — the sole failure (unittest-MLIRGenTests, a tuple bracket-vs-brace printing mismatch) is pre-existing and unrelated to this change.test-runnerharness).🤖 Generated with Claude Code