Skip to content
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

[NativeAOT] Convert operand of newarr to int when producing reverse P/Invoke array marshalling #98025

Merged
merged 4 commits into from
Feb 12, 2024

Conversation

filipnavara
Copy link
Member

Fixes the following error on linux-arm (#98022):

ILC: /home/navara/runtime/src/coreclr/jit/importer.cpp:9382
ILC: Assertion failed 'genActualTypeIsIntOrI(op2) : Possibly bad IL with CEE_newarr at offset 002Bh (op1=NULL op2=long stkDepth=0)' in 'SizeParamIndex.PInvoke.PassingByOutTest:MarshalCStyleArrayLong_AsByOut_AsSizeParamIndex(byref,byref):ubyte' during 'Importation' (IL size 147; hash 0x8bc3ebdf; FullOpts)

ILC: /home/navara/runtime/src/coreclr/jit/importer.cpp:9382
ILC: Assertion failed 'genActualTypeIsIntOrI(op2) : Possibly bad IL with CEE_newarr at offset 000Ch (op1=NULL op2=long stkDepth=0)' in 'Internal.CompilerGenerated.<Module>:ReverseDelegateStub__DelUlongArrByRefAsCdeclCaller(uint,uint):int' during 'Importation' (IL size 204; hash 0x41204394; FullOpts)

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 6, 2024
@MichalStrehovsky
Copy link
Member

Most uses of EmitElementCount seem to assume int32 though. I think the bug is that we have a couple (two that I saw) places that store it into an nint local. But then seem to still use it as int32.

Weird that this shows up as a problem on 32bit platform. I would expect it to show up on 64bit due to size differences.

@filipnavara
Copy link
Member Author

filipnavara commented Feb 6, 2024

The IL opcode newarr is documented to take native int from the stack (https://learn.microsoft.com/en-us/dotnet/api/system.reflection.emit.opcodes.newarr?view=net-8.0):

The number of elements in the new array should be specified as a native int.

On 64-bit platforms it happens to work with any size of int but on 32-bit ones this gets hit for ulong out size parameter.

I can change it to conv.i4 or conv.ovf.i4 if you prefer. The array size cannot be bigger anyway.

@MichalStrehovsky
Copy link
Member

The IL opcode newarr is documented to take native int from the stack

Huh, that's weird. ECMA-335 spec says int32 or nint and normally one would see int32.

Here we store the result into int32:

// loads the number of elements
EmitElementCount(codeStream, MarshalDirection.Forward);
codeStream.EmitStLoc(vLength);

If I'm reading it right, coreclr will convert to i4 with overflow checking:

pslILEmit->EmitCONV_OVF_I4();

@filipnavara
Copy link
Member Author

If I'm reading it right, coreclr will convert to i4 with overflow checking:

Let's match CoreCLR then.

@am11
Copy link
Member

am11 commented Feb 6, 2024

On 64-bit platforms it happens to work with any size of int but on 32-bit ones this gets hit for ulong out size parameter.

Nice quote to have as a code comment! 😉

@filipnavara filipnavara changed the title [NativeAOT] Convert operand of newarr to native int when producing reverse P/Invoke array marshalling [NativeAOT] Convert operand of newarr to int when producing reverse P/Invoke array marshalling Feb 7, 2024
@MichalStrehovsky MichalStrehovsky merged commit e7b6d06 into dotnet:main Feb 12, 2024
107 of 110 checks passed
@filipnavara filipnavara deleted the il_array_marshal_32b branch February 12, 2024 06:57
@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Interop-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants