[WIP] Fix integer-divide overflow in JIT strength reduction - #134502
jakobbotsch with Copilot wants to merge 6 commits into
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: @dotnet/runtime-infrastructure |
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ab8269e9-fb00-449c-8da8-63fb5bf7edee Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
| if (a == -1) | ||
| { | ||
| // Avoid signed remainder overflow for MinValue % -1. Return positive 1 | ||
| // so that callers do not divide MinValue by -1 either. | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
@copilot Make this return -1 in that case, and update the comment.
There was a problem hiding this comment.
Updated in da6ba1d; completed in 43776e6. Gcd now returns -1 and the comment is corrected. This exposed another MinValue / -1 fault in RephraseIV, so that path now uses wrapping unsigned negation. Linux x64 Debug build and all 356 Regression_ro_2 cases passed, including the 10 focused int/long cases.
Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
|
@copilot Instead of this fix let's just fix it by making |
Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
Implemented in 88f40f2. |
| [Theory] | ||
| [InlineData(0, 0)] | ||
| [InlineData(1, 0)] | ||
| [InlineData(2, int.MaxValue)] | ||
| [InlineData(5, -10)] | ||
| [InlineData(6, int.MaxValue - 14)] | ||
| public static void TestInt32(int n, int expected) | ||
| { | ||
| Assert.Equal(expected, SumInt32(n)); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(0, 0L)] | ||
| [InlineData(1, 0L)] | ||
| [InlineData(2, long.MaxValue)] | ||
| [InlineData(5, -10L)] | ||
| [InlineData(6, long.MaxValue - 14)] | ||
| public static void TestInt64(int n, long expected) | ||
| { | ||
| Assert.Equal(expected, SumInt64(n)); | ||
| } |
There was a problem hiding this comment.
@copilot We do not need multiple InlineData to test this. A single Fact with a single test value is fine.
There was a problem hiding this comment.
Updated in ec4e4cd to one [Fact] using n = 5, with assertions for both int and long. The focused fact and all 364 Regression_ro_2 cases passed on Linux x64 Checked.
Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
MihaZupan
added
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
int.MinValue % -1inGcd, faulting the JIT with an integer-divide overflow #133756int.MinValue % -1inGcd, faulting the JIT with an integer-divide overflow #133756