-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
On 64 bit platforms, "stelem.ref" and "ldelema" ignore the high bits of a native int index #71571
Conversation
…of a native int index Fix dotnet#52817
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update Mono too?
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs
Outdated
Show resolved
Hide resolved
We had the same problem for NewArray helper some time ago and we fixed it there by changing the NewArray helper to take native int. Would it be better to do the same thing here to avoid the code duplication? For R2R, we have introduced a new compat band in .NET 7, so it is ok to make JIT helper breaking changes without doing anything special about it. |
…gh bits of a native int index" This reverts commit 47eba9a.
@@ -830,7 +830,7 @@ private static unsafe void StelemRef_Helper(ref object element, MethodTable* ele | |||
} | |||
|
|||
ref object rawData = ref Unsafe.As<byte, object>(ref Unsafe.As<RawArrayData>(array).Data); | |||
return ref Unsafe.Add(ref rawData, index); | |||
return ref Unsafe.Add(ref rawData, (int)index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is Unsafe.Add
overload that takes IntPtr
. It should not be necessary to cast the index
to int
here. It just going to add an extra unnecessary instruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the problem is that the "minimal runtime" does not compile without the cast, just add the necessary method to the minimal Unsafe used by the minimal runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise. Thank you!
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
Fixes #52817