Skip to content

Fix a race condition in GCStress #41540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BruceForstall merged 1 commit into from
Aug 29, 2020
Merged

Fix a race condition in GCStress #41540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 60 additions & 32 deletions src/coreclr/src/vm/gccover.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,23 +744,37 @@ void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID
//Determine if instruction before the safe point is call using immediate (BLX Imm) or call by register (BLX Rm)
BOOL instructionIsACallThroughRegister = FALSE;
BOOL instructionIsACallThroughImmediate = FALSE;

#if defined(TARGET_ARM)

// POSSIBLE BUG: Note that we are looking backwards by 2 or 4 bytes, looking for particular call instruction encodings.
// However, we don't know if the previous instruction is 2 bytes or 4 bytes. Looking back 2 bytes could be looking into
// the middle of a 4-byte instruction. The only safe way to do this is by walking forward from the first instruction of
// the function.

// call by register instruction is two bytes (BL<c> Reg T1 encoding)
WORD instr = *((WORD*)savedInstrPtr - 1);

instr = instr & 0xff87;
if((instr ^ 0x4780) == 0)
if ((instr ^ 0x4780) == 0)
{
// It is call by register
instructionIsACallThroughRegister = TRUE;
}
else

@BruceForstall BruceForstall Aug 28, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only code change here is putting the instructionIsACallThroughImmediate case as an else clause, if we didn't first see a blx reg instruction. The rest is just whitespace and semicolons.

{
// call using immediate instructions are 4 bytes (BL<c> <label> T1 encoding)
instr = *((WORD*)savedInstrPtr - 2);
instr = instr & 0xf800;
if ((instr ^ 0xf000) == 0)
{
if ((*(((WORD*)savedInstrPtr) - 1) & 0xd000) == 0xd000)
{
// It is call by immediate
instructionIsACallThroughImmediate = TRUE;
}
}
}

// call using immediate instructions are 4 bytes (BL<c> <label> T1 encoding)
instr = *((WORD*)savedInstrPtr - 2);
instr = instr & 0xf800;
if((instr ^ 0xf000) == 0)
if((*(((WORD*)savedInstrPtr)-1) & 0xd000) == 0xd000)
// It is call by immediate
instructionIsACallThroughImmediate = TRUE;
#elif defined(TARGET_ARM64)
DWORD instr = *((DWORD*)savedInstrPtr - 1);

Expand All @@ -778,6 +792,7 @@ void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID
instructionIsACallThroughRegister = TRUE;
}
#endif // _TARGET_XXXX_

// safe point must always be after a call instruction
// and cannot be both call by register & immediate
// The safe points are also marked at jump calls( a special variant of
Expand Down Expand Up @@ -1383,16 +1398,15 @@ void DoGcStress (PCONTEXT regs, NativeCodeVersion nativeCodeVersion)
Thread *pThread = GetThread();
_ASSERTE(pThread);

if (!IsGcCoverageInterruptInstruction(instrPtr))
{
// This assert can fail if another thread changed original instruction to
// GCCoverage Interrupt instruction between these two commands. Uncomment it
// when threading issue gets resolved.
// _ASSERTE(IsOriginalInstruction(instrPtr, gcCover, offset));

// Someone beat us to it, just go on running.
return;
}
// There is a race condition with the computation of `atCall`. Multiple threads could enter
// this function (DoGcStress) at the same time. If one reads `*instrPtr` and sets `atCall`
// to `true`, it will proceed to, lower down in this function, call `pThread->CommitGCStressInstructionUpdate()`
// to replace the GCStress instruction at the call back to the original call instruction.
// Other threads could then read `*instrPtr` and see the actual call instruction instead of the
// call-specific GCStress instruction (INTERRUPT_INSTR_CALL[_32]). If `atCall` is set to false as
// a result, then we'll do a GCStress as if this is a fully-interruptible code site, which is isn't,
// which can leads to asserts (or, presumably, other failures). So, we have to check
// `if (!IsGcCoverageInterruptInstruction(instrPtr))` after we read `*instrPtr`.

bool atCall;
bool afterCallProtect[2] = { false, false };
Expand All @@ -1418,14 +1432,15 @@ void DoGcStress (PCONTEXT regs, NativeCodeVersion nativeCodeVersion)
}

#elif defined(TARGET_ARM)

forceStack[6] = (WORD*)instrPtr; // This is so I can see it fastchecked

size_t instrLen = GetARMInstructionLength(instrPtr);

