Sve/Scatter test: Add Debug.Assert for array length #116158
Conversation
kunalspathak
changed the title
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
There was a problem hiding this comment.
Pull Request Overview
Adds runtime assertions in test templates to detect out-of-range vector indices before array access, helping diagnose the root cause of issue #115712.
- Inserts
Debug.Assertchecks when computing base pointers to validate index bounds. - Updates scatter, gather, and first-fault gather test templates with similar guard logic.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/tests/JIT/HardwareIntrinsics/Arm/Shared/SveScatterVectorBases.template | Added bounds check for outArray indexing |
| src/tests/JIT/HardwareIntrinsics/Arm/Shared/SveGatherVectorVectorBases.template | Added bounds check for baseArray indexing |
| src/tests/JIT/HardwareIntrinsics/Arm/Shared/SveGatherVectorFirstFaultingVectorBases.template | Added bounds checks in both gather loops |
| @@ -120,6 +120,10 @@ namespace JIT.HardwareIntrinsics.Arm._Sve | |||
| {Op2BaseType} baseAddrToValidate = (({Op2BaseType})outArrayPtr + (sizeof({Op2BaseType}) * inAddress[i])); | |||
|
|
|||
| // Make sure we got the correct base pointers. | |||
| if ((int)inAddress[i] >= (int)outArray.Length) | |||
There was a problem hiding this comment.
Casting inAddress[i] to int before comparing to outArray.Length could overflow or miss negative values. Consider comparing using the original index type, e.g., if (inAddress[i] >= outArray.Length), to ensure a correct bounds check.
| if ((int)inAddress[i] >= (int)outArray.Length) | |
| if (inAddress[i] >= outArray.Length) |
Copilot uses AI. Check for mistakes.
| @@ -124,6 +124,10 @@ namespace JIT.HardwareIntrinsics.Arm | |||
| {Op2BaseType} baseAddrToValidate = (({Op2BaseType})baseArrayPtr + (sizeof({RetBaseType}) * inArray2[i])); | |||
|
|
|||
| // Make sure we got the correct base pointers. | |||
| if ((int)inArray2[i] >= (int)baseArray.Length) | |||
There was a problem hiding this comment.
The cast to int may overflow or ignore negative values. Use the original index type for comparison: if (inArray2[i] >= baseArray.Length) to reliably guard against out-of-range accesses.
| if ((int)inArray2[i] >= (int)baseArray.Length) | |
| if (inArray2[i] >= baseArray.Length) |
Copilot uses AI. Check for mistakes.
| @@ -135,6 +135,10 @@ namespace JIT.HardwareIntrinsics.Arm | |||
| {Op2BaseType} baseAddrToValidate = (({Op2BaseType})baseArrayPtr + (sizeof({RetBaseType}) * inArray2[i])); | |||
|
|
|||
| // Make sure we got the correct base pointers. | |||
| if ((int)inArray2[i] >= (int)baseArray.Length) | |||
There was a problem hiding this comment.
Avoid casting to int for inArray2[i]; this can introduce overflow or sign issues. Compare the original index type directly against baseArray.Length for accurate validation.
Copilot uses AI. Check for mistakes.
| @@ -157,6 +161,10 @@ namespace JIT.HardwareIntrinsics.Arm | |||
| {Op2BaseType} baseAddrToValidate = (({Op2BaseType})baseArrayPtr + (sizeof({RetBaseType}) * inArray2Ffr[i])); | |||
|
|
|||
| // Make sure we got the correct base pointers. | |||
| if ((int)inArray2Ffr[i] >= (int)baseArray.Length) | |||
There was a problem hiding this comment.
In the first-fault loop, casting inArray2Ffr[i] to int may mis-handle large or negative values. Use the original type (e.g., if (inArray2Ffr[i] >= baseArray.Length)) for correct bounds checking.
| if ((int)inArray2Ffr[i] >= (int)baseArray.Length) | |
| if (inArray2Ffr[i] >= baseArray.Length) |
Copilot uses AI. Check for mistakes.
|
@dotnet/arm64-contrib |
|
@dotnet/jit-contrib @amanasifkhalid PTAL |
| @@ -135,6 +135,10 @@ namespace JIT.HardwareIntrinsics.Arm | |||
| {Op2BaseType} baseAddrToValidate = (({Op2BaseType})baseArrayPtr + (sizeof({RetBaseType}) * inArray2[i])); | |||
|
|
|||
| // Make sure we got the correct base pointers. | |||
| if ((int)inArray2[i] >= (int)baseArray.Length) | |||
There was a problem hiding this comment.
nit: Why not pass the condition directly to Debug.Assert?
There was a problem hiding this comment.
Yeah, I started with Console.WriteLine and converted to Debug.Assert. Fixed it.
| @@ -193,7 +197,7 @@ namespace JIT.HardwareIntrinsics.Arm._Sve | |||
| { | |||
| {Op2BaseType} baseAddrToValidate = (({Op2BaseType})_dataTable.outArrayPtr + (sizeof({Op2BaseType}) * _addressArr[i])); | |||
|
|
|||
| // Make sure we got the correct base pointers. | |||
| // Make sure we got the correct base pointers. | |||
There was a problem hiding this comment.
| // Make sure we got the correct base pointers. | |
| // Make sure we got the correct base pointers. |
|
/ba-g failures are unrelated |
Add more tracking to catch reasons for getting #115712.