Skip to content

Fix for stress failure when adjusting effective IP while stackwalking may put it on a wrong instruction. #100376

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
merged 3 commits into from
Mar 30, 2024
Merged

Fix for stress failure when adjusting effective IP while stackwalking may put it on a wrong instruction. #100376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4acf8b
fix
VSadov Mar 27, 2024
6b91f33
same change for x86 and NativeAOT, get rid of ICodeManagerFlags::Abor…
VSadov Mar 28, 2024
d330b3b
Apply suggestions from code review
VSadov Mar 29, 2024
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
1 change: 0 additions & 1 deletion src/coreclr/inc/eetwain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ enum ICodeManagerFlags
ExecutionAborted = 0x0002, // execution of this function has been aborted
// (i.e. it will not continue execution at the
// current location)
AbortingCall = 0x0004, // The current call will never return
UpdateAllRegs = 0x0008, // update full register set
CodeAltered = 0x0010, // code of that function might be altered
// (e.g. by debugger), need to call EE
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/inc/gcinfodecoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ enum ICodeManagerFlags
ExecutionAborted = 0x0002, // execution of this function has been aborted
// (i.e. it will not continue execution at the
// current location)
AbortingCall = 0x0004, // The current call will never return
ParentOfFuncletStackFrame
= 0x0040, // A funclet for this frame was previously reported

Expand Down
18 changes: 17 additions & 1 deletion src/coreclr/jit/codegenlinear.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,9 @@ void CodeGen::genCodeForBBlist()

