Repository navigation
AF in in System.Globalization.CharUnicodeInfo:InternalGetNumericValue #11492
Description
Found by dotnet/coreclr#21048. Remove NoOptimization workaround and run crossgen System.Private.CoreLib.dll.
Assert failure(PID 2976 [0x00000ba0], Thread: 3816 [0x0ee8]): Assertion failed '((regMask & emitThisGCrefRegs) && (ins == INS_add)) || ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))' in 'System.Globalization.CharUnicodeInfo:InternalGetNumericValue(int):double' (IL size 175)
Activity
Add workaround for https://github.com/dotnet/coreclr/issues/21051
CarolEidt commentedon Nov 16, 2018
@jkotas - do you have any insight as to what in dotnet/coreclr#21048 triggered this?
jkotas commentedon Nov 16, 2018
This is the sequence that triggered this:
ReadOnlySpan<byte>. Before this change, the bytes were copies from the IL image tobyte[]array and then used. After this change, the bytes are used directly from the IL image.ReadOnlySpan<byte>makes the Span to be used in new patterns that are not common today (but becoming increasingly more common as folks start taking advantage of this new C# feature). The Span indexer intrinsic expansion seems to have problems when combined with the RVA-backed field. We have not noticed it because of the Span indexer intrinsics were not getting detected properly when Update Unicode data and optimize CharUnicodeInfo indexes coreclr#20983 went in.Add workaround for https://github.com/dotnet/coreclr/issues/21051
Add workaround for https://github.com/dotnet/coreclr/issues/21051
Add workaround for https://github.com/dotnet/coreclr/issues/21051
Add workaround for https://github.com/dotnet/coreclr/issues/21051
Add workaround for https://github.com/dotnet/coreclr/issues/21051
AndyAyersMS commentedon Nov 16, 2018
@CarolEidt I can look at this, if you haven't already started.
CarolEidt commentedon Nov 16, 2018
@AndyAyersMS - I haven't started, so you're welcome to it!
AndyAyersMS commentedon Nov 16, 2018
I think the issue is that we always report RVA static addresses as
TYP_LONGand notTYP_BYREF. In this case the static address gets retyped as a byref viaUnsafe.Asand we end up with an implicit retyping across an assignment (after a bunch of inlinling, so we can't hope to get the types right when importing):If left in this state we eventually get confused about the GC impact of the resulting mov instruction and since this feeds an add we assert about inconsistent GC state at the add.
Seems like we could spot this in inlining when stitching things back up and retype the source, or we could try and handle things later, though not sure when would be the best time.
Am going to investigate what retyping early looks like.
pentp commentedon Nov 16, 2018
Unsafe.AsRef<T> (void* source)should have very similar behaviour (long used as a byref) - how is that handled?AndyAyersMS commentedon Nov 16, 2018
Good question. I am not sure how it differs yet.
Disambiguate Span indexer intrinsic (#6587)
AndyAyersMS commentedon Nov 20, 2018
Targeted retyping early happens to fix this case but I'm not sure if it is fully general (or how carefully I should match just this pattern vs general type mismatches).
We're pretty lax about letting longs and byrefs be interchangeable, especially across inlines. And for the most part this seems to be tolerated pretty well downstream. But in this particular case we have a long being reinterpreted as a byref, and the "birth" of the byref happens in a reg-reg move with the same source and destination register. So we don't emit the mov instruction and so lose track of birthpoint and this is what triggers the assert.
However there must be more to this case, because when I flag places where we skip emitting movs where the target is a GC type and the source is not, there are a fair number of hits in corelib that don't trigger asserts.
So, will keep digging.
AndyAyersMS commentedon Nov 20, 2018
Based on some spot checks, the reason this case asserts and other similar cases don't is that in this case the value is subsequently used in an add, and we have a check that if an add is going to produce a byref it needs to have started with a byref (or a gcref).
So if we lose track of a byref birth (because of an implicit cast that turns into a self-move that gets dropped as it seems pointless) and then don't add to the result, we probably won't notice that we lost the byref birth.
I'm not quite sure where to go with this... one take is that we just need to patch things up (at some point, possibly when swapping in inline ret exprs as this can introduce in-tree reinterprets); another is that this whole setup of allowing byrefs and ints/longs to interoperate is fundamentally broken and will lead to GC holes and we need to add explicit casts and/or attempt to retype more broadly.
@dotnet/jit-contrib thoughts?
AndyAyersMS commentedon Nov 20, 2018
Here's the "targed fix" which just tries to handle this one case.
CarolEidt commentedon Nov 21, 2018
I suspect that the only practical fix, at least in the near term, is to patch things up. I've never been entirely comfortable with the degree to which the JIT allows byrefs and native ints to interoperate, but I would imagine that unraveling that would be rather complex.
briansull commentedon Nov 26, 2018
I agree with Carol's comments. The targeted fix looks fine.
(Also remove the comments above the NoOptimizations attributes that you removed)
briansull commentedon Nov 26, 2018
I also have concerns in general about the new Unsafe.As method as it could be breaking many other type checks/assumptions
JIT: retype some return expressions
Add workaround for https://github.com/dotnet/coreclr/issues/21051