Skip to content

Preserve JIT handle bits in cross-target builds#129034

Open
MichalStrehovsky wants to merge 3 commits into
mainfrom
fix/jit-cross-target-handle-bits
Open

Preserve JIT handle bits in cross-target builds#129034
MichalStrehovsky wants to merge 3 commits into
mainfrom
fix/jit-cross-target-handle-bits

Conversation

@MichalStrehovsky
Copy link
Copy Markdown
Member

Fixes #122013

Ensure RyuJIT keeps host-sized handle values when cross-targeting from 64-bit to 32-bit builds.

Sending for consideration. This is obviously AI generated, but I don't like we have this bug open for so long.

Cc @EgorBo

MichalStrehovsky and others added 2 commits June 5, 2026 14:27
Ensure RyuJIT keeps host-sized handle values when cross-targeting from 64-bit to 32-bit builds.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 5, 2026 05:34
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 5, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts RyuJIT’s constant/handle plumbing so that, in cross-target scenarios (e.g., 64-bit host compiling 32-bit target), handle values are treated as host-pointer-sized where required, avoiding truncation of the upper bits during constant import/propagation and preventing incorrect folding.

Changes:

  • Update static-readonly constant import to request sizeof(ssize_t) bytes for TYP_REF so gtNewGenericCon(TYP_REF, ...) reads a complete host-sized handle.
  • Tighten/extend handle constant preservation in JIT assertion propagation and constant bashing so handle VNs don’t lose upper bits on 32-bit targets.
  • Re-enable/strengthen coverage by removing test workarounds/skips that were masking the issue.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/tests/Regressions/coreclr/15647/interfacestatics.ilproj Removes TrimMode workaround so the test exercises the problematic optimization again.
src/tests/JIT/Methodical/jitinterface/bug603649.cs Re-enables the test by removing the ActiveIssue skip.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Adds a DEBUG invariant check to catch truncated/invalid handles early.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs Adjusts getStaticFieldContent object-handle buffer handling for cross-target correctness.
src/coreclr/jit/importer.cpp Requests host-sized bytes for TYP_REF static-readonly constants to avoid truncation.
src/coreclr/jit/compiler.hpp Avoids incorrect debug-time “fits in int32” assertions for GC-typed constants.
src/coreclr/jit/assertionprop.cpp Preserves host-sized handle constants/printing when VNs represent handles.

Comment on lines 2378 to +2385
case FrozenObjectNode:
if ((valueOffset == 0) && (bufferSize == targetPtrSize))
if (isObjectHandleBuffer)
{
// save handle's value to buffer
nint handle = ObjectToHandle(data);
new Span<byte>(&handle, targetPtrSize).CopyTo(new Span<byte>(buffer, targetPtrSize));
Span<byte> destination = new Span<byte>(buffer, bufferSize);
destination.Clear();
new ReadOnlySpan<byte>(&handle, Math.Min(sizeof(nint), bufferSize)).CopyTo(destination);
Comment on lines +3987 to 3990
const int fieldValueSize = (fieldType == TYP_REF) ? (int)sizeof(ssize_t) : genTypeSize(fieldType);
assert(bufferSize >= fieldValueSize);
if (info.compCompHnd->getStaticFieldContent(field, buffer, fieldValueSize))
{
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 8, 2026

🤖 Copilot Code Review — PR #129034

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: Real bug, well-documented in #122013. When cross-compiling from a 64-bit host to a 32-bit target, handle values (host-process-sized ssize_t) were truncated to target pointer size (int), losing the debug high-bit marker (0x4000000000000000) and causing incorrect constant folding. The test failures in bug603649 and interfacestatics are directly caused by this.

Approach: Multi-site fix that addresses the specific paths where truncation occurs: the importer's getStaticFieldContent call, the assertion propagation constant extraction and bashing, the BashToConst assertion, and the managed-side getStaticFieldContent implementation. The defensive Debug.Assert in HandleToObject is a strong addition suggested by @jkotas in the issue discussion.

Summary: ⚠️ Needs Human Review. The fix for the primary data flow (impImportStaticReadOnlyFieldgetStaticFieldContentVNForGenericCon → assertion prop) is correct and addresses the reported test failures. However, I've confirmed a concrete concern about the isObjectHandleBuffer logic that could silently return truncated handles to the JIT through an alternate code path (fgValueNumberConstLoad). A JIT expert should assess whether this secondary path is reachable in practice and whether it needs to be fixed in this PR or as a follow-up.


Detailed Findings

✅ Primary fix path — Correct end-to-end

The chain from impImportStaticReadOnlyField (requesting sizeof(ssize_t) bytes for TYP_REF) → managed getStaticFieldContent (accepting sizeof(nint) buffers via isObjectHandleBuffer) → gtNewGenericCon (reading ssize_t from the buffer) is correct. The full 64-bit handle is preserved throughout.

Verified in ConstantValueInternal (valuenum.h line 1257–1260): for handle VNs (CEA_Handle), the return is (T) VNHandle.m_cnsVal where m_cnsVal is ssize_t. So ConstantValue<ssize_t> correctly preserves the full value, while ConstantValue<int> truncates. The PR's change in optIsTreeKnownIntValue to use ssize_t for handles is therefore correct.

BashToConst assertion relaxation

Skipping FitsIn<int32_t> for GC types is correct. In cross-targeting, a host-sized (64-bit) handle stored as TYP_REF on a 32-bit target would fail the assertion despite being a valid handle value.

HandleToObject debug assertion

Good addition. Directly catches any code path that truncates the high bit before reaching the managed side. This was specifically suggested in the issue discussion.

✅ Test changes

Removing [ActiveIssue] and TrimMode=partial workarounds is correct — they were masking this bug.

⚠️ isObjectHandleBuffer accepting targetPtrSize can return truncated handles

In CorInfoImpl.RyuJit.cs (line 2349–2350):

bool isObjectHandleBuffer =
    (valueOffset == 0) && ((bufferSize == targetPtrSize) || (bufferSize == sizeof(nint)));

When bufferSize == targetPtrSize (4 bytes on a 32-bit target with a 64-bit host), the FrozenObjectNode copy logic at line 2385:

new ReadOnlySpan<byte>(&handle, Math.Min(sizeof(nint), bufferSize)).CopyTo(destination);

copies only Math.Min(8, 4) = 4 bytes of the handle. On little-endian, this is the low 4 bytes. The debug high-bit marker (0x4000000000000000) lives in the upper 4 bytes and is silently lost. The method returns true, claiming success, but the buffer contains a truncated handle.

The primary caller (impImportStaticReadOnlyField) now passes sizeof(ssize_t) (8 bytes) so it's fine. But fgValueNumberConstLoad (valuenum.cpp line 12949) still passes genTypeSize(tree->TypeGet()) = TARGET_POINTER_SIZE = 4 for TYP_REF, and it allows TYP_REF trees (line 12942: !tree->TypeIs(TYP_BYREF, TYP_STRUCT)). If this path is reachable for a static readonly object field in cross-targeting, it would get a truncated handle, and VNForGenericCon(TYP_REF, buffer) would create a handle VN with the wrong value.

I think the FrozenObjectNode case should return false when bufferSize < sizeof(nint) rather than returning truncated data. This would prevent the JIT from using a corrupted constant, while allowing null (value == null) to still be returned since all-zeros is correct for null regardless of buffer size. Alternatively, fgValueNumberConstLoad should pass sizeof(ssize_t) for TYP_REF.

This is the finding I'm most uncertain about — I believe it's a real truncation risk, but a JIT expert should confirm whether the fgValueNumberConstLoad → getStaticFieldContent path is actually reachable for TYP_REF static readonly fields in cross-targeting scenarios. If impImportStaticReadOnlyField always folds these constants during import, this VN-time path may never fire for the affected field types.

⚠️ TYP_INT branch in optAssertionPropGlobal_RelOp (line 4630–4637)

The TYP_INT branch still uses ConstantValue<int>(vnCns) without handle awareness:

op1->BashToConst(vnStore->ConstantValue<int>(vnCns));

For a TYP_INT handle VN in cross-targeting, ConstantValue<int> returns (int)(ssize_t)m_cnsVal, truncating the 64-bit handle. The subsequent IsVNHandle check sets flags but operates on the already-truncated value.

This affects non-object handles (class handles, method handles, etc.) which are TYP_INT on a 32-bit target. A JIT expert should evaluate whether these handle VNs can reach this assertion prop path. If they can, the same ConstantValue<ssize_t> fix should be applied here.

💡 Debug printf format for TYP_INT (line 4588)

The TYP_INT debug printf still uses ConstantValue<int> and %d format:

printf("%d\n", vnStore->ConstantValue<int>(vnCns));

While this is only debug output, it would print the truncated value for handles, making debugging misleading. Not blocking, but worth fixing for consistency with the TYP_REF/TYP_BYREF printf fixes in this same PR.

💡 Repeated pattern could use a helper (follow-up)

The pattern vnStore->IsVNHandle(vnCns) ? vnStore->ConstantValue<ssize_t>(vnCns) : static_cast<ssize_t>(vnStore->ConstantValue<size_t>(vnCns)) appears 4 times in assertionprop.cpp. Consider extracting a helper in a follow-up PR to reduce duplication and prevent inconsistency.

Generated by Code Review for issue #129034 · ● 8.6M ·

@MichalStrehovsky
Copy link
Copy Markdown
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RyuJIT losing top bits of handles

2 participants