Author: | v-wenyuxu |
---|---|
Assignees: | - |
Labels: | `os-windows`, `GCStress`, `arch-x64`, `area-CodeGen-coreclr`, `untriaged`, `blocking-clean-ci-optional` |
Milestone: | - |
Closed v-wenyuxu closed 6 months ago
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch See info in area-owners.md if you want to be subscribed.
Author: | v-wenyuxu |
---|---|
Assignees: | - |
Labels: | `os-windows`, `GCStress`, `arch-x64`, `area-CodeGen-coreclr`, `untriaged`, `blocking-clean-ci-optional` |
Milestone: | - |
This is a JIT assert; changing area to codegen
This is failing in runtime-coreclr jitstress, too.
@TIHan, PTAL. It is blocking clean ci optional. It is asserting in Generate Code.
Will do.
Did a bisection, and https://github.com/dotnet/runtime/pull/98623 I believe is what allowed this assertion to appear. I'm not sure yet as to the cause. This could have been a bug for a while, but only manifested itself with the PR.
This only occurs with:
DOTNET_EnableHWIntrinsic=0
DOTNET_JITMinOpts=1
DOTNET_TieredCompilation=0
Minimal repo:
[MethodImpl(MethodImplOptions.NoInlining)]
static void Consume(Vector<byte> v) { }
static unsafe void Main()
{
int tmp = 1;
Vector<byte> vector = Unsafe.As<int, Vector256<byte>>(ref tmp).AsVector();
Consume(vector);
}
When run:
DOTNET_EnableHWIntrinsic=0
DOTNET_JITMinOpts=1
DOTNET_TieredCompilation=0
c:\work\runtime>C:\work\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\CoreRun.exe "C:\Users\will\source\repos\ConsoleApp13\ConsoleApp13\bin\Release\net8.0\ConsoleApp13.dll"
Assert failure(PID 50956 [0x0000c70c], Thread: 50052 [0xc384]): Assertion failed '((int)offs < 0) || emitComp->opts.IsOSR()' in 'ConsoleApp13.Program:Main()' during 'Generate code' (IL size 25; hash 0x6a7f561b; MinOpts)
File: C:\work\runtime\src\coreclr\jit\emitxarch.cpp:3876
Image: C:\work\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\corerun.exe
Using the minimal repo, I can hit the same assert with a commit from Dec 2023, so https://github.com/dotnet/runtime/pull/98623 is not the cause, but it may have allowed this issue to manifest.
@EgorBo @tannergooding Based on the minimal repo, I'm convinced that the Unsafe code is dangerous and basically not correct.
From the original issue, the assert was:
Assert failure(PID 8168 [0x00001fe8], Thread: 7604 [0x1db4]): Assertion failed '((int)offs < 0) || emitComp->opts.IsOSR()' in 'System.SpanHelpers:Fill[ubyte](byref,ulong,ubyte)' during 'Generate code' (IL size 906; hash 0xd8592510; MinOpts)
It invoked public static unsafe void Fill<T>(ref T refData, nuint numElements, T value)
, the T
was byte
, and when looking at the implementation, you find:
else if (sizeof(T) == 32)
{
if (Vector<byte>.Count == 32)
{
vector = Unsafe.As<T, Vector256<byte>>(ref tmp).AsVector();
}
else
{
Debug.Fail("Vector<T> isn't 256 bits in size?");
goto CannotVectorize;
}
}
Because we turned off HWIntrinsics and TieredCompilation and forced MinOpts, the JIT is going to try to emit that path even if it never gets ran.
I guess the JIT really does not like trying to emit Unsafe.As<byte, Vector256<byte>>(ref tmp).AsVector();
.
I'm convinced that the Unsafe code is dangerous and basically not correct.
The code is unreachable and so while it is unsafe, it isn't "incorrect". This is in part a weird aspect of generic code where some paths are nonsensical if they were to be actually taken, but the guards prevent that, so its fine. We couldn't properly write generic code if this kind of logic weren't allowed.
It looks like we hit this because we have an EBP offset that is not negative and we expect it will always be negative. In this case it isn't since we're reading more bytes than exist in the frame. It's possible we should detect this case and not allow an EBP based frame or should otherwise ensure that this type of mismatch is handled.
It's also possible a more trivial fix is to make this use Unsafe.BitCast
which will resolve to the software fallback which throws in the case that the sizes mismatch. This is technically "safer" and avoids the operand being address taken. That of course doesn't resolve the underlying assert, but it would avoid the min-opts assert if we basically want to say "its fine if the codegen is incorrect since the path is unreachable, we won't do anything that could trigger it in our own code"
@BruceForstall @jakobbotsch I'm not sure what the best way to handle this is. Based on the minimal repo, the code is technically valid
, but because it is going outside the bounds of the stack and trying to use EBP, the offset is positive but it expected it to be negative which is why it asserts.
I had a conversation with @tannergooding , and he had a few suggestions:
I think there are a few ways that it could be addressed, but I'm not really sure what's "right" here
- We remove the assert, its technically faulty and incorrect in the face of code a user might write
- We update the JIT to account for this type of aliasing, either disallowing the use of EBP or tracking that we might access invalid offsets
- We say the assert is fine, since its debug only anyways, and we should avoid code that might trigger it in our own code. This would likely involve switching to Unsafe.BitCast which better fits the intent and the guards that the actual code has (this might not be possible though, given the generic constraints, I don't remember what they are)
3
may not actually be possible. 2
looks tricky, and 1
is simple but could lead to bugs appearing in the future.
The reason why 3 may be difficult/not possible is because BitCast
is constrained (TFrom/TTo must be struct) but the calling API might not be. We do have options for that, like removing the constraints and doing the "is value type" check manually (which we've done in other places), would just need to work through that if that's the path opted for.
BitCast
ends up working around the assert here because it is preserved as a call which throws if the sizes don't match, so we'd never get an "invalid" reinterpret cast leftover in the code the JIT sees.
If the assert hits, we're going to generate code this is (probably) going to corrupt the stack (in this case, read garbage). In this case it looks like it's because we're unrolling a Vector256 store and the second instruction has a displacement that makes the ebp offset positive.
One way to fix this is to make all the asserts in emitInsSizeSVCalcDisp
based on the variable frame offset before adding the displacement dsp
. This significantly weakens the asserts, but if we must allow unsafe code that is truly unsafe, then that might be a way to go.
we're going to generate code this is (probably) going to corrupt the stack (in this case, read garbage)
Notably we're generating the code here, but it will never execute in this particular scenario because we have guards that ensure the path is only taken when the read would be valid. It's mostly getting generated because MinOpts
prevents the removal of definitely dead code so we get stuck with this unreachable extra logic.
A user could certainly write such code themselves without the guards however, in which case we will generate the code and not hit any assert (since it will use the production JIT).
I expect the assert here was mostly to catch places where the JIT computes an incorrect offset, but that of course doesn't account for users doing it themselves (either "safely" due to the code being unreachable like we have, or "unsafely" because the user has written some incorrect code). Hence why I suggested 3 as an alternative fix, basically don't allow MinOpts
to see the code that is unreachable and would cause the assert, so our own code would never trigger it, but user code still might.
One way to fix this is to make all the asserts in emitInsSizeSVCalcDisp based on the variable frame offset before adding the displacement dsp.
I tried this, but the assert now gets hit a lot in normal cases. The change was this:
/* Get the frame offset of the (non-temp) variable */
offs = emitComp->lvaFrameAddress(var, &EBPbased);
if (EBPbased)
{
#if defined(TARGET_AMD64) && !defined(UNIX_AMD64_ABI)
// If localloc is not used, then ebp chaining is done and hence
// offset of locals will be at negative offsets, Otherwise offsets
// will be positive. In future, when RBP gets positioned in the
// middle of the frame so as to optimize instruction encoding size,
// the below asserts needs to be modified appropriately.
// However, for Unix platforms, we always do frame pointer chaining,
// so offsets from the frame pointer will always be negative.
if (emitComp->compLocallocUsed || emitComp->opts.compDbgEnC)
{
noway_assert((int)offs >= 0);
}
else
#endif
{
// Dev10 804810 - failing this assert can lead to bad codegen and runtime crashes
CLANG_FORMAT_COMMENT_ANCHOR;
#ifdef UNIX_AMD64_ABI
const LclVarDsc* varDsc = emitComp->lvaGetDesc(var);
bool isRegPassedArg = varDsc->lvIsParam && varDsc->lvIsRegArg;
// Register passed args could have a stack offset of 0.
noway_assert((int)offs < 0 || isRegPassedArg || emitComp->opts.IsOSR());
#else // !UNIX_AMD64_ABI
// OSR transitioning to RBP frame currently can have mid-frame FP
noway_assert(((int)offs < 0) || emitComp->opts.IsOSR());
#endif // !UNIX_AMD64_ABI
}
}
offs += dsp;
A user could certainly write such code themselves without the guards however, in which case we will generate the code and not hit any assert (since it will use the production JIT).
Except for these are noway_assert
, which hit in Release builds. They drop back to MinOpts, which wouldn't help in this case.
Without this assert we might generate bad code, if we don't calculate the instruction size properly in the code that follows.
I tried this, but the assert now gets hit a lot in normal cases. The change was this:
That's basically what I was thinking.
What do you recommend we do then?
if we don't calculate the instruction size properly in the code that follows.
Any reason we don't just ensure we compute the size correctly in this case? I would've thought it already mostly just works since the logic for [ebp+offset]
is pretty static based on what the offset is and is ultimately shared with all other addressing mode paths.
That is, any non-zero offset requires the SIB
modifier. That offset for both 32-bit and 64-bit can be encoded as 1 or 4 bytes and is sign extended to the pointer width, thus any offset between [-128, +127]
is 1-byte and anything else is 4-bytes. There's really not much to get wrong here in terms of instruction size prediction.
I am working on a change which will resolve this by not containing out-of-bounds access to locals, which is how we've been handling this problem so far.
Failed in: runtime-coreclr gcstress-extra 20240302.2
Failed tests:
coreclr windows x64 Checked gcstress0xc_jitminopts_heapverify1 @ Windows.10.Amd64.Open
- JIT/Regression/JitBlue/Runtime_63942/Runtime_63942/Runtime_63942.cmd
Error message:
Assert failure(PID 4248 [0x00001098], Thread: 4428 [0x114c]): Assertion failed '((int)offs < 0) || emitComp->opts.IsOSR()' in 'System.SpanHelpers:Fill[ubyte](byref,ulong,ubyte)' during 'Generate code' (IL size 906; hash 0xd8592510; MinOpts)
File: D:_work1ssrccoreclrjitemitxarch.cpp:3876
Image: C:hwBC2C09DApcorerun.exe
Return code: 1
Raw output file: C:hwBC2C09DAwA6ED093BuploadsJitBlueRuntime_63942Runtime_63942output.txt
Raw output:
BEGIN EXECUTION
"C:hwBC2C09DApcorerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" -p "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization=true" Runtime_63942.dll
Expected: 100
Actual: -1073740286
END EXECUTION - FAILED
FAILED
Test failed. Trying to see if dump file was created in C:cores since 3/2/2024 11:08:56 PM
Processing C:corescorerun.exe.4248.dmp
Invoking: C:Program Files (x86)Windows Kits10Debuggersdcdb.exe -c "$<C:hwBC2C09DA mp3luamd.tmp" -z "C:corescorerun.exe.4248.dmp"
stdout:
Microsoft (R) Windows Debugger Version 10.0.18362.1 AMD64
Copyright (c) Microsoft Corporation. All rights reserved.
Loading Dump File [C:corescorerun.exe.4248.dmp]
User Mini Dump File with Full Memory: Only application data is available
************* Path validation summary **************
Response Time (ms) Location
OK C:hwBC2C09DApPDB
Symbol search path is: C:hwBC2C09DApPDB
Executable search path is:
Windows 10 Version 14393 MP (4 procs) Free x64
Product: Server, suite: TerminalServer DataCenter SingleUserTS
10.0.14393.6343 (rs1_release.230913-1727)
Machine Name:
Debug session time: Sat Mar 2 23:08:59.000 2024 (UTC + 0:00)
System Uptime: 0 days 1:47:14.309
Process Uptime: 0 days 0:00:03.000
............................
************* Symbol Loading Error Summary **************
Module name Error
ntdll The system cannot find the file specified
You can troubleshoot most symbol related issues by turning on symbol loading diagnostics (!sym noisy) and repeating the command that caused symbols to be loaded.
You should also verify that your symbol search path (.sympath) is correct.
This dump file has an exception of interest stored in it.
The stored exception information can be accessed via .ecxr.
(1098.114c): Unknown exception - code c0000602 (first/second chance not available)
For analysis of this file, run !analyze -v
*** WARNING: Unable to verify checksum for coreclr.dll
KERNELBASE!RaiseFailFastException+0xae:
00007ffd`456396ce 803d4d78120000 cmp byte ptr [KERNELBASE!local_unwind+0x7fe2 (00007ffd`45760f22)],0 ds:00007ffd`45760f22=00
0:000> cdb: Reading initial command '$<C:hwBC2C09DA mp3luamd.tmp'
0:000> .load C:Users
unner.dotnetsossos.dll
0:000> ~*k
. 0 Id: 1098.114c Suspend: 0 Teb: 00000092`cb596000 Unfrozen
Child-SP RetAddr Call Site
00000092`cb775f10 00007ffd`3d794f4c KERNELBASE!RaiseFailFastException+0xae
00000092`cb7764e0 00007ffd`3d123236 coreclr!_DbgBreakCheck+0x2ec
*** WARNING: Unable to verify checksum for clrjit.dll
00000092`cb777650 00007ffd`320b998c coreclr!CEEInfo::doAssert+0x76
00000092`cb7776f0 00007ffd`320b95aa clrjit!assertAbort+0x13c
(Inline Function) --------`-------- clrjit!noWayAssertAbortHelper+0x17
00000092`cb779780 00007ffd`32309989 clrjit!noWayAssertBodyConditional+0x4a
00000092`cb7797b0 00007ffd`323094bd clrjit!emitter::emitInsSizeSVCalcDisp+0x279
00000092`cb779800 00007ffd`3230fc02 clrjit!emitter::emitInsSizeSV+0x13d
00000092`cb779860 00007ffd`322ee313 clrjit!emitter::emitIns_R_S+0x1a2
(Inline Function) --------`-------- clrjit!CodeGen::genCodeForCpBlkUnroll::__l40::<lambda_1>::operator()+0x23
00000092`cb7798c0 00007ffd`322f1ac2 clrjit!CodeGen::genCodeForCpBlkUnroll+0x403
00000092`cb7799d0 00007ffd`322f45ac clrjit!CodeGen::genCodeForStoreBlk+0x192
00000092`cb779a10 00007ffd`3208968a clrjit!CodeGen::genCodeForTreeNode+0x142c
00000092`cb779c30 00007ffd`320811f4 clrjit!CodeGen::genCodeForBB
Stack trace:
at Xunit.Assert.True(Nullable`1 condition, String userMessage) in /_/src/Microsoft.DotNet.XUnitAssert/src/BooleanAsserts.cs:line 146
at Xunit.Assert.True(Boolean condition, String userMessage) in /_/src/Microsoft.DotNet.XUnitAssert/src/BooleanAsserts.cs:line 128
at TestLibrary.OutOfProcessTest.RunOutOfProcessTest(String assemblyPath)
at Program.<<Main>$>g__TestExecutor428|0_429(StreamWriter tempLogSw, StreamWriter statsCsvSw, <>c__DisplayClass0_0&)
Failed in: runtime-coreclr gcstress-extra 20240225.1
Failed tests:
Error message:
Stack trace: