[wasm][R2R] Fix Crst level violation between NativeImageEagerFixups and PregeneratedStringThunks - #131355
Merged
Conversation
…nd PregeneratedStringThunks
During a composite R2R load on WebAssembly, the eager-fixup path takes
CrstPregeneratedStringThunks while already holding CrstNativeImageEagerFixups.
Both were assigned the same rank (level 7) because neither declared an ordering
relative to the other, so the nested acquisition trips the Crst level
consistency check ("Can't take level 7 lock ... because you already holding
level 7 lock ...").
Declare NativeImageEagerFixups AcquiredBefore PregeneratedStringThunks in
CrstTypes.def and regenerate crsttypes_generated.h (NativeImageEagerFixups is
now level 8, above PregeneratedStringThunks at 7), making the nested
acquisition well-ordered.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts CoreCLR Crst lock-order metadata to establish a defined ordering between NativeImageEagerFixups and PregeneratedStringThunks, preventing checked-build Crst level violations when both locks are acquired on the WASM R2R path.
Changes:
- Updates
CrstTypes.defto specifyNativeImageEagerFixupsis acquired beforePregeneratedStringThunks(and still beforeUnresolvedClassLock). - Regenerates
crsttypes_generated.h, updatingCrstNativeImageEagerFixups’s computed level from 7 to 8 to satisfy the new ordering constraint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/inc/CrstTypes.def | Adds the AcquiredBefore PregeneratedStringThunks dependency for NativeImageEagerFixups to define lock acquisition order. |
| src/coreclr/inc/crsttypes_generated.h | Regenerated Crst level map reflects the new ordering by bumping CrstNativeImageEagerFixups from level 7 to 8. |
Contributor
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
davidwrighton
approved these changes
Jul 28, 2026
Member
Author
|
/ba-g arm64 failure is infra |
This was referenced Aug 3, 2026
radekdoulik
added a commit
that referenced
this pull request
Aug 4, 2026
## Summary CoreCLR browser-WASM cannot use `fork` or managed process creation, so runtime tests marked `RequiresProcessIsolation` were omitted from merged runners. This PR enables those tests without adding Node-specific process APIs to managed runtime code: - the merged runner emits a plan using its existing filtering and striping; - the host Bash or Batch script executes each existing per-test wrapper in a fresh child shell; - the merged runner imports the child results into its normal xUnit report. The PR also includes the runtime-test infrastructure, platform gating, and focused runtime/test fixes needed to make priority-0 and priority-1 CoreCLR browser-WASM coverage pass consistently. This reuses the existing CoreCLR `corerun` wrappers; it does not introduce a parallel `browserhost` scheduler or duplicate per-test execution semantics. ## Execution model The browser run has three phases: 1. **Plan:** the generated merged runner writes the selected out-of-process assembly paths. 2. **Execute:** the host runs each existing `.sh` or `.cmd` wrapper and records its output, exit code, pass/skip state, and skip reason. 3. **Import:** the normal merged run executes in-process tests and reports the precomputed child outcomes through the existing xUnit summary. Existing wrappers remain authoritative for test arguments, environment variables, pre/post commands, expected exit codes, and per-test timeouts. Merged-runner arguments and stripe state are cleared while child wrappers run and restored afterward. The result format is versioned and bound to a per-run token. Missing plans, wrappers, results, malformed records, or stale tokens fail as infrastructure errors. A child in which every test is skipped is reported as skipped rather than passed. Empty plan/status environment settings are treated as unset. The host orchestration marker is emitted only for CoreCLR browser-WASM merged runners that contain out-of-process tests. ## Browser-WASM enablement and fixes - **Discovery:** only buildable projects with runnable wrappers are added to merged-runner OOP plans. - **Interpreter:** `InterpreterTester` runs `Interpreter.dll` directly through the host OOP path with the required interpreter environment instead of calling `Process.Start` inside the browser. - **ReadyToRun assets:** host Crossgen2 assets are staged for cross-architecture runs, and emitted WASM components, including composite output, are validated. - **Composite ReadyToRun tests:** affected CoreCLR browser tests carry an ActiveIssue for [#131767](#131767). Their browser projects do not force Crossgen2, allowing the managed wrapper to report the skip before CoreCLR probes the unsupported composite WebCIL layout. Other targets retain their existing composite R2R coverage. - **Host paths:** Batch orchestration retains Windows paths for host file operations while exposing Unix-style absolute paths to browser managed code. - **Multithreading:** `FeatureMultithreading` is available during test project evaluation. Tests whose only requirement is managed multithreading use that global capability, so they remain excluded on single-threaded targets and become buildable when threads are enabled. - **Precise platform scope:** tests requiring background/server GC, native threads, child processes, unsupported browser APIs, mutable host filesystem behavior, or impractical interpreter workloads are gated or excluded at the narrowest applicable level. - **Shutdown lifecycle:** the CoreCLR browser shutdown wrapper is installed from Emscripten `preRun`, after native exports are bound, so explicit `Environment.Exit` values are preserved and no finalization work is scheduled while the runtime exits. - **Background GC information test:** the browser CoreCLR `GetGCMemoryInfo` test carries an ActiveIssue for [#131766](#131766), which tracks the assertion from requesting `GCKind.Background` when background GC is not compiled. This PR does not change the product behavior. - **Test assumptions:** focused adaptations remove incidental process, threading, filesystem, and host-tool assumptions while preserving the behavior each test is intended to cover. - **Post-merge CI coverage:** build 1528104 exposed 30 deterministic OOP failures after newer `main` changes activated more browser scenarios. The eager-fixup lock-order failure is fixed on `main` by [#131355](#131355). Composite WebCIL loading and background-GC information remain tracked separately by #131767 and #131766. The `Int128` field-layout correctness fix is proceeding in [#131421](#131421); other unsupported cases are gated only for CoreCLR browser-WASM with explicit re-enable prerequisites. No shipping public API is added. ## Deferred scope Per-test native assets, UCO thunk generation, and native relinking are intentionally deferred. Tests that need them remain guarded by their existing tracking issues. The remaining non-multithreading/non-relinking exclusions were audited: 41 represent recoverable coverage and 16 exercise browser-inapplicable contracts. The recoverable set and its prerequisites are tracked by [#131321](#131321), with links from each suppression site. The newly exposed native-relinking test remains tracked by [#123946](#123946). ## Validation The following complete-suite results were recorded before the two product fixes were moved to their dedicated tracking issues: | CoreCLR browser-WASM run | Total | Passed | Skipped | Failed | | --- | ---: | ---: | ---: | ---: | | Priority 0 after the `main` merge | 3,970 | 3,412 | 558 | 0 | | Priority 0 OOP subset | 485 | 408 | 77 | 0 | | Earlier full priority 0+1 validation | 14,726 | 13,850 | 876 | 0 | | Earlier priority 0+1 OOP subset | 740 | 651 | 89 | 0 | Current-head validation after removing those fixes: - rebuilt the Checked CoreCLR browser-WASM runtime and libraries with zero warnings or errors; - cleanly built all ten affected browser test projects; - ran all ten generated wrappers against the unfixed runtime: every wrapper exited with the expected status and reported #131766 or #131767 through the OOP skip-status protocol, without either runtime assertion; - verified that the shared non-R2R browser `BasicTest` remains enabled; - verified on macOS that the R2R variant still forces Crossgen2, emits and validates its composite image, and passes. Additional earlier validation: - composite and non-composite WASM ReadyToRun output; - native `NativeLibraryTests` asset staging; - plan/import success, failure, filtering, striping, stale/malformed/missing records, environment-sensitive wrappers, and all-skipped children; - unset, empty, and non-empty plan/status environment settings; - direct browser-WASM `InterpreterTester` execution under Node; - generated Bash syntax and generated Batch structure/path conversion; - Checked browser runtime rebuild plus a fresh priority-0 payload and test layout after the `main` merge; - all 22 new gates across CoreCLR browser, WASI, desktop, and Mono browser evaluations, preserving the pre-existing Mono ReadyToRun exclusions; - the complete 72-runner Node run, including the ReadyToRun, base services, JIT, Loader, and tracing work items that failed in build 1528104. | Representative CoreCLR browser library suite | Total | Passed | Skipped | Failed | | --- | ---: | ---: | ---: | ---: | | `System.Threading.Timer.Tests` | 33 | 16 | 17 | 0 | | `System.Runtime.InteropServices.JavaScript.Tests` | 476 | 474 | 2 | 0 | | `System.Reflection.Emit.Tests` (Debug) | 2,028 | 2,017 | 11 | 0 | | `System.Runtime.Tests` (Debug) | 75,726 | 75,559 | 167 | 0 | The generated Batch path has been inspected and generated successfully but still requires execution on Windows CI. > [!NOTE] > This pull request description was updated with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44f4b4ea-409c-4a52-89a0-f488a89488b9
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
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.
The WASM R2R path acquires the
NativeImageEagerFixupsandPregeneratedStringThunksCrsts without a defined acquisition order between them, which trips a Crst level violation.Add an
AcquiredBefore PregeneratedStringThunksedge toNativeImageEagerFixupsinCrstTypes.defso the two locks have a well-defined ordering, and regeneratecrsttypes_generated.h(CrstNativeImageEagerFixups level 7 -> 8). Nothing acquiresNativeImageEagerFixupsbefore another lock, so the change does not cascade to any other Crst level.Note
This pull request was authored with assistance from GitHub Copilot.