if ((call != nullptr) && (call->gtOper == GT_CALL))
{
if ((call->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0)
if ((call->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0 ||
Copy link
Member

Choose a reason for hiding this comment

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

Can we extend GenTreeCall::IsNoReturn() to cover the helper case too? Either by setting the no return flag when the helper is set, or by putting this check there?

Copy link
Member Author

@VSadov VSadov Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have tried, naively, to put

    if (s_helperCallProperties.AlwaysThrow((CorInfoHelpFunc)helper))
    {
        result->gtCallMoreFlags |= GTF_CALL_M_DOES_NOT_RETURN;
    }

in gtNewHelperCallNode

, but that resulted in

  Assertion failed 'call->gtCallType == CT_USER_FUNC' in 'System.ModuleHandle:ResolveFieldHandle(int,System.RuntimeTypeHandle[],System.Run
  timeTypeHandle[]):System.RuntimeFieldHandle:this' during 'Merge throw blocks' (IL size 283; hash 0x040825d3; FullOpts)

        if (!call->IsNoReturn())
        {
            continue;
        }

        // Sanity check -- only user funcs should be marked do not return
        assert(call->gtCallType == CT_USER_FUNC);

I could not easily tell if the assert is just an observation, that could be relaxed or indeed guarding some assumptions made in the code, so I did not continue on that path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Commenting out that assert leads to others:

  Assert failure(PID 35472 [0x00008a90], Thread: 41468 [0xa1fc]): Assertion failed 'updateCount < optNoReturnCallCount' in 'System.Tests.S
  tringComparerTests:Create_CreatesValidComparer():this' during 'Merge throw blocks' (IL size 543; hash 0xc45bc068; FullOpts)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'd prefer entering an issue on this.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, looks like there are some assumptions in the throw helper merging to sort out.

((call->AsCall()->gtCallType == CT_HELPER) &&
Compiler::s_helperCallProperties.AlwaysThrow(call->AsCall()->GetHelperNum())))
{
instGen(INS_BREAKPOINT); // This should never get executed
}
Expand Down Expand Up @@ -756,6 +758,20 @@ void CodeGen::genCodeForBBlist()

case BBJ_ALWAYS:
{
GenTree* call = block->lastNode();
if ((call != nullptr) && (call->gtOper == GT_CALL))
{
if ((call->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0 ||
((call->AsCall()->gtCallType == CT_HELPER) &&
Compiler::s_helperCallProperties.AlwaysThrow(call->AsCall()->GetHelperNum())))
{
// NOTE: We should probably never see a BBJ_ALWAYS block ending with a throw in a first place.
Copy link
Member

Choose a reason for hiding this comment

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

I take it you did see such cases? Can you open an issues for us to follow-up and switch them over to BBJ_THROWs?

Copy link
Member

Choose a reason for hiding this comment

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

Note this might largely clean itself up by adding the logic to IsNoReturn, though there is one place in morph where we still check the flag rather than the property.

Copy link
Member Author

@VSadov VSadov Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I take it you did see such cases?

Yes, I wanted this to be an assert, so it would not happen by accident in the future. To my surprise, the assert was triggered by existing code.

// If that is fixed, this condition can be just an assert.
// For the reasons why we insert a BP, see the similar code in "case BBJ_THROW:" above.
instGen(INS_BREAKPOINT); // This should never get executed
}
}

// If this block jumps to the next one, we might be able to skip emitting the jump
if (block->CanRemoveJumpToNext(compiler))
{
Expand Down
13 changes: 5 additions & 8 deletions src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,14 +212,11 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo,
ASSERT(((uintptr_t)codeOffset & 1) == 0);
#endif

if (!isActiveStackFrame)
bool executionAborted = ((UnixNativeMethodInfo*)pMethodInfo)->executionAborted;

if (!isActiveStackFrame && !executionAborted)
{
// If we are not in the active method, we are currently pointing
// to the return address. That may not be reachable after a call (if call does not return)
// or reachable via a jump and thus have a different live set.
// Therefore we simply adjust the offset to inside of call instruction.
// NOTE: The GcInfoDecoder depends on this; if you change it, you must
// revisit the GcInfoEncoder/Decoder
// the reasons for this adjustment are explained in EECodeManager::EnumGcRefs
codeOffset--;
}

Expand All @@ -230,7 +227,7 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo,
);

ICodeManagerFlags flags = (ICodeManagerFlags)0;
if (((UnixNativeMethodInfo*)pMethodInfo)->executionAborted)
if (executionAborted)
flags = ICodeManagerFlags::ExecutionAborted;

if (IsFilter(pMethodInfo))
Expand Down
13 changes: 5 additions & 8 deletions src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,10 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo,
PTR_uint8_t gcInfo;
uint32_t codeOffset = GetCodeOffset(pMethodInfo, safePointAddress, &gcInfo);

bool executionAborted = ((CoffNativeMethodInfo *)pMethodInfo)->executionAborted;

ICodeManagerFlags flags = (ICodeManagerFlags)0;
if (((CoffNativeMethodInfo *)pMethodInfo)->executionAborted)
if (executionAborted)
flags = ICodeManagerFlags::ExecutionAborted;

if (IsFilter(pMethodInfo))
Expand All @@ -446,14 +448,9 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo,
flags = (ICodeManagerFlags)(flags | ICodeManagerFlags::ActiveStackFrame);

#ifdef USE_GC_INFO_DECODER
if (!isActiveStackFrame)
if (!isActiveStackFrame && !executionAborted)
{
// If we are not in the active method, we are currently pointing
// to the return address. That may not be reachable after a call (if call does not return)
// or reachable via a jump and thus have a different live set.
// Therefore we simply adjust the offset to inside of call instruction.
// NOTE: The GcInfoDecoder depends on this; if you change it, you must
// revisit the GcInfoEncoder/Decoder
// the reasons for this adjustment are explained in EECodeManager::EnumGcRefs
codeOffset--;
}

Expand Down
20 changes: 9 additions & 11 deletions src/coreclr/vm/eetwain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1466,17 +1466,15 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pRD,
}
else
{
/* However if ExecutionAborted, then this must be one of the
* ExceptionFrames. Handle accordingly
*/
_ASSERTE(!(flags & AbortingCall) || !(flags & ActiveStackFrame));

if (flags & AbortingCall)
{
curOffs--;
LOG((LF_GCINFO, LL_INFO1000, "Adjusted GC reporting offset due to flags ExecutionAborted && AbortingCall. Now reporting GC refs for %s at offset %04x.\n",
methodName, curOffs));
}
// Since we are aborting execution, we are either in a frame that actually faulted or in a throwing call.
// * We do not need to adjust in a leaf
// * A throwing call will have unreachable <brk> after it, thus GC info is the same as before the call.
//
// Either way we do not need to adjust.

// NOTE: only fully interruptible methods may need to report anything here as without
// exception handling all current local variables are already unreachable.
// EnumerateLiveSlots will shortcircuit the partially interruptible case just a bit later.
}

// Check if we have been given an override value for relOffset
Expand Down
8 changes: 1 addition & 7 deletions src/coreclr/vm/gc_unwind_x86.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3686,13 +3686,7 @@ bool EnumGcRefsX86(PREGDISPLAY pContext,
}
else
{
/* However if ExecutionAborted, then this must be one of the
* ExceptionFrames. Handle accordingly
*/
_ASSERTE(!(flags & AbortingCall) || !(flags & ActiveStackFrame));

newCurOffs = (flags & AbortingCall) ? curOffs-1 // inside "call"
: curOffs; // at faulting instr, or start of "try"
newCurOffs = curOffs;
}

ptrOffs = 0;
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/stackwalk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ class CrawlFrame
if (!HasFaulted() && !IsIPadjusted())
{
_ASSERTE(!(flags & ActiveStackFrame));
flags |= AbortingCall;
}
}

Expand Down