Add debuggee process stop at attach for CLI protocol.

Fix CLI and MI/GDB protocols exit behavior in case debuggee process was attached,
will detach now instead of terminate attached debuggee process.

src/debugger/managedcallback.cpp

@@ -140,6 +140,12 @@ bool ManagedCallback::CallbacksWorkerException(ICorDebugAppDomain *pAppDomain, I
     return true;
 }
 
+bool ManagedCallback::CallbacksWorkerCreateProcess()
+{
+    m_debugger.NotifyProcessCreated();
+    return false;
+}
+
 void ManagedCallback::CallbacksWorker()
 {
     std::unique_lock<std::mutex> lock(m_callbacksMutex);
@@ -169,6 +175,9 @@ void ManagedCallback::CallbacksWorker()
         case CallbackQueueCall::Exception:
             m_stopEventInProcess = CallbacksWorkerException(c.iCorAppDomain, c.iCorThread, c.EventType, c.ExcModule);
             break;
+        case CallbackQueueCall::CreateProcess:
+            m_stopEventInProcess = CallbacksWorkerCreateProcess();
+            break;
         default:
             // finish loop
             // called from destructor only, don't need call pop()
@@ -531,10 +540,32 @@ HRESULT STDMETHODCALLTYPE ManagedCallback::EvalException(ICorDebugAppDomain *pAp
 
 // https://docs.microsoft.com/en-us/dotnet/framework/unmanaged-api/debugging/icordebugmanagedcallback-createprocess-method
 // Notifies the debugger when a process has been attached or started for the first time.
+// Remarks
+// This method is not called until the common language runtime is initialized. Most of the ICorDebug methods will return CORDBG_E_NOTREADY before the CreateProcess callback.
 HRESULT STDMETHODCALLTYPE ManagedCallback::CreateProcess(ICorDebugProcess *pProcess)
 {
     LogFuncEntry();
-    m_debugger.NotifyProcessCreated();
+
+    // Important! Care about callback queue before NotifyProcessCreated() call.
+    // In case of `attach`, NotifyProcessCreated() call will notify debugger that debuggee process attached and debugger
+    // should stop debuggee process by dirrect `Pause()` call. From another side, callback queue have bunch of asynchronous
+    // added entries and, for example, `CreateThread()` could be called after this callback and broke our debugger logic.
+    ToRelease<ICorDebugAppDomainEnum> domains;
+    ICorDebugAppDomain *pAppDomain;
+    ULONG domainsFetched;
+    if (SUCCEEDED(pProcess->EnumerateAppDomains(&domains)))
+    {
+        // At this point we have only one domain for sure.
+        if (SUCCEEDED(domains->Next(1, &pAppDomain, &domainsFetched)) && domainsFetched == 1)
+        {
+            // Don't AddRef() here for pAppDomain! We get it with AddRef() from Next() and will release in m_callbacksQueue by ToRelease destructor.
+            return AddCallbackToQueue(pAppDomain, [&]()
+            {
+                m_callbacksQueue.emplace_back(CallbackQueueCall::CreateProcess, pAppDomain, nullptr, nullptr, STEP_NORMAL, ExceptionCallbackType::FIRST_CHANCE);
+            });
+        }
+    }
+
     return ContinueProcessWithCallbacksQueue(pProcess);
 }
 

src/debugger/managedcallback.h

@@ -34,7 +34,8 @@ class ManagedCallback final : public ICorDebugManagedCallback, ICorDebugManagedC
         Breakpoint,
         StepComplete,
         Break,
-        Exception
+        Exception,
+        CreateProcess
     };
 
     struct CallbackQueueEntry
@@ -75,6 +76,7 @@ class ManagedCallback final : public ICorDebugManagedCallback, ICorDebugManagedC
     bool CallbacksWorkerStepComplete(ICorDebugAppDomain *pAppDomain, ICorDebugThread *pThread, CorDebugStepReason reason);
     bool CallbacksWorkerBreak(ICorDebugAppDomain *pAppDomain, ICorDebugThread *pThread);
     bool CallbacksWorkerException(ICorDebugAppDomain *pAppDomain, ICorDebugThread *pThread, ExceptionCallbackType eventType, const std::string &excModule);
+    bool CallbacksWorkerCreateProcess();
     HRESULT AddCallbackToQueue(ICorDebugAppDomain *pAppDomain, std::function<void()> callback);
     bool HasQueuedCallbacks(ICorDebugProcess *pProcess);
     HRESULT ContinueAppDomainWithCallbacksQueue(ICorDebugAppDomain *pAppDomain);

src/debugger/manageddebugger.cpp

@@ -63,6 +63,7 @@ extern "C" const IID IID_IUnknown = { 0x00000000, 0x0000, 0x0000, {0xC0, 0x00, 0
 
 namespace
 {
+    const auto startupWaitTimeout = std::chrono::milliseconds(5000);
 
     const std::string envDOTNET_STARTUP_HOOKS = "DOTNET_STARTUP_HOOKS";
 #ifdef FEATURE_PAL
@@ -117,8 +118,10 @@ namespace
 
 void ManagedDebugger::NotifyProcessCreated()
 {
-    std::lock_guard<std::mutex> lock(m_processAttachedMutex);
+    std::unique_lock<std::mutex> lock(m_processAttachedMutex);
     m_processAttachedState = ProcessAttachedState::Attached;
+    lock.unlock();
+    m_processAttachedCV.notify_one();
 }
 
 void ManagedDebugger::NotifyProcessExited()
@@ -183,8 +186,6 @@ ManagedDebugger::ManagedDebugger() :
     m_justMyCode(true),
     m_stepFiltering(true),
     m_hotReload(false),
-    m_startupReady(false),
-    m_startupResult(S_OK),
     m_unregisterToken(nullptr),
     m_processId(0),
     m_ioredirect(
@@ -400,19 +401,13 @@ VOID ManagedDebugger::StartupCallback(IUnknown *pCordb, PVOID parameter, HRESULT
 {
     ManagedDebugger *self = static_cast<ManagedDebugger*>(parameter);
 
-    std::unique_lock<std::mutex> lock(self->m_startupMutex);
-
-    self->m_startupResult = FAILED(hr) ? hr : self->Startup(pCordb, self->m_processId);
-    self->m_startupReady = true;
+    self->Startup(pCordb, self->m_processId);
 
     if (self->m_unregisterToken)
     {
         self->m_dbgshim.UnregisterForRuntimeStartup(self->m_unregisterToken);
         self->m_unregisterToken = nullptr;
     }
-
-    lock.unlock();
-    self->m_startupCV.notify_one();
 }
 
 // From dbgshim.cpp
@@ -640,7 +635,6 @@ static void PrepareSystemEnvironmentArg(const std::map<std::string, std::string>
 
 HRESULT ManagedDebugger::RunProcess(const std::string& fileExec, const std::vector<std::string>& execArgs)
 {
-    static const auto startupCallbackWaitTimeout = std::chrono::milliseconds(5000);
     HRESULT Status;
 
     IfFailRet(CheckNoProcess());
@@ -652,7 +646,6 @@ HRESULT ManagedDebugger::RunProcess(const std::string& fileExec, const std::vect
         ss << " \"" << EscapeShellArg(arg) << "\"";
     }
 
-    m_startupReady = false;
     m_clrPath.clear();
 
     HANDLE resumeHandle = 0; // Fake thread handle for the process resume
@@ -689,26 +682,13 @@ HRESULT ManagedDebugger::RunProcess(const std::string& fileExec, const std::vect
     IfFailRet(m_dbgshim.ResumeProcess(resumeHandle));
     m_dbgshim.CloseResumeHandle(resumeHandle);
 
-    // Wait for ManagedDebugger::StartupCallback to complete
-
-    /// FIXME: if the process exits too soon the ManagedDebugger::StartupCallback()
-    /// is never called (bug in dbgshim?).
-    /// The workaround is to wait with timeout.
-    const auto now = std::chrono::system_clock::now();
-
-    std::unique_lock<std::mutex> lock(m_startupMutex);
-    if (!m_startupCV.wait_until(lock, now + startupCallbackWaitTimeout, [this](){return m_startupReady;}))
-    {
-        // Timed out
-        m_dbgshim.UnregisterForRuntimeStartup(m_unregisterToken);
-        m_unregisterToken = nullptr;
+    std::unique_lock<std::mutex> lockAttachedMutex(m_processAttachedMutex);
+    if (!m_processAttachedCV.wait_for(lockAttachedMutex, startupWaitTimeout, [this]{return m_processAttachedState == ProcessAttachedState::Attached;}))
         return E_FAIL;
-    }
 
-    if (SUCCEEDED(m_startupResult))
-        m_sharedProtocol->EmitExecEvent(PID{m_processId}, fileExec);
+   m_sharedProtocol->EmitExecEvent(PID{m_processId}, fileExec);
 
-    return m_startupResult;
+    return S_OK;
 }
 
 HRESULT ManagedDebugger::CheckNoProcess()
@@ -838,7 +818,13 @@ HRESULT ManagedDebugger::AttachToProcess(DWORD pid)
     IfFailRet(m_dbgshim.CreateDebuggingInterfaceFromVersionEx(CorDebugVersion_4_0, pBuffer, &pCordb));
 
     m_unregisterToken = nullptr;
-    return Startup(pCordb, pid);
+    IfFailRet(Startup(pCordb, pid));
+
+    std::unique_lock<std::mutex> lockAttachedMutex(m_processAttachedMutex);
+    if (!m_processAttachedCV.wait_for(lockAttachedMutex, startupWaitTimeout, [this]{return m_processAttachedState == ProcessAttachedState::Attached;}))
+        return E_FAIL;
+
+    return S_OK;
 }
 
 HRESULT ManagedDebugger::GetExceptionInfo(ThreadId threadId, ExceptionInfo &exceptionInfo)

src/debugger/manageddebugger.h

@@ -91,11 +91,6 @@ class ManagedDebugger : public IDebugger
     bool m_stepFiltering;
     bool m_hotReload;
 
-    std::mutex m_startupMutex;
-    std::condition_variable m_startupCV;
-    bool m_startupReady;
-    HRESULT m_startupResult;
-
     PVOID m_unregisterToken;
     DWORD m_processId;
     std::string m_clrPath;

src/main.cpp

@@ -496,6 +496,9 @@ int
         if (run || pidDebuggee)
             cliProtocol->SetRunningState();
 
+        if (pidDebuggee != 0)
+            cliProtocol->Pause();
+
         // run commands passed in command line via '-ex' option
         cliProtocol->Source({initCommands});
     }

src/protocols/cliprotocol.cpp

@@ -1566,7 +1566,7 @@ HRESULT CLIProtocol::doCommand<CommandTag::Quit>(const std::vector<std::string>
     // no mutex locking needed here
     m_exit = true;
     m_sources.reset(nullptr);
-    m_sharedDebugger->Disconnect(IDebugger::DisconnectAction::DisconnectTerminate);
+    m_sharedDebugger->Disconnect(); // Terminate debuggee process if debugger ran this process and detach in case debugger was attached to it.
     return S_OK;
 }
 
@@ -1645,16 +1645,9 @@ HRESULT CLIProtocol::doCommand<CommandTag::Attach>(const std::vector<std::string
     applyCommandMode();
     lock.unlock();
 
-    Status = m_sharedDebugger->ConfigurationDone();
-    if (SUCCEEDED(Status))
-    {
-        output = "^running";
-
-        lock.lock();
-        m_processStatus = Running;
-        m_state_cv.notify_all();
-    }
-    return Status;
+    IfFailRet(m_sharedDebugger->ConfigurationDone());
+    IfFailRet(m_sharedDebugger->Pause(ThreadId::AllThreads));
+    return S_OK;
 }
 
 template <>
@@ -2323,7 +2316,7 @@ void CLIProtocol::CommandLoop()
 
     printf("^exit\n");
 
-    m_sharedDebugger->Disconnect(IDebugger::DisconnectAction::DisconnectTerminate);
+    m_sharedDebugger->Disconnect(); // Terminate debuggee process if debugger ran this process and detach in case debugger was attached to it.
 
     linenoiseHistorySave(HistoryFileName);
     linenoiseHistoryFree();

src/protocols/cliprotocol.h

@@ -202,6 +202,9 @@ class CLIProtocol : public IProtocol
         virtual ~LineReader() {}
     };
 
+    // pause debugee execution
+    void Pause();
+
 private:
     // This function should be used by any others CLIProtocol's functions
     // to read input lines for interpreting (commands, etc...)
@@ -228,9 +231,6 @@ class CLIProtocol : public IProtocol
     static CLIProtocol* g_console_owner;
     static std::mutex g_console_mutex; // mutex which protect g_console_owner
 
-    // pause debugee execution
-    void Pause();
-
     // process Ctrl-C events
     static void interruptHandler();
 

src/protocols/miprotocol.cpp

@@ -784,7 +784,7 @@ static HRESULT HandleCommand(std::shared_ptr<IDebugger> &sharedDebugger, Breakpo
         return variablesHandle.DeleteVar(args.at(0));
     }},
     { "gdb-exit", [&](const std::vector<std::string> &args, std::string &output) -> HRESULT {
-        sharedDebugger->Disconnect(IDebugger::DisconnectAction::DisconnectTerminate);
+        sharedDebugger->Disconnect(); // Terminate debuggee process if debugger ran this process and detach in case debugger was attached to it.
         return S_OK;
     }},
     { "file-exec-and-symbols", [&](const std::vector<std::string> &args, std::string &output) -> HRESULT {
@@ -1048,7 +1048,7 @@ void MIProtocol::CommandLoop()
     }
 
     if (!m_exit)
-        m_sharedDebugger->Disconnect(IDebugger::DisconnectAction::DisconnectTerminate);
+        m_sharedDebugger->Disconnect(); // Terminate debuggee process if debugger ran this process and detach in case debugger was attached to it.
 
     Printf("%s^exit\n", token.c_str());
     Printf("(gdb)\n");