IndexOutOfRangeException in System.Numerics.Vector<T>(array) Where array Size < 32 Bytes

[zcanann]

System.Numerics.Vector<byte> vec = new System.Numerics.Vector<byte>(new byte[] { 4, 3, 2, 1, 6, 4, 2, 4 });

This call will crash with an IndexOutOfRangeException exception. I'm pulling in this library as a NuGet package. If I pass in >= 32 bytes, this code works.

The same logic applies to other data types if they do not fill enough at least 32 bytes.

Is there a reason for this behavior? My use case involves passing variable length bytes (anywhere from 1 to millions). The constructor does not make this behavior clear, and I'd prefer to not have to pad my byte arrays to use this.

If the library is not supposed to support values less than the size of the XMM/YMM register, then it would be ideal to at least throw a more specific exception.

[benaadams]

System.Numerics.Vector<byte>.Count should tell you the size it supports (which varies based on CPU) it always chooses the largest supported vector size

[zcanann]

Perhaps an exception with more information would be useful then (ie expecting x bytes)

IndexOutOfRangeException is a bit vague for first-time users of this library.

EDIT: Alternatively, address https://github.com/dotnet/corefx/issues/23448 to improve the clarity of what constructor calls do.

[terrajobst]

Normally I'd expect the constructor to throw an ArgumentException if the array's length is less than than Vector<T>.Count. I don't recall whether we explicitly don't do length validation for perf reasons. Also, I don't recall whether the constructor is an intrinsic or not.

@CarolEidt, do you have any thoughts here?

EDIT: @eerhardt just pointed me to the code. Looks intentional, but we do have validation so we could easily change the exception. Changing the exception type is a binary breaking change, not sure it's worth it. We should, however, update the message to explain the error condition.

[eerhardt]

The issue is that the constructor that takes only an array calls into the constructor that takes an array and an index. So that's where the "index" in "IndexOutOfRangeException" comes from -

https://github.com/dotnet/corefx/blob/075df17be2057be1d47f11597f0ee735cb39f60b/src/System.Numerics.Vectors/src/System/Numerics/Vector.tt#L186-L189

Perhaps just adding an appropriate message to this exception would suffice?

[zcanann]

Adding a message seems like a sufficient solution to me

[CarolEidt]

Adding a message seems like a sufficient solution to me

Agreed.

[WinCPP]

May I pick this up for the changes?

To test how it works, I did some change to pass a string "myexception" to the constructor of IndexOutOfRangeException as mentioned in this note by @eerhardt on the thread. However the change doesn't seem to have effect. It still puts out the string "Index was outside the bounds of the array." as exception message; i.e. the default exception message. Seems like my changes to the 'tt' file and the corresponding auto-generated file that has compiled into the assembly aren't being picked up.

I am not sure if I have followed correct step... Help appreciated.

[clarkis117]

@WinCPP You need to build the T4 templates in Visual Studio, so it generates a new .cs file with the changes you made. For the tests, this may involve copying the ...includes.tt file to same directory as the test.tt file. Otherwise, Visual Studio will give you a build error when attempting to build the T4 templates in the solution.

[WinCPP]

@clarkis117 Thanks. I am working on #23688 at this time and would like to take this up next.

@clarkis117 Now I remember, that time I had done the changes to the tt file itself but found that the somehow the generic message in the IndexOutOfRangeException was not getting overridden. It has been sometime that I worked on it. Will redo everything. Thanks!

[jkotas]

Seems like my changes to the 'tt' file and the corresponding auto-generated file that has compiled into the assembly aren't being picked up.

The implementation of [Intrinsics] methods is duplicated in the JIT. The code in .cs files is just a fallback for cases where the JIT intrinsic expansion does not kick in (e.g. when the method is called via reflection, or on platforms where the SIMD intrinsics are not implemented yet). It means that the change of exception message has to be done in the JIT to make it actually kick in.

[WinCPP]

The implementation of [Intrinsics] methods is duplicated in the JIT. The code in .cs files is just a fallback for cases where the JIT intrinsic expansion does not kick in (e.g. when the method is called via reflection, or on platforms where the SIMD intrinsics are not implemented yet). It means that the change of exception message has to be done in the JIT to make it actually kick in.

Is this something that I can do or is it offlimits for me...? I am interested...

[jkotas]

It is not one liner change, but I am sure that you would be able to do it if you dive into it. There is pretty active community around the JIT intrinsics ( @4creators @CarolEidt @eerhardt @fiigii @sdmaclea @tannergooding ) that I am sure will be happy to give you guidance as necessary.

[fiigii]

@WinCPP JIT uses a GenTreeBoundsChk IR (with GT_SIMD_CHK oper) to throw the exception.

https://github.com/dotnet/coreclr/blob/aaafa705f6aa930343061a0753928839b1e75bf2/src/jit/simd.cpp#L2886-L2906

I guess this change may need to add a new value in enum SpecialCodeKind because the current SCK_RNGCHK_FAIL has the fixed error message.

[4creators]

Another piece of code to dive into is here:

https://github.com/dotnet/coreclr/blob/aaafa705f6aa930343061a0753928839b1e75bf2/src/jit/simd.cpp#L2647-L2664

What is interesting this are types of checks as above SCK_RNGCHK_FAIL and SCK_ARG_EXCPN

[WinCPP]

@fiigii @4creators now taking a look into this. Thanks for inputs.

[MarcoRossignoli]

@jkotas is there some docs on this?I mean [Intrinsic], *.Portable.cs conventions, debug System.Private.CoreLib etc...i'm very interested to go deeper and help more...but sometimes is a bit cryptic where put the hands(first reason coreclr/corefx are huge code bases with a lot of tecnologies...and this is amazing, the best school). For instance i like to work on System.Private.CoreLib.sln .net side but i haven't found on guide how to 'dev/attach debug' code for that part, i undestood that the tests are all on CoreFx so after write code we need to test with local build. Sorry to all for noise on this issue...but like @WinCPP i did some experiments with same results 😅

[jkotas]

[Intrinsic]

https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Runtime/CompilerServices/IntrinsicAttribute.cs has the two line version of the documentation. Comment in front of Compiler::impIntrinsic in https://github.com/dotnet/coreclr/blob/master/src/jit/importer.cpp has details how these intrinsic methods are recognized and expanded in the JIT.

*.Portable.cs conventions

https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/project-guidelines.md#code-file-naming-conventions applied to System.Memory project.

System.Private.CoreLib.sln .net side

@JeremyKuhne and I talked about this yesterday. @JeremyKuhne Could you please share your plans to make this more straightforward once you have them?

[MarcoRossignoli]

@jkotas great!Thank you very much!

[JeremyKuhne]

@MarcoRossignoli this is the best we have at the moment: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits

We want to improve the process, so don't hesitate to provide feedback. You can add to the docs, create issues, whatever. Please feel free to tag me whenever you do.

[MarcoRossignoli]

Ok @JeremyKuhne! thank's a lot for the infos!

[WinCPP]

I studied the two places that @fiigii and @4creators mentioned. The first one (ref here) appears to be related indexed access on the array whereas the second one (ref here) appears to be related to initialization (SIMDIntrinsicInitArray and SIMDIntrinsicInitArrayX). Looks like the latter is the one of interest for us...?

SCK_RNGCHK_FAIL maps to Core helper function enum value CORINFO_HELP_RNGCHKFAIL (flowgraph.cpp) which ultimately leads to the jit helper function JIT_RngChkFail in jithelpers.cpp. So a perhaps a new value in the enum SpecialCodeKind will have to follow similar flow (with other places where these enum values are handled). Hope I am making sense.

However, in JIT_RngChkFail (jithelpers.cpp) function there appears to be an invocation of ComPlusThrow with kIndexOutOfRangeException which perhaps maps to the localized string for the exception. I was not able to find where kIndexOutOfRangeException leads to, where it is defined, etc.

Additionally, all these exceptions thrown as ComPlusThrow appear use exception strings without any parameters / place-holders for dynamic data. In our case we are interested to show a log line error / exception message which will inform the user about minimum number of elements required in source array as per the datatype for the SIMD instruction. (Please refer to dotnet/coreclr#16733 and dotnet/corefx#26499 which I recently finished for Span<T> based constructor for Vector).

Appreciate inputs on whether I am on right track.

[jkotas]

I was not able to find where kIndexOutOfRangeException leads to, where it is defined, etc

It is defined via macros from https://github.com/dotnet/coreclr/blob/master/src/vm/rexcep.h

[4creators]

Check the following places

https://github.com/dotnet/coreclr/blob/b3ea3629f6ada693bd6db259485f87b7a8beb78d/src/utilcode/ex.cpp#L84-L99
https://github.com/dotnet/coreclr/blob/b3ea3629f6ada693bd6db259485f87b7a8beb78d/src/utilcode/ex.cpp#L1625-L1663

and follow the logic

It will specifically lead to

https://github.com/dotnet/coreclr/blob/b3ea3629f6ada693bd6db259485f87b7a8beb78d/src/inc/utilcode.h#L725-L896

The class CCompRC is used to access strings

//*****************************************************************************
// CCompRC manages string Resource access for COM+. This includes loading
// the MsCorRC.dll for resources as well allowing each thread to use a
// a different localized version.
//*****************************************************************************
class CCompRC
{

[WinCPP]

With help of pointers provided above, I tried finding the code. The problem that I face is I'm unable to crack the link between ComPlusThrow that I mentioned previously and the CCompRC. Actually I am handicapped by lack of code browser... is there an sln file for this code base? I am using Visual Studio Code but it has limitations. How do you guys browse the code?

For the new exception message "At least {0} element(s) are expected in the parameter "{1}" defined in src/mscorlib/Resources/Strings.resx how would ComPlusThrow provide the parameters (am unable to find similar instances) and how will the C# type of format string (containing placeholders such as {0}) be parsed into native C++ style format string that will have %d and %s respectively? That is something that I'm trying to understand / search examples. (I am assuming that the MsCorRC.dll contains the resource strings from the above mentioned resx file).

Between, quick check - is this issue no more within the realm of 'up-for-grabs' and am I consuming too many cycles of yours? If not then I am interested to continue... Thanks!

[danmoseley]

@WinCPP I don't know the best way, but I open this in VS:  C:\git\coreclr\bin\obj\Windows_NT.x64.Debug\src\vm\wks\cee_wks.vcxproj

The vcxproj's are generated by CMake during the build.

[danmoseley]

For the new exception message "At least {0} element(s) are expected in the parameter "{1}" defined in src/mscorlib/Resources/Strings.resx how would ComPlusThrow provide the parameters (am unable to find similar instances) and how will the C# type of format string (containing placeholders such as {0}) be parsed into native C++ style format string that will have %d and %s respectively?

COMPlusThrow takes up to 6 extra args eg

               COMPlusThrow(kCannotUnloadAppDomainException,
                                 IDS_EE_ADUNLOAD_CANT_UNWIND_THREAD,
                                 sThreadId);

in this case the resource is in C:\git\coreclr\src\dlls\mscorrc\mscorrc.rc

  IDS_EE_ADUNLOAD_CANT_UNWIND_THREAD      "AppDomain cannot be unloaded because the thread '%1' cannot be unwound out of it."

[tannergooding]

How do you guys browse the code?

git grep is generally my goto

[4creators]

How do you guys browse the code?

git grep is generally my goto

Similar way in VS Code is to use search with functionName or in Visual Studio open coreclr\bin\Windows_NT.x64.Release\CoreCLR.sln or any other combination of Platform Configuration and use Find with FindAll (you can use regex in search as well). In my experience Intellisense in VS is not always reliable.

[WinCPP]

@danmosemsft

in this case the resource is in C:\git\coreclr\src\dlls\mscorrc\mscorrc.rc

Please let me rephrase my question. Currently following exception is thrown which defined in src/mscorlib/Resources/Strings.resx#L373-375 and not in src/dlls/mscorrc/mscorrc.rc.

  <data name="Arg_IndexOutOfRangeException" xml:space="preserve">
    <value>Index was outside the bounds of the array.</value>
  </data>

Essentially JITter appears to be sourcing the exception string from a managed resource bundle, but this string doesn't have parameter placeholders. The new more descriptive exception message with placeholders for minimum element count is already defined here in managed resource file.
I was under impression that, if the exception string is already defined in managed resource then we have to use the same in native code as well and not duplicate it in rc file, and hence the search for native code that will process the C# style format string, but there doesn't seem to be any. So looks like I have to duplicate the string into the above rc file that you mentioned with native format specifiers. Appreciate inputs.

[WinCPP]

hmm and further I'm trying to figure out the place where the IR sub-tree generated by GenTreeBoundsChk in simd.cpp, is traversed to emit code for the throw BB. Because that is the place where I guess tree traversal code will have to be added for the new SpecialCodeKind to pull out from the IR tree, the minimum number of elements expected for the Vector<T> per the simd type and that value will have to be passed to the ComPlusThrow method. I hope I am making sense.

Between, this appears to be different from the "app domain unload" exception case discussed above, because that appears to be directly thrown from the compiled code in JIT and not related to on-the-fly compiled (JITted) code.

I guess the code generation code that I am interested in is in src\jit\codegenxarch.cpp...?

---

EDIT 1: I think I got it... it is CodeGen::genRangeCheck (codegenxarch.cpp) which then calls CodeGen::genJumpToThrowHlpBlk (codegencommon.cpp)...

[danmoseley]

Incidentally, Arg_IndexOutOfRangeException really ought to be changed at some point to include the index and the array length. This is consistent with all the other places we have been including more useful data in exception messages, eg., filesystem paths

[WinCPP]

Few days back, for issue dotnet/corefx#24343 (PR dotnet/coreclr#16733) I added the following new exception to be thrown in Span based constructor for Vector<T> and also patched it up in T[] based constructor...

<data name="Arg_InsufficientNumberOfElements" xml:space="preserve">
   <value>At least {0} element(s) are expected in the parameter "{1}".</value>
</data>

To include the index and the length, are we looking to have message such as Beginning at the specified index '{0}', the source should have at least '{1}' number of element(s) or something similar...?

[danmoseley]

@WinCPP I think the existing string is very famliar to developers' eyes so I would change it only a little

  <data name="Arg_IndexOutOfRangeExceptionWithValue" xml:space="preserve">
    <value>Index {0} was outside the bounds of the array of length {1}.</value>
  </data>

This does not need to be in this PR. Fully eliminating the old string might take some time/several PR's.

[WinCPP]

True. But I thought the ask in this issue (opening few comments) was to give at information as to how many number of elements are expected (should be present at the minimum) in the source... And the index throws a spinner as then the minimum number of elements in the source should be with respect to the index...

[danmoseley]

Fair enough

[jkotas]

Arg_IndexOutOfRangeException really ought to be changed at some point to include the index and the array length.

If you do this throughout the framework, you are going to bloat the code quite a bit and likely introduce number of performance regressions in the process.

We should understand the value we would be adding by doing so. It is pretty rare to be able to diagnose the index out of range exception by just looking at the bad index. You typically need to know the larger context, so I doubt that knowing the value of the bad index would improve diagnostic experience significantly.

[danmoseley]

Alright -- makes sense -- disregard my suggestion ... 😃

[WinCPP]

Hi. I think I have figured out how to pass the vectorLength (for showing in exception message) to the function CodeGen::genJumpToThrowHlpBlk (here). I plan to have a following overload, mostly identical to original, that will accept the IR tree of the index (generated here).

void CodeGen::genJumpToThrowHlpBlk(emitJumpKind jumpKind, SpecialCodeKind codeKind, GenTree* indexTree, GenTree* failBlk = nullptr)    // note the additional 3rd argument indexTree

Idea is to traverse the indexTree, find out the vectorLength and then using emitter::emitIns_I and CodeGen::genSinglePush, populate the arguments for the new exception call (that takes 1 parameter) and then the call genEmitHelperCall (here) with argSize of 1. (Something on the following lines done in here).

        getEmitter()->emitIns_I(INS_push, EA_4BYTE, vectorLength);
        genSinglePush();
        args += REGSIZE_BYTES;

However, I am not sure how to handle the "throw block reuse" in the function genEmitHelperCall as passing context specific vectorLength is against reuse of throw block, which I believe is optimization. And secondly, does this whole thing get affected by different architectures...?

I hope I am on right tracks. Inputs appreciated.

[jkotas]

I think CodeGen::genJumpToThrowHlpBlk is likely too late to insert the extra arguments for the throw helper.

It tends to be complex and fragile to generate complex intrinsic expansions in the JIT like what you are trying to do here. You may look at this from other direction. Make the Vector constructor non-intriniscs, like:

        public Vector(T[] values)
            : this()
        {
            if ((typeof(T) == typeof(Byte))
                || (typeof(T) == typeof(SByte))
                || (typeof(T) == typeof(UInt16))
                || (typeof(T) == typeof(Int16))
                || (typeof(T) == typeof(UInt32))
                || (typeof(T) == typeof(Int32))
                || (typeof(T) == typeof(UInt64))
                || (typeof(T) == typeof(Int64))
                || (typeof(T) == typeof(Single))
                || (typeof(T) == typeof(Double)))
            {
                if (values.Length < Count)
                {
                    throw new IndexOutOfRangeException(SR.Format(SR.Arg_InsufficientNumberOfElements, Vector<T>.Count, nameof(values)));
                }
                this = Unsafe.ReadUnaligned<Vector<T>>(ref values.GetRawSzArrayData());
            }
            else
            {
                throw new NotSupportedException(SR.Arg_TypeNotSupported);
            }
        }

And then see whether we are losing any optimizations if you do this. If we are losing any optimizations with non-intrinsic implementation like this, look at what needs to be done to get them back.

[WinCPP]

@jkotas Thanks for the inputs. Need some guidance around this line.

And then see whether we are losing any optimizations if you do this. If we are losing any optimizations with non-intrinsic implementation like this, look at what needs to be done to get them back.

Do you mean performance data or you mean the generated IL code, etc.? I haven't done that earlier and hence some inputs around what I have to inspect (and perhaps the tools) will help me a lot and would give me an opportunity to learn something new. Thanks!

[jkotas]

Do you mean performance data or you mean the generated IL code, etc.?

I meant generated native code. It is where any missing optimizations are going to show up. Here is what you can try to do:

• Build checked CoreCLR build
• Find or write a simple test that calls Vector(T[] values) constructor in a loop over an array
• set COMPlus_JitDisasm=...name of the test method... in environment
• Run the test. It should print the native disassembly to the console. Compare the disassembly before/after the change.

Alternatively, you can use tools like http://adamsitnik.com/Disassembly-Diagnoser/ . Or just look at the native code using native debugger.

[WinCPP]

Hi @jkotas, resuming after considerable time. I did the following,

1. Built coreclr as checked build with the changes you had suggested here for the Vector(T[]) constructor (with removal of Intrinsic attribute on the constructor). CLR code changes are in this commit.
2. Built corefx using private clr binaries with command

build -release -- /p:CoreCLROverridePath=D:\WinCPP\coreclr\bin\Product\Windows_NT.x64.Checked

3. Wrote a test case that invoked the constructor in loop and using the ComPlus_JitDisasm set for specific method got the dump of the assembly. Please see this commit for the test case.

But still the call to the JIT exception throw method CORINFO_HELP_RNGCHKFAIL is generated with overall increase in byte code size by 9 bytes. Please refer to here and here for assembly dump before and after the coreclr changes.

         8379081F             cmp      dword ptr [rcx+8], 31
         7705                 ja       SHORT G_M22984_IG05
         E8DBCB0B5E           call     CORINFO_HELP_RNGCHKFAIL

Not sure if this is correct way. Appreciate your advice.

---

EDIT 1: I am working on Windows 10 x64 machine. Is this some issue of netcoreapp vs some other type of build that I should be using...?

[jkotas]

Try generating the jit log for the method using ComPlus_JitDump and then try to figure out why the optimization did not happen. https://github.com/dotnet/coreclr/blob/master/Documentation/botr/ryujit-tutorial.md has some documentation about the internal organization of the JIT.

[WinCPP]

Hi all. Want to keep you updated that I'm not getting cycles to do this one at this point in time. Apologies.

[Gnbrkm41]

Looks intentional, but we do have validation so we could easily change the exception. Changing the exception type is a binary breaking change, not sure it's worth it. We should, however, update the message to explain the error condition.

I wonder what is the design decision here, whether IndexOutOfRangeException is okay for perf reasons or to eventually change to ArgumentException? I was editing the documents for the constructors and I'm stuck whether to document the exception thrown when the length of the array is less than Count.

[jkotas]

I'm stuck whether to document the exception thrown when the length of the array is less than Count.

Yes, it is fine to document the current behavior for exception thrown by Vector constructors. Vector<T> constructors do not follow the framework design guidelines for performance reasons. Another difference from the standard is that they throw NullReferenceException (vs. the ArgumentNullException recommended by the framework design guidelines).