Skip to content

Conversation

janvorli
Copy link
Member

The RhNewString length argument is declared as int, but the asm implementation was using the full 64 bits of the related argument register. That doesn't match the calling convention where there is no guarantee that the upper 32 bits are zeroed. That has caused problem in the interpreter that always loads full registers from the interpreter stack and so sometimes, the upper 32 bits were a garbage and the function failed because the length was seemingly too large.

This change fixes it by changing signature of the RhNewString so that the length is nint in managed code / intptr_t in native code.

The RhNewString length argument is declared as int, but the asm
implementation was using the full 64 bits of the related argument
register. That doesn't match the calling convention where there is no
guarantee that the upper 32 bits are zeroed. That has caused problem in
the interpreter that always loads full registers from the interpreter
stack and so sometimes, the upper 32 bits were a garbage and the
function failed because the length was seemingly too large.

This change fixes it by changing signature of the RhNewString so that
the length is nint in managed code / intptr_t in native code.
@janvorli janvorli added this to the 10.0.0 milestone Jul 11, 2025
@janvorli janvorli requested a review from jkotas July 11, 2025 18:10
@janvorli janvorli self-assigned this Jul 11, 2025
@Copilot Copilot AI review requested due to automatic review settings July 11, 2025 18:10
Copy link
Contributor

@Copilot 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 updates the RhNewString API to use pointer‐sized length arguments throughout the managed, native, and assembly layers, preventing upper‐32‐bit garbage from invalidating string lengths.

  • Change RhNewString and RhNewString_UP FCDECL signatures in jitinterface.h from DWORD to INT_PTR
  • Update managed extern and helper methods (RhNewString, FastAllocateString) to use nint
  • Adjust native implementation (portable.cpp) to use intptr_t and correct AMD64 assembly comments to reference the 64-bit register

Reviewed Changes

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

Show a summary per file
File Description
src/coreclr/vm/jitinterface.h Changed RhNewString signatures to INT_PTR and removed obsolete alloc declarations
src/coreclr/runtime/amd64/AllocFast.asm Updated comment to RDX for the 64-bit length register
src/coreclr/runtime/amd64/AllocFast.S Updated comment to RSI for the 64-bit length register
src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs Updated extern signature to nint
src/coreclr/nativeaot/System.Private.CoreLib/src/System/String.NativeAot.cs Updated FastAllocateString parameter to nint
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs Updated extern signature to nint
src/coreclr/nativeaot/Runtime/portable.cpp Changed FCIMPL signature to intptr_t for length
Comments suppressed due to low confidence (3)

src/coreclr/nativeaot/Runtime/portable.cpp:130

  • Consider adding unit or interop tests that pass large and negative intptr_t lengths to RhNewString to verify proper truncation or error handling, ensuring this change is fully covered.
FCIMPL2(String *, RhNewString, MethodTable * pArrayEEType, intptr_t numElements)

src/coreclr/runtime/amd64/AllocFast.asm:131

  • [nitpick] Only AMD64 comments were updated here; please verify and update corresponding length‐register comments on other architectures (e.g., ARM64) to keep documentation consistent.
;;  RDX == character/element count

@janvorli janvorli merged commit b58e37d into dotnet:main Jul 14, 2025
100 checks passed
@janvorli janvorli deleted the fix-rhnewstring branch July 14, 2025 15:02
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants