Repository navigation
[wasm][R2R] Fix null ReturnTypeDesc dereference in SpillStructCallResult - #134981
Merged
Merged
Conversation
Wasm has FEATURE_MULTIREG_RET == 0, so GenTreeCall::GetReturnTypeDesc() returns nullptr. Spilling a struct call result that is returned in a single register but whose destination local has no primitive register type (e.g. a struct wrapping Vector128<T>, returned as v128) dereferenced it and crashed crossgen2. Fixes #134976 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 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
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
The function is no longer only reached for 3/5/6/7-byte returns; on Wasm it also handles single-register struct returns (e.g. v128) whose layout has no primitive register type. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
lewing
added a commit
that referenced
this pull request
Oct 1, 2026
…n Wasm (#134984) ## Motivation a17debe (#129494) excluded Wasm from the 16-byte `TYP_SIMD16` case in `ClassLayout::GetRegisterType()` while Wasm SIMD16 local loads were still NYI. Those SIMD NYIs are no longer present in `src/coreclr/jit/codegenwasm.cpp`. Because of the exclusion, on Wasm a struct wrapping `Vector128<T>` that is returned as `v128` (call typed `simd16`) and stored to a `TYP_STRUCT` local has `lclRegType == TYP_UNDEF`, so `Lowering::LowerStoreLocCommon` routes it through `SpillStructCallResult`: a new do-not-enregister temp, a `STORE_LCL_FLD simd16` into it, then a struct copy into the destination. That path also crashed crossgen2 with a null `ReturnTypeDesc` dereference (#134976, fixed separately in #134981). This PR removes the reason Wasm takes that path for v128 returns. ## Change In `ClassLayout::GetRegisterType()` (`src/coreclr/jit/layout.h`), change `#if defined(FEATURE_SIMD) && !defined(TARGET_WASM)` back to `#ifdef FEATURE_SIMD`, so 16-byte struct layouts get `TYP_SIMD16` as their register type on Wasm as on other SIMD targets. ## Validation Performed in a sibling worktree: browser-wasm Checked, osx-arm64 host crossgen2, with the #134981 fix also applied. - `./build.sh -s clr+libs -os browser -c checked -lc release`: succeeded - `src/tests/build.sh -browser checked priority1 -test:JIT/HardwareIntrinsics/HardwareIntrinsics_General_r.csproj -test:JIT/HardwareIntrinsics/HardwareIntrinsics_General_ro.csproj /p:LibrariesConfiguration=Release`: succeeded - `src/tests/run.sh wasm checked --runcrossgen2tests --node --runner-filter=HardwareIntrinsics_General` (IL-CG2 dirs and .wasm images were deleted first to force recompilation): - `HardwareIntrinsics_General_r`: 2584/2584 passed - `HardwareIntrinsics_General_ro`: 2551/2551 passed - JitDump of `VectorImmBinaryOpTest__op_LeftShiftByte1:RunStructLclFldScenario` (Vector128_1_r): the JIT no longer creates the `Return value temp` spill and stores the `simd16` call result directly. - R2R image sizes: - `Vector128_1_r.wasm`: 4,957,655 → 4,951,047 bytes (-6.6 KB) - `Vector128_1_ro.wasm`: 4,926,510 → 4,919,838 bytes (-6.7 KB) On this branch, `./build.sh clr.wasmjit -c checked` succeeded. ## Caveat Validation covered only the HardwareIntrinsics General runners. This change affects every 16-byte struct local on Wasm, so it needs broader Wasm R2R coverage (e.g. the outerloop R2R_CG2 browser-wasm lane) and sign-off from the Wasm JIT owners. Related to #134976 and #134981 > [!NOTE] > This PR description was drafted with GitHub Copilot assistance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jakobbotsch
approved these changes
Oct 1, 2026
jakobbotsch
left a comment
jakobbotsch
left a comment
Member
There was a problem hiding this comment.
Seems reasonable.
I would like to clean things up so that the return ABI info is always available in GenTreeCall, at which point wasm should also put something in there, but that is a separate task.
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
2 participants
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.
Wasm defines
FEATURE_MULTIREG_RETas 0, soGenTreeCall::GetReturnTypeDesc()returnsnullptr.Lowering::SpillStructCallResultdereferenced it unconditionally to get the single return field offset.This path is reached on Wasm when a struct call result is returned in a single value but the destination local can't be retyped to a primitive. For example,
TestStructwraps aVector128<byte>, is returned asv128/simd16, and is stored into aTYP_STRUCTlocal. crossgen2 then crashed: an abort (exit 134) on Linux Helix, and a spin in the PAL fault dispatch on macOS.The fix uses offset 0 when multi-reg returns aren't supported.
GetSingleReturnFieldOffset()returns the same value on every target except RISC-V and LoongArch.Validation
VectorImmBinaryOpTest__op_LeftShiftByte1:RunStructLclFldScenario. After the fix, all assemblies inHardwareIntrinsics_General_rand_rocompile../build.sh -s clr+libs -os browser -c checked -lc release, thensrc/tests/build.sh -browser checked priority1 -test:JIT/HardwareIntrinsics/HardwareIntrinsics_General_r.csproj -test:JIT/HardwareIntrinsics/HardwareIntrinsics_General_ro.csproj /p:LibrariesConfiguration=Release, thensrc/tests/run.sh wasm checked --runcrossgen2tests --node --runner-filter=HardwareIntrinsics_General:HardwareIntrinsics_General_r: 2584/2584 passedHardwareIntrinsics_General_ro: 2551/2551 passedsrc/coreclr/scripts/jitformat.py: no changes.Resolves #134976
Note
This PR was drafted with GitHub Copilot assistance.