Skip to content
This repository has been archived by the owner on Nov 1, 2020. It is now read-only.

Bad codegen in Boolean::TryParse #5661

Closed
MichalStrehovsky opened this issue Apr 7, 2018 · 3 comments · Fixed by #5689
Closed

Bad codegen in Boolean::TryParse #5661

MichalStrehovsky opened this issue Apr 7, 2018 · 3 comments · Fixed by #5689
Assignees
Labels
Milestone

Comments

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Apr 7, 2018

TryParse(ReadOnlySpan<char> value, out bool result) accesses two string literals and makes a Span out of them - "True" and "False".

RyuJIT with optimizations enabled will reuse the first Span for "True" instead of making a new one for "False".

To repro, simply compile Console.WriteLine(Boolean.Parse("False")); with optimizations enabled. It will throw an exception because we don't match "False" to "False", but try to match "True" twice.

My guess is there's something wrong with this piece of code in RyuJIT:

https://github.com/dotnet/coreclr/blob/0f0320e58fd006a02cdecf7ae45426f54da333e5/src/jit/gentree.cpp#L6426-L6430

@MichalStrehovsky MichalStrehovsky added this to the Preview milestone Apr 7, 2018
@MichalStrehovsky
Copy link
Member Author

Blocking AvaloniaUI/Avalonia#1476.

@sandreenko Is my analysis remotely correct? Could you point me in the right direction?

JitDump:
jitdump.txt

The interesting lines are the ones with (reloc 0x421150) and (reloc 0x421158) - those are the string literal loads. Notice how we touch the second reloc, but then don't do anything with it.

@sandreenko
Copy link

Thanks for your analysis, it looks correct, I will try to fix it asap.

MichalStrehovsky added a commit to MichalStrehovsky/corert that referenced this issue Apr 9, 2018
When I was investigating dotnet#5661 yesterday I found we're asserting RyuJIT when compiling `System.IO.BinaryReader:.ctor(ref,ref,bool)`. This works around the issue.

No new bug filed since we track this in  dotnet#5180.
MichalStrehovsky added a commit that referenced this issue Apr 9, 2018
When I was investigating #5661 yesterday I found we're asserting RyuJIT when compiling `System.IO.BinaryReader:.ctor(ref,ref,bool)`. This works around the issue.

No new bug filed since we track this in  #5180.
@sandreenko
Copy link

sandreenko commented Apr 10, 2018

It is a value numbering problem where we for some reasons have wrong null VNs for both true and false cases. I am working on the fix.

MichalStrehovsky added a commit to MichalStrehovsky/corert that referenced this issue Apr 12, 2018
sergiy-k pushed a commit that referenced this issue Apr 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants