From 5d20d127c1aebca0a346c498e652cdcf0c174282 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Thu, 22 Jul 2021 16:19:52 -0700 Subject: [PATCH] Disable JIT xarch asserts on gcref/byref computation The emitter has asserts that an xarch RMW inc/dec/add/sub of a byref must have an incoming gcref/byref to be legal. This is (no longer) true due to extensive use of Span and Unsafe constructs, where we often see lclheap or other integer typed values cast to byref. Also, the emitter only updates its GC info when an instruction is generated. When one of these casts from integer to byref ends up getting the same register allocated, and its instruction is thus omitted, the emitter doesn't get the appropriate gcref update (this problem is being attempted to be solved elsewhere). For now, disable these asserts. Re-enable the tests disabled in #54207 Fixes #51728, #54007 --- src/coreclr/jit/emitxarch.cpp | 29 +++++++++++++++---- .../System.IO.Hashing/tests/Crc32Tests.cs | 1 - .../System.IO.Hashing/tests/Crc64Tests.cs | 1 - .../tests/XxHash32Tests.007.cs | 1 - .../System.IO.Hashing/tests/XxHash32Tests.cs | 1 - .../tests/XxHash32Tests.f00d.cs | 1 - .../tests/XxHash64Tests.007.cs | 1 - .../System.IO.Hashing/tests/XxHash64Tests.cs | 1 - .../tests/XxHash64Tests.f00d.cs | 1 - 9 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index c35a6675fe757f..6e0de0d59e195b 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -11914,10 +11914,12 @@ BYTE* emitter::emitOutputR(BYTE* dst, instrDesc* id) #endif if (id->idGCref()) { - // The reg must currently be holding either a gcref or a byref - // and the instruction must be inc or dec - assert(((emitThisGCrefRegs | emitThisByrefRegs) & regMask) && - (ins == INS_inc || ins == INS_dec || ins == INS_inc_l || ins == INS_dec_l)); + assert(ins == INS_inc || ins == INS_dec || ins == INS_inc_l || ins == INS_dec_l); + // We would like to assert that the reg must currently be holding either a gcref or a byref. + // However, we can see cases where a LCLHEAP generates a non-gcref value into a register, + // and the first instruction we generate after the LCLHEAP is an `inc` that is typed as + // byref. We'll properly create the byref gcinfo when this happens. + // assert((emitThisGCrefRegs | emitThisByrefRegs) & regMask); assert(id->idGCref() == GCT_BYREF); // Mark it as holding a GCT_BYREF emitGCregLiveUpd(GCT_BYREF, id->idReg1(), dst); @@ -12222,15 +12224,30 @@ BYTE* emitter::emitOutputRR(BYTE* dst, instrDesc* id) case INS_sub: assert(id->idGCref() == GCT_BYREF); +#if 0 #ifdef DEBUG + // Due to elided register moves, we can't have the following assert. + // For example, consider: + // t85 = LCL_VAR byref V01 arg1 rdx (last use) REG rdx + // /--* t85 byref + // * STORE_LCL_VAR byref V40 tmp31 rdx REG rdx + // Here, V01 is type `long` on entry, then is stored as a byref. But because + // the register allocator assigned the same register, no instruction was + // generated, and we only (currently) make gcref/byref changes in emitter GC info + // when an instruction is generated. We still generate correct GC info, as this + // instruction, if writing a GC ref even through reading a long, will go live here. + // These situations typically occur due to unsafe casting, such as with Span. + regMaskTP regMask; regMask = genRegMask(reg1) | genRegMask(reg2); // r1/r2 could have been a GCREF as GCREF + int=BYREF - // or BYREF+/-int=BYREF + // or BYREF+/-int=BYREF assert(((regMask & emitThisGCrefRegs) && (ins == INS_add)) || ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))); -#endif +#endif // DEBUG +#endif // 0 + // Mark r1 as holding a byref emitGCregLiveUpd(GCT_BYREF, reg1, dst); break; diff --git a/src/libraries/System.IO.Hashing/tests/Crc32Tests.cs b/src/libraries/System.IO.Hashing/tests/Crc32Tests.cs index 4e5ac8753d3f79..6d2993f1509e47 100644 --- a/src/libraries/System.IO.Hashing/tests/Crc32Tests.cs +++ b/src/libraries/System.IO.Hashing/tests/Crc32Tests.cs @@ -93,7 +93,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase) InstanceAppendAllocateAndResetDriver(testCase); } - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)] [Theory] [MemberData(nameof(TestCases))] public void InstanceMultiAppendGetCurrentHash(TestCase testCase) diff --git a/src/libraries/System.IO.Hashing/tests/Crc64Tests.cs b/src/libraries/System.IO.Hashing/tests/Crc64Tests.cs index 546501aa4d5459..d647e4b9587989 100644 --- a/src/libraries/System.IO.Hashing/tests/Crc64Tests.cs +++ b/src/libraries/System.IO.Hashing/tests/Crc64Tests.cs @@ -97,7 +97,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase) InstanceAppendAllocateAndResetDriver(testCase); } - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)] [Theory] [MemberData(nameof(TestCases))] public void InstanceMultiAppendGetCurrentHash(TestCase testCase) diff --git a/src/libraries/System.IO.Hashing/tests/XxHash32Tests.007.cs b/src/libraries/System.IO.Hashing/tests/XxHash32Tests.007.cs index cf2e967d32a688..eed5aec365b3eb 100644 --- a/src/libraries/System.IO.Hashing/tests/XxHash32Tests.007.cs +++ b/src/libraries/System.IO.Hashing/tests/XxHash32Tests.007.cs @@ -105,7 +105,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase) InstanceAppendAllocateAndResetDriver(testCase); } - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)] [Theory] [MemberData(nameof(TestCases))] public void InstanceMultiAppendGetCurrentHash(TestCase testCase) diff --git a/src/libraries/System.IO.Hashing/tests/XxHash32Tests.cs b/src/libraries/System.IO.Hashing/tests/XxHash32Tests.cs index 15c671d12bee98..f9dccad0294028 100644 --- a/src/libraries/System.IO.Hashing/tests/XxHash32Tests.cs +++ b/src/libraries/System.IO.Hashing/tests/XxHash32Tests.cs @@ -118,7 +118,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase) InstanceAppendAllocateAndResetDriver(testCase); } - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)] [Theory] [MemberData(nameof(TestCases))] public void InstanceMultiAppendGetCurrentHash(TestCase testCase) diff --git a/src/libraries/System.IO.Hashing/tests/XxHash32Tests.f00d.cs b/src/libraries/System.IO.Hashing/tests/XxHash32Tests.f00d.cs index 007acd7655f559..5171e62f8ca562 100644 --- a/src/libraries/System.IO.Hashing/tests/XxHash32Tests.f00d.cs +++ b/src/libraries/System.IO.Hashing/tests/XxHash32Tests.f00d.cs @@ -105,7 +105,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase) InstanceAppendAllocateAndResetDriver(testCase); } - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)] [Theory] [MemberData(nameof(TestCases))] public void InstanceMultiAppendGetCurrentHash(TestCase testCase) diff --git a/src/libraries/System.IO.Hashing/tests/XxHash64Tests.007.cs b/src/libraries/System.IO.Hashing/tests/XxHash64Tests.007.cs index 4da264fb5ff103..5c2e575377870d 100644 --- a/src/libraries/System.IO.Hashing/tests/XxHash64Tests.007.cs +++ b/src/libraries/System.IO.Hashing/tests/XxHash64Tests.007.cs @@ -115,7 +115,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase) InstanceAppendAllocateAndResetDriver(testCase); } - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)] [Theory] [MemberData(nameof(TestCases))] public void InstanceMultiAppendGetCurrentHash(TestCase testCase) diff --git a/src/libraries/System.IO.Hashing/tests/XxHash64Tests.cs b/src/libraries/System.IO.Hashing/tests/XxHash64Tests.cs index 39245a975d8785..973d108fc93778 100644 --- a/src/libraries/System.IO.Hashing/tests/XxHash64Tests.cs +++ b/src/libraries/System.IO.Hashing/tests/XxHash64Tests.cs @@ -131,7 +131,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase) InstanceAppendAllocateAndResetDriver(testCase); } - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)] [Theory] [MemberData(nameof(TestCases))] public void InstanceMultiAppendGetCurrentHash(TestCase testCase) diff --git a/src/libraries/System.IO.Hashing/tests/XxHash64Tests.f00d.cs b/src/libraries/System.IO.Hashing/tests/XxHash64Tests.f00d.cs index 79d370e93d197c..23006a571627b3 100644 --- a/src/libraries/System.IO.Hashing/tests/XxHash64Tests.f00d.cs +++ b/src/libraries/System.IO.Hashing/tests/XxHash64Tests.f00d.cs @@ -115,7 +115,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase) InstanceAppendAllocateAndResetDriver(testCase); } - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)] [Theory] [MemberData(nameof(TestCases))] public void InstanceMultiAppendGetCurrentHash(TestCase testCase)