Conversation
|
Azure Pipelines: 16 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @agocke |
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to the failing assertion and adds focused regression coverage for the negative-constant cases that previously asserted in Checked/Debug.
Pull request overview
This PR fixes a Checked/Debug JIT importer assertion that incorrectly fires when System.Array.GetLength/GetLowerBound/GetUpperBound is called with a negative constant dimension (e.g., -1), ensuring the call behaves like Release builds and throws IndexOutOfRangeException instead of asserting.
Changes:
- JIT importer: change the debug assertion to validate signed
intrepresentability of the constant dimension (so negative constants no longer trip the assertion). - Tests: extend an existing multidimensional-array regression test to cover
-1andint.MinValueforGetLength,GetLowerBound, andGetUpperBound.
File summaries
| File | Description |
|---|---|
| src/coreclr/jit/importercalls.cpp | Adjusts importer assertion for constant MD-array dimension arguments to avoid asserting on negative constants while preserving the existing unsigned rank bounds check. |
| src/tests/JIT/Regression/JitBlue/Runtime_60957/Runtime_60957.cs | Adds xUnit coverage ensuring negative constant dimensions throw IndexOutOfRangeException for GetLength/GetLowerBound/GetUpperBound. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
|
PTAL @dotnet/jit-contrib trivial fix of a bad assert |
| // is `int` sized. | ||
| INT64 dimValue = gtDim->AsIntConCommon()->IntegralValue(); | ||
| assert((unsigned int)dimValue == dimValue); | ||
| assert((int)dimValue == dimValue); |
There was a problem hiding this comment.
Does the (unsigned int)dimValue below need to be changed?
There was a problem hiding this comment.
Well, the code can be rewritten, but currently it's correct, (unsigned int)dimValue (for negative dimValue) overflows and doesn't pass the rank check
|
@copilot fix merge conflict. |
…constant-dimension Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The fix is narrowly scoped, preserves the existing bounds check, and includes targeted coverage for the reported assertion failure.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Calls such as
array.GetLength(-1)trigger a Checked/Debug JIT assertion instead of throwingIndexOutOfRangeException.intrepresentability, preserving the existing unsigned bounds check and Release behavior.-1andint.MinValueforGetLength,GetLowerBound, andGetUpperBound.Array.GetLengthasserts in the importer #133556