From 4af002be887572569458a8e14434ba51238330b3 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 71f59672eba1c..4329c04e7264c 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -4126,7 +4126,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 @@ -4263,7 +4266,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 99e09ebbc3582d2a02b8f5a53ff3014e21cf6abc 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 fd26e7fe3135d..f21202545142b 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7410,7 +7410,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 911ab782daa7db5023b68b54a692ce3daa7b7f2a 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 f21202545142b..ac374bb7aaf0b 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7410,7 +7410,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 4329c04e7264c..ea189de6260ad 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -4126,10 +4126,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 @@ -4266,10 +4263,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; @@ -5753,7 +5747,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); @@ -5938,7 +5934,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 a5d573cf0b2b9b7b5dd95bbb731f23a17b9cb233 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 ac374bb7aaf0b..7b097ec4bce72 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7410,7 +7410,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 75de07650f3c4b875408d1f0d7f55bb98d3d68ba 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 ea189de6260ad..3bd0b0c59e06d 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -5749,7 +5749,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); @@ -5936,10 +5936,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 499db0f0ba9f72171698899fa6544f5a8d93ea6e 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 7b097ec4bce72..c18e5fdc0c37d 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7411,7 +7411,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 3bd0b0c59e06d..b9cc68a82a523 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -5936,10 +5936,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 365da67a091a286a68ca5de6eab13473ddd346f8 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 c18e5fdc0c37d..4e1736c96583a 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -7412,7 +7412,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 cfeb6cd57fc5f2c85d016f43dcdef2115f405ab0 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 b9cc68a82a523..2ddc2c3b120c9 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -5746,10 +5746,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); @@ -5936,7 +5935,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; }