if (instrLen == 2)
{
WORD instrVal = *(WORD*)instrPtr;
atCall = (instrVal == INTERRUPT_INSTR_CALL);
atCall = (instrVal == INTERRUPT_INSTR_CALL);
afterCallProtect[0] = (instrVal == INTERRUPT_INSTR_PROTECT_RET);
}
else
Expand All @@ -1434,10 +1449,12 @@ void DoGcStress (PCONTEXT regs, NativeCodeVersion nativeCodeVersion)

DWORD instrVal32 = *(DWORD*)instrPtr;

atCall = (instrVal32 == INTERRUPT_INSTR_CALL_32);
atCall = (instrVal32 == INTERRUPT_INSTR_CALL_32);
afterCallProtect[0] = (instrVal32 == INTERRUPT_INSTR_PROTECT_RET_32);
}

#elif defined(TARGET_ARM64)

DWORD instrVal = *(DWORD *)instrPtr;
forceStack[6] = &instrVal; // This is so I can see it fastchecked

Expand All @@ -1446,6 +1463,17 @@ void DoGcStress (PCONTEXT regs, NativeCodeVersion nativeCodeVersion)

#endif // _TARGET_*

if (!IsGcCoverageInterruptInstruction(instrPtr))

@BruceForstall BruceForstall Aug 28, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the real fix: moving this code here from above the atCall setting code.

{
// This assert can fail if another thread changed original instruction to
// GCCoverage Interrupt instruction between these two commands. Uncomment it
// when threading issue gets resolved.
// _ASSERTE(IsOriginalInstruction(instrPtr, gcCover, offset));

// Someone beat us to it, just go on running.
return;
}

#ifdef TARGET_X86
/* are we at the very first instruction? If so, capture the register state */
bool bShouldUpdateProlog = true;
Expand Down Expand Up @@ -1553,18 +1581,18 @@ void DoGcStress (PCONTEXT regs, NativeCodeVersion nativeCodeVersion)

#if defined(TARGET_X86) || defined(TARGET_AMD64) || defined(TARGET_ARM) || defined(TARGET_ARM64)

/* In non-fully interrruptable code, if the EIP is just after a call instr
/* In non-fully interruptible code, if the EIP is just after a call instr
means something different because it expects that we are IN the
called method, not actually at the instruction just after the call. This
is important, because until the called method returns, IT is responsible
for protecting the return value. Thus just after a call instruction
we have to protect EAX if the method being called returns a GC pointer.

To figure this out, we need to stop AT the call so we can determine the
target (and thus whether it returns a GC pointer), and then place the
target (and thus whether it returns one or more GC pointers), and then place
a different interrupt instruction so that the GCCover harness protects
EAX before doing the GC). This effectively simulates a hijack in
non-fully interruptible code */
the return value register(s) before doing the GC. This effectively simulates
a hijack in non-fully interruptible code */

/* TODO. Simulating the hijack could cause problems in cases where the
return register is not always a valid GC ref on the return offset.
Expand Down Expand Up @@ -1597,15 +1625,15 @@ void DoGcStress (PCONTEXT regs, NativeCodeVersion nativeCodeVersion)
// We are in preemptive mode in JITTed code. This implies that we are into IL stub
// close to PINVOKE method. This call will never return objectrefs.
#ifdef TARGET_ARM
size_t instrLen = GetARMInstructionLength(nextInstr);
if (instrLen == 2)
*(WORD*)nextInstr = INTERRUPT_INSTR;
else
*(DWORD*)nextInstr = INTERRUPT_INSTR_32;
size_t instrLen = GetARMInstructionLength(nextInstr);
if (instrLen == 2)
*(WORD*)nextInstr = INTERRUPT_INSTR;
else
*(DWORD*)nextInstr = INTERRUPT_INSTR_32;
#elif defined(TARGET_ARM64)
*(DWORD*)nextInstr = INTERRUPT_INSTR;
*(DWORD*)nextInstr = INTERRUPT_INSTR;
#else
*nextInstr = INTERRUPT_INSTR;
*nextInstr = INTERRUPT_INSTR;
#endif
}
else
Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/tests/issues.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,9 +543,6 @@
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/CLR-x86-JIT/V1-M13-RTM/b91248/b91248/*">
<Issue>https://github.com/dotnet/runtime/issues/12979</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/GitHub_27924/GitHub_27924/*">
<Issue>https://github.com/dotnet/runtime/issues/36681</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Directed/PREFIX/unaligned/4/arglist_Target_ARM/*">
<Issue>https://github.com/dotnet/runtime/issues/12979</Issue>
</ExcludeList>
Expand Down