From 01e183f55d3ee7da4184321af550f69e68435311 Mon Sep 17 00:00:00 2001 From: Thays Grazia Date: Thu, 5 Dec 2024 11:41:37 -0300 Subject: [PATCH 1/8] Fixing step becomes a go --- src/coreclr/vm/threadsuspend.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 6bc046e2b40bd4..8483a42d616d1a 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -4112,7 +4112,10 @@ bool Thread::SysStartSuspendForDebug(AppDomain *pAppDomain) #if defined(FEATURE_THREAD_ACTIVATION) // Inject an activation that will interrupt the thread and try to bring it to a safe point - thread->InjectActivation(Thread::ActivationReason::SuspendForDebugger); + if ((thread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) + { + thread->InjectActivation(Thread::ActivationReason::SuspendForDebugger); + } #endif // FEATURE_THREAD_ACTIVATION && TARGET_WINDOWS } else @@ -4249,7 +4252,10 @@ bool Thread::SysSweepThreadsForDebug(bool forceSync) #if defined(FEATURE_THREAD_ACTIVATION) // Inject an activation that will interrupt the thread and try to bring it to a safe point - thread->InjectActivation(Thread::ActivationReason::SuspendForDebugger); + if ((thread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) + { + thread->InjectActivation(Thread::ActivationReason::SuspendForDebugger); + } #endif // FEATURE_THREAD_ACTIVATION && TARGET_WINDOWS continue; From f8ea98871c1336e83776e0b0a79f1d146a417423 Mon Sep 17 00:00:00 2001 From: Thays Grazia Date: Thu, 5 Dec 2024 17:05:47 -0300 Subject: [PATCH 2/8] Trying to avoid step that becomes a go --- src/coreclr/debug/ee/controller.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 67baeb90ae24ef..9a5cea9cc35360 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7568,7 +7568,10 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) if (!g_pEEInterface->IsManagedNativeCode(ip)) { LOG((LF_CORDB,LL_INFO10000, "DS::TSS: not in managed code, Returning false (case 0)!\n")); - DisableSingleStep(); + if ((thread->m_State & Thread::TS_DebugWillSync) == 0) + { + DisableSingleStep(); + } return false; } From 8b78bbdf138f61aac8ae321087678ab72b1730de Mon Sep 17 00:00:00 2001 From: Thays Grazia Date: Fri, 6 Dec 2024 14:35:19 -0300 Subject: [PATCH 3/8] Adding comments and more protections. --- src/coreclr/debug/ee/controller.cpp | 5 +++++ src/coreclr/vm/threadsuspend.cpp | 21 +++++++++++---------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 9a5cea9cc35360..7abb7757e2c85a 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7568,7 +7568,12 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) if (!g_pEEInterface->IsManagedNativeCode(ip)) { LOG((LF_CORDB,LL_INFO10000, "DS::TSS: not in managed code, Returning false (case 0)!\n")); + // Sometimes we can get here coming with a CallStack that is coming from an APC + // this will disable the single stepping and wrongly resume an app that the user + // is stepping. +#ifdef FEATURE_THREAD_ACTIVATION if ((thread->m_State & Thread::TS_DebugWillSync) == 0) +#endif { DisableSingleStep(); } diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 8483a42d616d1a..95a08c0fb0e95f 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -4112,10 +4112,7 @@ bool Thread::SysStartSuspendForDebug(AppDomain *pAppDomain) #if defined(FEATURE_THREAD_ACTIVATION) // Inject an activation that will interrupt the thread and try to bring it to a safe point - if ((thread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) - { - thread->InjectActivation(Thread::ActivationReason::SuspendForDebugger); - } + thread->InjectActivation(Thread::ActivationReason::SuspendForDebugger); #endif // FEATURE_THREAD_ACTIVATION && TARGET_WINDOWS } else @@ -4252,10 +4249,7 @@ bool Thread::SysSweepThreadsForDebug(bool forceSync) #if defined(FEATURE_THREAD_ACTIVATION) // Inject an activation that will interrupt the thread and try to bring it to a safe point - if ((thread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) - { - thread->InjectActivation(Thread::ActivationReason::SuspendForDebugger); - } + thread->InjectActivation(Thread::ActivationReason::SuspendForDebugger); #endif // FEATURE_THREAD_ACTIVATION && TARGET_WINDOWS continue; @@ -5739,7 +5733,9 @@ BOOL CheckActivationSafePoint(SIZE_T ip) // The criteria for safe activation is to be running managed code. // Also we are not interested in handling interruption if we are already in preemptive mode. - BOOL isActivationSafePoint = pThread != NULL && + // Also we are not interested in handling interruption if we are single stepping + BOOL isActivationSafePoint = pThread != NULL && + (pThread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0 && pThread->PreemptiveGCDisabled() && ExecutionManager::IsManagedCode(ip); @@ -5924,7 +5920,12 @@ bool Thread::InjectActivation(ActivationReason reason) { return true; } - + // Avoid APC calls when the thread is in single step state to avoid any + // wrong resume because it's running a native code. + if ((m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) + { + return false; + } #ifdef FEATURE_SPECIAL_USER_MODE_APC _ASSERTE(UseSpecialUserModeApc()); From 40854f4cc69bbafc495eb2822a33cc1c42cfffe9 Mon Sep 17 00:00:00 2001 From: Thays Grazia Date: Fri, 6 Dec 2024 14:46:34 -0300 Subject: [PATCH 4/8] fixing comment --- src/coreclr/debug/ee/controller.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 7abb7757e2c85a..cf3c38fd7f7f94 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7568,7 +7568,7 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) if (!g_pEEInterface->IsManagedNativeCode(ip)) { LOG((LF_CORDB,LL_INFO10000, "DS::TSS: not in managed code, Returning false (case 0)!\n")); - // Sometimes we can get here coming with a CallStack that is coming from an APC + // Sometimes we can get here with a callstack that is coming from an APC // this will disable the single stepping and wrongly resume an app that the user // is stepping. #ifdef FEATURE_THREAD_ACTIVATION From f8b5368a057f478ec90a2f41f63faf5234c5a5c4 Mon Sep 17 00:00:00 2001 From: Thays Grazia Date: Sat, 7 Dec 2024 18:02:16 -0300 Subject: [PATCH 5/8] Checking if removing this comments, CI failures are gone. --- src/coreclr/vm/threadsuspend.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 95a08c0fb0e95f..41677d62b8d29e 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -5735,7 +5735,7 @@ BOOL CheckActivationSafePoint(SIZE_T ip) // Also we are not interested in handling interruption if we are already in preemptive mode. // Also we are not interested in handling interruption if we are single stepping BOOL isActivationSafePoint = pThread != NULL && - (pThread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0 && + //(pThread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0 && pThread->PreemptiveGCDisabled() && ExecutionManager::IsManagedCode(ip); @@ -5922,10 +5922,10 @@ bool Thread::InjectActivation(ActivationReason reason) } // Avoid APC calls when the thread is in single step state to avoid any // wrong resume because it's running a native code. - if ((m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) + /*if ((m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) { return false; - } + }*/ #ifdef FEATURE_SPECIAL_USER_MODE_APC _ASSERTE(UseSpecialUserModeApc()); From 932b8f4da171559896ffe67ead8cca9936e681c7 Mon Sep 17 00:00:00 2001 From: Thays Grazia Date: Sat, 7 Dec 2024 19:59:27 -0300 Subject: [PATCH 6/8] Adding part of the changes to understand the failures on CI --- src/coreclr/debug/ee/controller.cpp | 2 +- src/coreclr/vm/threadsuspend.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 3f2ffad5b4660e..235c734dc7dd7e 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7586,7 +7586,7 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) { LOG((LF_CORDB,LL_INFO10000, "DS::TSS: not in managed code, Returning false (case 0)!\n")); // Sometimes we can get here with a callstack that is coming from an APC - // this will disable the single stepping and wrongly resume an app that the user + // this will disable the single stepping and incorrectly resume an app that the user // is stepping. #ifdef FEATURE_THREAD_ACTIVATION if ((thread->m_State & Thread::TS_DebugWillSync) == 0) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 41677d62b8d29e..782001558b29f1 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -5922,10 +5922,10 @@ bool Thread::InjectActivation(ActivationReason reason) } // Avoid APC calls when the thread is in single step state to avoid any // wrong resume because it's running a native code. - /*if ((m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) + if ((m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) { return false; - }*/ + } #ifdef FEATURE_SPECIAL_USER_MODE_APC _ASSERTE(UseSpecialUserModeApc()); From e6c35bab71aa9f802b950dd2fb64de6849ad03e3 Mon Sep 17 00:00:00 2001 From: Thays Grazia Date: Sat, 7 Dec 2024 20:00:02 -0300 Subject: [PATCH 7/8] Update src/coreclr/debug/ee/controller.cpp Co-authored-by: mikelle-rogers <45022607+mikelle-rogers@users.noreply.github.com> --- src/coreclr/debug/ee/controller.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 235c734dc7dd7e..544bc824aa0e0e 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7587,7 +7587,7 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) LOG((LF_CORDB,LL_INFO10000, "DS::TSS: not in managed code, Returning false (case 0)!\n")); // Sometimes we can get here with a callstack that is coming from an APC // this will disable the single stepping and incorrectly resume an app that the user - // is stepping. + // is stepping through. #ifdef FEATURE_THREAD_ACTIVATION if ((thread->m_State & Thread::TS_DebugWillSync) == 0) #endif From c07d85ab1a4ad6ffcaf43e9dc6c83264a7dbfdec Mon Sep 17 00:00:00 2001 From: Thays Grazia Date: Sat, 7 Dec 2024 21:28:29 -0300 Subject: [PATCH 8/8] Fixing wrong fix. --- src/coreclr/vm/threadsuspend.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 782001558b29f1..505ceb174684f3 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -5732,10 +5732,9 @@ BOOL CheckActivationSafePoint(SIZE_T ip) Thread *pThread = GetThreadNULLOk(); // The criteria for safe activation is to be running managed code. - // Also we are not interested in handling interruption if we are already in preemptive mode. - // Also we are not interested in handling interruption if we are single stepping + // Also we are not interested in handling interruption if we are already in preemptive mode nor if we are single stepping BOOL isActivationSafePoint = pThread != NULL && - //(pThread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0 && + (pThread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0 && pThread->PreemptiveGCDisabled() && ExecutionManager::IsManagedCode(ip); @@ -5922,7 +5921,7 @@ bool Thread::InjectActivation(ActivationReason reason) } // Avoid APC calls when the thread is in single step state to avoid any // wrong resume because it's running a native code. - if ((m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0) + if ((m_StateNC & Thread::TSNC_DebuggerIsStepping) != 0) { return false; }