Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

JIT: fix byte range used by RangeCheck#21915

Merged
AndyAyersMS merged 2 commits into
dotnet:masterfrom
AndyAyersMS:FixByteRange
Jan 11, 2019
Merged

JIT: fix byte range used by RangeCheck#21915
AndyAyersMS merged 2 commits into
dotnet:masterfrom
AndyAyersMS:FixByteRange

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

Fix wrong byte range introduced in #21857.

Thanks to @jakobbotsch for spotting this.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

AndyAyersMS commented Jan 9, 2019

cc @dotnet/jit-contrib

No diffs in pmi fx.

@hughbe
Copy link
Copy Markdown

hughbe commented Jan 9, 2019

Just curious and for a bunch of small jit fixes, why do they not have tests? I get slightly confused when code changes occur without tests? In llvm, for example, all small changes verify actual asm output (obviously too much in clr) but does the type of testing in coreclr need to be improved?

@BruceForstall
Copy link
Copy Markdown

@hughbe It's generally a good idea to add tests, when possible. If @AndyAyersMS can come up with a test exhibiting a difference here, that would be great. Sometimes it's too expensive or not feasible.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

I'll see if I can come up with something, though the most likely impact is a missed optimization, and we don't have a great testing strategy for those.

@filipnavara
Copy link
Copy Markdown
Member

filipnavara commented Jan 10, 2019

Looks like in this case you may end-up with missing bounds check in extreme case. That probably should be testable. Feel free to correct me if I am wrong.

I am thinking of pseudo code akin to this:

Array data = Array.CreateInstance(typeof(int),
    new int[] { -127, 256 });
byte index = -128;
data[index] = 0; // Range check would be incorrectly omitted and memory accessed out of range

@AndyAyersMS
Copy link
Copy Markdown
Member Author

I have something similar in the works, will push it on this PR shortly. RangeCheck doesn't model a lot of IR operators so you have to be careful to choose just the right expressions.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Test covers some of the byte/sbyte boundaries, and will fail without this fix.

@AndyAyersMS AndyAyersMS merged commit 360e70f into dotnet:master Jan 11, 2019
@AndyAyersMS AndyAyersMS deleted the FixByteRange branch January 11, 2019 01:55
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Range is -128 to 127, not -127 to 128.


Commit migrated from dotnet/coreclr@360e70f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants