Remove the December 2025 LTO de-inlining stopgap now that WTF::opaque() is volatile - #403
Conversation
…() is volatile b2beff6 ("Temp: attempt to fix", Dec 2025) was a stopgap while chasing a FinalizationRegistry miscompile on Alpine Linux LTO builds. It demoted ALWAYS_INLINE to plain inline and ALWAYS_INLINE_LAMBDA to nothing across the whole tree, moved Heap::isMarked, MarkedBlock::isMarked, and MarkedBlock::Handle::isLive out of line as NEVER_INLINE, marked Dependency::fence/loadAndFence and JSFinalizationRegistry::finalizeUnconditionally NEVER_INLINE, and swapped the x86_64 compiler fences for std::atomic_thread_fence. The actual root cause was found upstream: 302941@main made the inline asm in WTF::opaque() non-volatile, so the optimizer could elide or relocate it, breaking the Dependency-based fenceless reads in the GC's liveness checks. Upstream fixed it in 308243@main (webkit.org/b/308660, "inline asm in WTF::opaque() needs to be volatile"), which this tree has carried since February. This restores upstream parity at our merge base (3722912): ALWAYS_INLINE and ALWAYS_INLINE_LAMBDA are enforced again in release builds, the GC liveness checks are back in their inline headers (upstream measured ~2% on splay from isLive's inlining alone), and the fence definitions match upstream.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (2)
WalkthroughChangesThe pull request moves JavaScriptCore heap mark and liveness checks into inline headers. It updates compiler inlining and x86 fence macros, and removes Heap marking and liveness
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Given that it touches GC liveness hot paths, memory-ordering primitives, and re-enables __always_inline__ tree-wide — and the author notes the decisive musl LTO regression run is still pending — a human look and the CI result are worth waiting on before merging.
What was reviewed:
- Confirmed
WTF::opaque()andcompilerFence()both carryasm volatilewith the needed constraints, so the stated root-cause fix is in place. - Restored bodies of
Heap::isMarked,MarkedBlock::isMarked, and bothHandle::isLiveoverloads match the versions removed from the .cpp files (plus the upstream comment block). - x86_64
loadLoadFence/loadStoreFence/storeStoreFencereverting tocompilerFence()is sound under TSO and matches the upstream pattern.
Extended reasoning...
Overview
This PR reverts a December 2025 stopgap that worked around a FinalizationRegistry miscompile on Alpine musl LTO builds. It restores upstream parity across eight files: ALWAYS_INLINE/ALWAYS_INLINE_LAMBDA regain __attribute__((__always_inline__)) in NDEBUG builds (Compiler.h), the GC liveness checks (Heap::isMarked, MarkedBlock::isMarked, MarkedBlock::Handle::isLive) move back from .cpp NEVER_INLINE definitions to their inline headers, Dependency::fence/loadAndFence and JSFinalizationRegistry::finalizeUnconditionally drop NEVER_INLINE, and the x86_64 load-load/load-store/store-store fences revert from std::atomic_thread_fence to compilerFence().
Security risks
The direct security exposure is memory safety in the concurrent GC. If the root-cause analysis were wrong and the optimizer can still reorder or elide the fenceless-read machinery under LTO, isLive/isMarked could return stale results, leading to premature collection and use-after-free. I verified that WTF::opaque() now uses asm volatile("" : "+r"(value) ::) and compilerFence() uses asm volatile("" ::: "memory"), which are the correct barriers. On x86_64 TSO, compiler fences are sufficient for load-load/load-store/store-store ordering, so that reversion is semantically fine. There is no injection, auth, or data-exposure surface here.
Level of scrutiny
High. Three factors push this well outside auto-approval territory: (1) the code sits on the GC marking/liveness hot path where a regression means heap corruption rather than a visible crash; (2) the ALWAYS_INLINE macro change is tree-wide and affects codegen for essentially all of JSC/WTF in release builds; (3) the original bug only reproduced on a specific toolchain/config (musl + LTO), and the author explicitly states that the decisive regression test — Bun's suite against the musl LTO preview artifacts — has not yet been run and will be reported before merge.
Other factors
The restored function bodies are byte-identical (modulo the reinstated explanatory comment) to what was removed from the .cpp files, and the PR description says they were diffed against upstream at the merge base. The bug-hunting pass found nothing. Local verification (clang 21 release build + FinalizationRegistry stress loop) passed, but that is not the configuration that originally failed. Given the pending CI gate the author called out and the criticality of the code, deferring to a human reviewer is the right call.
Preview Builds
|
|
Preview build succeeded across the full matrix (including linux musl lto). Bun-side CI validation is running in oven-sh/bun#37306, pinned to autobuild-preview-pr-403-a9acfec7. I will post the results here. |
objectPrototypeToString is defined ALWAYS_INLINE in ObjectPrototypeInlines.h. With only ObjectPrototype.h included, the call compiled to an extern reference that happened to resolve against an out-of-line copy JSC emitted while the fork demoted ALWAYS_INLINE to plain inline. oven-sh/WebKit#403 restores the attribute, no out-of-line copy exists, and every release link fails with an undefined symbol. Include the inlines header so this TU has the definition.
|
Bun CI results against the preview artifacts are in: oven-sh/bun#37306, build 91420 (196 jobs).
Summary: correctness validated across the matrix; the open question is the size-for-speed tradeoff, which is a maintainer call. |
The WebKit bump to 3997b59485da restores ALWAYS_INLINE to always_inline (oven-sh/WebKit#403), so release archives no longer carry an out-of-line copy of objectPrototypeToString and every link failed with an undefined symbol. Pull in the inline definition where it is called.
Restoring ALWAYS_INLINE (oven-sh/WebKit#403, in the 3997b59485da pin) re-inlines JSC hot paths that the December stopgap had left out of line, adding roughly 3 MB per target relative to main's older pin.
…after oven-sh/WebKit#403) No-Verification-Needed: include-only change required to link against the bumped WebKit; exercised by this PR's CI build.
`447082ab6897` → `3997b59485da` is three commits on the fork: - oven-sh/WebKit#405: JSC's `DebuggerParseData`, `CachedTypes` and `Completion` switches get a `BunTranspiledModule` arm next to `Module`. Runtime ESM that Bun hands to JSC with a prebuilt module record (`bun test --isolate` / `--parallel`, `bun build --compile` output) uses that source type, and `gatherDebuggerParseDataForSource` returned false for it, so `Debugger.setBreakpoint` replied "Could not resolve breakpoint" and `Debugger.setBreakpointByUrl` returned no locations for those files. - oven-sh/WebKit#406: exception checks on the inspector's pause / `evaluateOnCallFrame` / `Runtime.evaluate` paths (`JSJavaScriptCallFrame`, `JSInjectedScriptHost`, `jsToInspectorValue`). Under `validateExceptionChecks` (the ASAN lane) a paused inspectee used to abort at `JSJavaScriptCallFrame::scopeChain`, which is why `inspector.test.ts` stripped the flag for its children and `inspect.test.ts` sat in `no-validate-exceptions.txt`. Both workarounds are removed here; all of `test/cli/inspect/*` and `test/js/node/inspector/inspector.test.ts` pass locally with the flag forced on for every child. - oven-sh/WebKit#403 also landed in between: `ALWAYS_INLINE` is `__always_inline__` again in release builds, so `NodeUtilTypesModule.cpp` now includes `ObjectPrototypeInlines.h` for `objectPrototypeToString` (the only out-of-line use that relied on the old stopgap; release links locally against the new tarball). `test/cli/inspect/debugger-buntranspiledmodule.test.ts` (from #35754, updated for the `scriptType` param the Aug 2 upgrade introduced) drives `bun test --isolate` under `--inspect-wait` and asserts both breakpoint calls resolve; it fails on `447082ab` and passes here. #35605 makes every `bun run` module take the `BunTranspiledModule` path; splitting the bump out so it can land first. Supersedes #35754. --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
|
Since this reached Bun (oven-sh/bun#37352), Bun's linux x64 musl lane has crashed three times in the GC marking threads on already-collected cells (oven-sh/bun CI builds 96675, 97509, 100350), the configuration the December arrangement was added for. On x86_64 the liveness paths do not go through WTF::opaque(), so the fix cited here did not reach them. #466 restores what this removed, with the evidence. |
|
Correction to my comment above: #466 is closed. The December failure the stopgap was added for was fixed on the Bun side (oven-sh/bun@22b3be442c, a FileSink wrapper collected while a write was pending), so the stopgap was never shown to do anything and this PR's removal of it is not established as the cause of the current alpine x64 crashes. Details in #466. |
3 participants
b2beff6 ("Temp: attempt to fix", December 2025) was a stopgap added while chasing a FinalizationRegistry miscompile that only reproduced on Alpine Linux LTO builds, and it has survived every upstream merge since. It:
ALWAYS_INLINEto plaininlineandALWAYS_INLINE_LAMBDAto nothing, tree-wideHeap::isMarked,MarkedBlock::isMarked, and bothMarkedBlock::Handle::isLiveoverloads out of their inline headers into .cpp files asNEVER_INLINEDependency::fence,Dependency::loadAndFence, andJSFinalizationRegistry::finalizeUnconditionallyasNEVER_INLINEcompilerFence()fences withstd::atomic_thread_fenceThe actual root cause was upstream 302941@main ("Clean up opaque atomic variables"), which dropped
volatilefrom the inline asm inWTF::opaque(). Withoutvolatile, the optimizer may elide or relocate the asm statement, which breaks the Dependency-based fenceless reads in the GC liveness checks, and LTO made that visible. Upstream diagnosed and fixed it in 308243@main (https://bugs.webkit.org/show_bug.cgi?id=308660, "inline asm in WTF::opaque() needs to be volatile"), and this tree has carried that fix since February (9f672ca).With the root cause fixed, this removes the stopgap and restores upstream parity at our merge base (3722912):
ALWAYS_INLINE/ALWAYS_INLINE_LAMBDAenforce__always_inline__again in release (NDEBUG) buildsisLive's inlining alone, andHeap::isMarkedsits on the marking hot pathDependency::fence/loadAndFenceinline again, which matters most on ARM64 where every consume load otherwise became a real function callVerification:
-O3/NDEBUG,ENABLE_STATIC_JSC=ON, FTL,USE_BUN_JSC_ADDITIONS=ON) compiles cleanly and the jsc shell runsThe original failure mode was specific to musl LTO builds, so the regression test that matters is Bun's CI on linux x64 musl against this PR's preview artifacts. I will run Bun's suite with a draft PR pinned to the preview tag and report the result here before this merges.