Fix il2cpp error (take 2)

[mtmk]

Previous commit about this issue reverted since it wasn't fixing the issue. The actual error was:

Error: IL2CPP error for method 'System.Char[] NATS.Client.Core.Internal.NuidWriter::Refresh(System.UInt64&)' in assembly NATS.Client.Core.dll
System.ArgumentOutOfRangeException:
 Cannot create a constant value for types of System.UIntPtr for System.UIntPtr NATS.Client.Core.Internal.NuidWriter::NuidLength (Parameter 'declaredParameterOrFieldType')

With this fix now it compiles fine:

Looking at Unsafe.Add examples passing in uint or int instead of nuint is fine:

e.g. https://github.com/dotnet/runtime/blob/a7efcd9ca9255dc9faa8b4a2761cdfdb62619610/src/libraries/System.Private.CoreLib/src/System/Buffer.cs#L84

Note: I had to revert the last two commits (so ignore the previous four commits) to main which was done by mistake without PRs.

cc @galvesribeiro

[to11mtm]

For the sake of general review and future doc ; what are all of the 'conditions' we had to fix or deal with?

I think this would be good for general doc as well as putting together for future 'contributor guidelines' to help with knowing how to do the needful with IL2CPP

As it stands, I don't have bandwidth to test locally so will refrain from any approval but will take a look. <3

[to11mtm]

Looks OK but per other comments we should doc what we are learning along the way.

[to11mtm]

TBH I think this is probably better for comprehension sake anyway (i.e. scoping/etc)

[mtmk]

To elaborate a bit more: in the previous PR #543 I wrongly thought the IL2CPP error was caused by the inner method local reuse and my oversight in testing in Unity for some reason I thought it was fixing the issue. But after the preview release trying it again in a fresh unity project I realized the compile error was still there for Linux IL2CPP builds. Looking at the error more closely it looks like IL2CPP can't deal with nuint (UIntPtr) constants. Changing the type to uint instead fixed the error and it looked like passing uint instead of nuint to Unsafe.Add() calls did not make any difference in tests, also found example usages in dotnet/runtime which used int instead.

Also to note, fairly extensive tests for NuidWrite are passing as well:

  Passed NATS.Client.Core.Tests.NuidWriterTests.AllNuidsAreUnique_SmallSequentials [18 s]
  Passed NATS.Client.Core.Tests.NuidWriterTests.Only_last_few_digits_change [< 1 ms]
  Passed NATS.Client.Core.Tests.NuidWriterTests.NewInbox_NuidAppended(prefix: null) [< 1 ms]
  Passed NATS.Client.Core.Tests.NuidWriterTests.NewInbox_NuidAppended(prefix: "__INBOX") [< 1 ms]
  Passed NATS.Client.Core.Tests.NuidWriterTests.NewInbox_NuidAppended(prefix: "long-inbox-prefix-above-stackalloc-limit-of-64") [1 ms]
  Passed NATS.Client.Core.Tests.NuidWriterTests.NewInbox_NuidAppended(prefix: "") [< 1 ms]
  Passed NATS.Client.Core.Tests.NuidWriterTests.GetNextNuid_PrefixRenewed_Char [1 ms]
  Passed NATS.Client.Core.Tests.NuidWriterTests.GetNextNuid_BufferToShort_False_Char [1 ms]
  Passed NATS.Client.Core.Tests.NuidWriterTests.GetPrefix_PrefixAsExpected [< 1 ms]

As shown above in the main description local testing of Unity build completed successfully proving the fix works.

[galvesribeiro]

Looking at the error more closely it looks like IL2CPP can't deal with nuint (UIntPtr) constants.

Yeah, I think I saw something related to this when nuint was introduced. I think it is because of the runtime version Unity uses which doesn't have support to it and since IL2CPP is tied to it, it would fail.

[mtmk]

@to11mtm, @stebet, @rickdotnet, @caleblloyd sorry to pull you all in, it'd be good to have more eyes on this PR. It's a simple change but because of my wrong fix then a revert it became a little messy.

Tha change is very simple actually. NuidLength type is changed from nuint to uint to keep mono/unity il2cpp compilation happy. So the impact afaik, is where NuidLength type affecting the use of Unsafe.Add<char>(ref char source, nuint elementOffset) in this loop.

https://github.com/nats-io/nats.net.v2/blob/aceae9d4b902ecfc8e237606e3703ce1d96bc699/src/NATS.Client.Core/Internal/NuidWriter.cs#L74-L80

Before the change second parameter of Unsafe.Add nuint elementOffset had the exact matching type but now it is converted from uint to nuint. Now I don't have a lot of experience with intptrs but given the value of NuidLength is a const 22 (it's not like it can go near type bounds i.e. int.MaxValue) even if the platform JIT targeting is 32-bit I'm guessing we should be OK. Adding to that I did find examples of Unsafe.Add usage in dotnet/runtime which is using int as the second param:

https://github.com/dotnet/runtime/blob/a7efcd9ca9255dc9faa8b4a2761cdfdb62619610/src/libraries/System.Private.CoreLib/src/System/Buffer.cs#L83-L85

I feel this is good since tests are passing as well. Not sure how much more validation we can do. Let me know what you think.

[galvesribeiro]

LGTM

[jasper-d]

@mtmk This looks good too :)

The change from nuint to uint adds an extra move inside the for loop (x64), but that is hardly measurable because the multiplications inside the loop are dominating.