Implicit byref arguments are never on the GC heap - #133304
Conversation
Implicit byref argument storage must live outside the GC heap on all architectures using this convention, so callees can store into these arguments without write barriers. Bumps READYTORUN_MAJOR_VERSION to 29: crossgen2 now emits barrier-free stores, so older runtimes must reject these images. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2871fa5f-63ee-4a17-9721-6c4895a014b3
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
🟡 Changes recommended
The behavioral code-generation change lacks targeted regression coverage for write-barrier omission and GC reporting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the implicit-byref ABI contract so argument storage is guaranteed outside the GC heap, allowing the JIT to omit write barriers safely.
Changes:
- Marks implicit-byref stores as non-heap and updates promotion costing.
- Copies debugger func-eval arguments into GC-protected native memory.
- Documents the contract and advances ReadyToRun to version 29.
File summaries
| File | Description |
|---|---|
src/coreclr/vm/callhelpers.h |
Documents caller ownership and GC-reporting requirements. |
src/coreclr/vm/callhelpers.cpp |
Asserts implicit-byref arguments are not heap-backed. |
src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs |
Advances managed R2R version. |
src/coreclr/nativeaot/Runtime/inc/ModuleHeaders.h |
Advances NativeAOT R2R version. |
src/coreclr/jit/promotion.cpp |
Removes obsolete write-barrier costing. |
src/coreclr/jit/morph.cpp |
Marks implicit-byref stores as non-heap. |
src/coreclr/jit/lclvars.cpp |
Documents the new storage invariant. |
src/coreclr/inc/readytorun.h |
Advances and documents R2R version 29. |
src/coreclr/debug/ee/funceval.cpp |
Creates protected native copies for func-eval arguments. |
docs/design/coreclr/botr/clr-abi.md |
Documents the revised ABI contract. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
Reword the implicit-byref section so it does not lead with the Windows AMD64 calling convention, and move the AMD64 return-buffer-in-RAX note into the "Return buffers" section. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2871fa5f-63ee-4a17-9721-6c4895a014b3
The Blob Service "List Blobs" API pages its results, and the first page is not guaranteed to cover the whole prefix. Since the listing is ordered by blob name, a truncated first page drops entire target directories, which produced zero partitions for those targets and failed the Helix send with "SendHelixJob given no WorkItems to send". Follow the NextMarker until the listing is exhausted, and log the blob count so a truncated listing is visible in the setup log. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2871fa5f-63ee-4a17-9721-6c4895a014b3
|
Diffs aren't too motivating. Perhaps, a bit more can be improved with proper alias analysis (implicit byref never aliases with gc heap), but closing for now. |
Resolve ReadyToRun version conflicts by advancing the implicit-byref ABI change to version 30.0 while preserving main's 29.x changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restore superpmi_diffs_setup.py to main so this PR does not modify Python files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e3cbbf9d-9a2b-4a49-a4a9-905b60d41f15
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The ABI documentation misstates ARM64 exceptions, and the PR description names the wrong ReadyToRun major version.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (3)
Resolved since last review (1)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e3cbbf9d-9a2b-4a49-a4a9-905b60d41f15
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The ABI-wide code-generation change lacks targeted regression coverage, and its architecture table omits WebAssembly.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
Add WebAssembly ABI constraints for non-unwrappable structures. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@jkotas @jakobbotsch anything else here? I assume it fixes a potential gc hole we theoretically had (see thread ^), closes #133961 and removes a couple of write barriers. Follow up: stop representing it via BYREF (#82166) - this actually had some improvements and we can benefit from alias analysis: implicit byrefs never alias with GC heap objects and return buffer |
Implicit byref args are never on the GC heap (#133304), so type them as `TYP_I_IMPL` instead of `TYP_BYREF` (no GC reporting); async methods keep `TYP_BYREF` since their storage changes on resumption. Treat return buffers as non-null in `fgAddrCouldBeNull`, and use `fgAddrCouldBeHeap` (now aware of implicit byrefs) for write barrier selection. Contributes to #82166 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5bb92e6b-deb1-4963-9107-87559f446347


Implicit byref arguments (structs passed by reference under the hood: not 1/2/4/8 bytes on win-x64, larger than 16 bytes on arm64) are now guaranteed to never live on the GC heap, so callees can store into them without write barriers.
Bumps
READYTORUN_MAJOR_VERSIONto 30: crossgen2 now emits barrier-free stores, so older runtimes must reject these images.MINIMUM_READYTORUN_MAJOR_VERSIONstays at 26 - existing images can only forward an incoming implicit byref, never originate heap-backed storage for one.Fixes #133961