Merge pull request #3783 from mikem8361/fixsosbp

Fixed problem with bpmd not working sometimes.

Author: Mike McLaughlin

src/ToolBox/SOS/Strike/WatchCmd.cpp

@@ -92,16 +92,8 @@ HRESULT WatchCmd::Remove(int index)
 HRESULT WatchCmd::Print(int expansionIndex, __in_z WCHAR* expansionPath, __in_z WCHAR* pFilterName)
 {
     HRESULT Status = S_OK;
-    if ((Status = CheckEEDll()) != S_OK)
-    {
-        EENotLoadedMessage(Status);
-        return Status;
-    }                                                           
-    if ((Status = LoadClrDebugDll()) != S_OK)
-    {
-        DACMessage(Status);
-        return Status;
-    } 
+    INIT_API_EE();
+    INIT_API_DAC();
     EnableDMLHolder dmlHolder(TRUE);
     IfFailRet(InitCorDebugInterface());
 
@@ -214,6 +206,8 @@ HRESULT WatchCmd::RenameList(__in_z WCHAR* pOldName, __in_z WCHAR* pNewName)
 HRESULT WatchCmd::SaveList(__in_z WCHAR* pSaveName)
 {
     HRESULT Status = S_OK;
+    INIT_API_EE();
+    INIT_API_DAC();
     IfFailRet(InitCorDebugInterface());
 
     RemoveList(pSaveName);

src/ToolBox/SOS/Strike/exts.h

@@ -231,18 +231,22 @@ HRESULT CheckEEDll();
     if ((Status = ExtQuery(client)) != S_OK) return Status;     \
     if ((Status = ArchQuery()) != S_OK)      return Status;     \
     ControlC = FALSE;                                           \
-    g_bDacBroken = TRUE;
+    g_bDacBroken = TRUE;                                        \
+    g_clrData = NULL;                                           \
+    g_sos = NULL;                                        
 
-#define INIT_API_NODAC()                                        \
-    INIT_API_NOEE()                                             \
+#define INIT_API_EE()                                           \
     if ((Status = CheckEEDll()) != S_OK)                        \
     {                                                           \
         EENotLoadedMessage(Status);                             \
         return Status;                                          \
     }                                                           
 
-#define INIT_API()                                              \
-    INIT_API_NODAC()                                            \
+#define INIT_API_NODAC()                                        \
+    INIT_API_NOEE()                                             \
+    INIT_API_EE()
+
+#define INIT_API_DAC()                                          \
     if ((Status = LoadClrDebugDll()) != S_OK)                   \
     {                                                           \
         DACMessage(Status);                                     \
@@ -255,6 +259,10 @@ HRESULT CheckEEDll();
     ToRelease<ISOSDacInterface> spISD(g_sos);                   \
     ResetGlobals();
 
+#define INIT_API()                                              \
+    INIT_API_NODAC()                                            \
+    INIT_API_DAC()
+
 // Attempt to initialize DAC and SOS globals, but do not "return" on failure.
 // Instead, mark the failure to initialize the DAC by setting g_bDacBroken to TRUE.
 // This should be used from extension commands that should work OK even when no
@@ -263,7 +271,6 @@ HRESULT CheckEEDll();
 // feature.
 #define INIT_API_NO_RET_ON_FAILURE()                            \
     INIT_API_NOEE()                                             \
-    g_clrData = NULL;                                           \
     if ((Status = CheckEEDll()) != S_OK)                        \
     {                                                           \
         ExtOut("Failed to find runtime DLL (%s), 0x%08x\n", MAKEDLLNAME_A("coreclr"), Status); \

src/ToolBox/SOS/Strike/strike.cpp

@@ -6319,11 +6319,6 @@ class Breakpoints
 #ifdef FEATURE_PAL
         if (m_breakpoints == NULL)
         {
-            ULONG32 flags = 0;
-            g_clrData->GetOtherNotificationFlags(&flags);
-            flags &= ~(CLRDATA_NOTIFY_ON_MODULE_LOAD | CLRDATA_NOTIFY_ON_MODULE_UNLOAD);
-            g_clrData->SetOtherNotificationFlags(flags);
-
             g_ExtServices->ClearExceptionCallback();
         }
 #endif
@@ -6652,6 +6647,7 @@ class Breakpoints
 
         ToRelease<IXCLRDataMethodDefinition> pMeth = NULL;
         mod->GetMethodDefinitionByToken(pCur->methodToken, &pMeth);
+
         // We may not need the code notification. Maybe it was ngen'd and we
         // already have the method?
         // We can delete the current entry if ResolveMethodInstances() set all BPs
@@ -6865,8 +6861,12 @@ class CNotification : public IXCLRDataExceptionNotification4
             if(method->GetRepresentativeEntryAddress(&startAddr) == S_OK)
             {
                 CHAR buffer[100];
+#ifndef FEATURE_PAL
                 sprintf_s(buffer, _countof(buffer), "bp /1 %p", (void*) (size_t) (startAddr+catcherNativeOffset));
-                g_ExtControl->Execute(DEBUG_EXECUTE_NOT_LOGGED, buffer ,0);
+#else
+                sprintf_s(buffer, _countof(buffer), "breakpoint set --one-shot --address 0x%p", (void*) (size_t) (startAddr+catcherNativeOffset));
+#endif
+                g_ExtControl->Execute(DEBUG_EXECUTE_NOT_LOGGED, buffer, 0);
             }
             g_stopOnNextCatch = FALSE;
         }
@@ -7020,7 +7020,7 @@ HRESULT HandleExceptionNotification(ILLDBServices *client)
 
 DECLARE_API(bpmd)
 {
-    INIT_API();    
+    INIT_API_NOEE();    
     MINIDUMP_NOT_SUPPORTED();    
     int i;
     char buffer[1024];    
@@ -7169,9 +7169,6 @@ DECLARE_API(bpmd)
     LPWSTR Filename = (LPWSTR)alloca(MAX_LONGPATH * sizeof(WCHAR));
 
     BOOL bNeedNotificationExceptions = FALSE;
-#ifdef FEATURE_PAL
-    BOOL bNeedModuleNotificationExceptions = FALSE;
-#endif
 
     if (pMD == NULL)
     {
@@ -7190,7 +7187,7 @@ DECLARE_API(bpmd)
             MultiByteToWideChar(CP_ACP, 0, DllName.data, -1, Filename, MAX_LONGPATH);
         }
 
-        // get modules that may need a breakpoint bound
+        // Get modules that may need a breakpoint bound
         if ((Status = CheckEEDll()) == S_OK)
         {
             if ((Status = LoadClrDebugDll()) != S_OK)
@@ -7199,23 +7196,25 @@ DECLARE_API(bpmd)
                 DACMessage(Status);
                 return Status;
             }
-            else
-            {
-                // get the module list
-                moduleList = ModuleFromName(fIsFilename ? NULL : DllName.data, &numModule);
-
-                // Its OK if moduleList is NULL
-                // There is a very normal case when checking for modules after clr is loaded
-                // but before any AppDomains or assemblies are created
-                // for example:
-                // >sxe ld:clr
-                // >g
-                // ...
-                // ModLoad: clr.dll
-                // >!bpmd Foo.dll Foo.Bar
-            }
-        }
-
+            g_bDacBroken = FALSE;                                       \
+
+            // Get the module list
+            moduleList = ModuleFromName(fIsFilename ? NULL : DllName.data, &numModule);
+
+            // Its OK if moduleList is NULL
+            // There is a very normal case when checking for modules after clr is loaded
+            // but before any AppDomains or assemblies are created
+            // for example:
+            // >sxe ld:clr
+            // >g
+            // ...
+            // ModLoad: clr.dll
+            // >!bpmd Foo.dll Foo.Bar
+        }
+        // If LoadClrDebugDll() succeeded make sure we release g_clrData
+        ToRelease<IXCLRDataProcess> spIDP(g_clrData);
+        ToRelease<ISOSDacInterface> spISD(g_sos);
+        ResetGlobals();
 
         // we can get here with EE not loaded => 0 modules
         //                      EE is loaded => 0 or more modules
@@ -7308,24 +7307,13 @@ DECLARE_API(bpmd)
                 g_bpoints.Add(Filename, lineNumber, NULL);
             }
             bNeedNotificationExceptions = TRUE;
-#ifdef FEATURE_PAL
-            bNeedModuleNotificationExceptions = TRUE;
-#endif
         }
     }
     else /* We were given a MethodDesc already */
     {
         // if we've got an explicit MD, then we better have CLR and mscordacwks loaded
-        if ((Status = CheckEEDll()) != S_OK)
-        {
-            EENotLoadedMessage(Status);
-            return Status;
-        }
-        if ((Status = LoadClrDebugDll()) != S_OK)
-        {
-            DACMessage(Status);
-            return Status;
-        } 
+        INIT_API_EE()
+        INIT_API_DAC();
 
         DacpMethodDescData MethodDescData;
         ExtOut("MethodDesc = %p\n", (ULONG64) pMD);
@@ -7371,7 +7359,6 @@ DECLARE_API(bpmd)
         else
         {
             // Must issue a pending breakpoint.
-
             if (g_sos->GetMethodDescName(pMD, mdNameLen, FunctionName, NULL) != S_OK)
             {
                 ExtOut("Unable to get method name for MethodDesc %p\n", (ULONG64)pMD);
@@ -7394,13 +7381,6 @@ DECLARE_API(bpmd)
         sprintf_s(buffer, _countof(buffer), "sxe -c \"!HandleCLRN\" clrn");
         Status = g_ExtControl->Execute(DEBUG_EXECUTE_NOT_LOGGED, buffer, 0);        
 #else
-        if (bNeedModuleNotificationExceptions)
-        {
-            ULONG32 flags = 0;
-            g_clrData->GetOtherNotificationFlags(&flags);
-            flags |= (CLRDATA_NOTIFY_ON_MODULE_LOAD | CLRDATA_NOTIFY_ON_MODULE_UNLOAD);
-            g_clrData->SetOtherNotificationFlags(flags);
-        }
         Status = g_ExtServices->SetExceptionCallback(HandleExceptionNotification);
 #endif // FEATURE_PAL
     }
@@ -14099,9 +14079,7 @@ HRESULT SetNGENCompilerFlags(DWORD flags)
 }
 
 
-
 // This is an internal-only Apollo extension to save breakpoint/watch state
-#ifndef FEATURE_PAL
 DECLARE_API(SaveState)
 {
     INIT_API_NOEE();    
@@ -14138,16 +14116,14 @@ DECLARE_API(SaveState)
     ExtOut("Session breakpoints and watch expressions saved to %s\n", filePath.data);
     return S_OK;
 }
-#endif
 
+#endif // FEATURE_PAL
 
 DECLARE_API(StopOnCatch)
 {
     INIT_API();    
     MINIDUMP_NOT_SUPPORTED();    
 
-    CHAR buffer[100];
-    sprintf_s(buffer, _countof(buffer), "sxe -c \"!HandleCLRN\" clrn");
     g_stopOnNextCatch = TRUE;
     ULONG32 flags = 0;
     g_clrData->GetOtherNotificationFlags(&flags);
@@ -14157,7 +14133,6 @@ DECLARE_API(StopOnCatch)
     return S_OK;
 }
 
-
 // This is an undocumented SOS extension command intended to help test SOS
 // It causes the Dml output to be printed to the console uninterpretted so
 // that a test script can read the commands which are hidden in the markup
@@ -14167,8 +14142,6 @@ DECLARE_API(ExposeDML)
     return S_OK;
 }
 
-#endif // FEATURE_PAL
-
 // According to kksharma the Windows debuggers always sign-extend
 // arguments when calling externally, therefore StackObjAddr 
 // conforms to CLRDATA_ADDRESS contract.

src/ToolBox/SOS/Strike/util.cpp

@@ -62,10 +62,6 @@ IXCLRDataProcess *g_clrData = NULL;
 ISOSDacInterface *g_sos = NULL;
 ICorDebugProcess *g_pCorDebugProcess = NULL;
 
-#ifdef FEATURE_PAL
-PFN_CLRDataCreateInstance g_pCLRDataCreateInstance = NULL;
-#endif // FEATURE_PAL
-
 #ifndef IfFailRet
 #define IfFailRet(EXPR) do { Status = (EXPR); if(FAILED(Status)) { return (Status); } } while (0)
 #endif
@@ -4156,8 +4152,10 @@ void ResetGlobals(void)
 
 HRESULT LoadClrDebugDll(void)
 {
+    HRESULT hr = S_OK;
 #ifdef FEATURE_PAL
-    if (g_pCLRDataCreateInstance == NULL)
+    static IXCLRDataProcess* s_clrDataProcess = NULL;
+    if (s_clrDataProcess == NULL)
     {
         int err = PAL_InitializeDLL();
         if(err != 0)
@@ -4173,20 +4171,27 @@ HRESULT LoadClrDebugDll(void)
         {
             return E_FAIL;
         }
-        g_pCLRDataCreateInstance = (PFN_CLRDataCreateInstance)GetProcAddress(hdac, "CLRDataCreateInstance");
-        if (g_pCLRDataCreateInstance == NULL)
+        PFN_CLRDataCreateInstance pCLRDataCreateInstance = (PFN_CLRDataCreateInstance)GetProcAddress(hdac, "CLRDataCreateInstance");
+        if (pCLRDataCreateInstance == NULL)
         {
             FreeLibrary(hdac);
             return E_FAIL;
         }
+        ICLRDataTarget *target = new DataTarget();
+        hr = pCLRDataCreateInstance(__uuidof(IXCLRDataProcess), target, (void**)&s_clrDataProcess);
+        if (FAILED(hr))
+        {
+            s_clrDataProcess = NULL;
+            return hr;
+        }
+        ULONG32 flags = 0;
+        s_clrDataProcess->GetOtherNotificationFlags(&flags);
+        flags |= (CLRDATA_NOTIFY_ON_MODULE_LOAD | CLRDATA_NOTIFY_ON_MODULE_UNLOAD | CLRDATA_NOTIFY_ON_EXCEPTION);
+        s_clrDataProcess->SetOtherNotificationFlags(flags);
     }
-    ICLRDataTarget *target = new DataTarget();
-    HRESULT hr = g_pCLRDataCreateInstance(__uuidof(IXCLRDataProcess), target, reinterpret_cast<void**>(&g_clrData));
-    if (FAILED(hr))
-    {
-        g_clrData = NULL;
-        return hr;
-    }
+    g_clrData = s_clrDataProcess;
+    g_clrData->AddRef();
+    g_clrData->Flush();
 #else
     WDBGEXTS_CLR_DATA_INTERFACE Query;
 
@@ -4198,13 +4203,12 @@ HRESULT LoadClrDebugDll(void)
 
     g_clrData = (IXCLRDataProcess*)Query.Iface;
 #endif
-
-    if (FAILED(g_clrData->QueryInterface(__uuidof(ISOSDacInterface), (void**)&g_sos)))
+    hr = g_clrData->QueryInterface(__uuidof(ISOSDacInterface), (void**)&g_sos);
+    if (FAILED(hr))
     {
         g_sos = NULL;
-        return E_FAIL;
+        return hr;
     }
-
     return S_OK;
 }
 
@@ -4769,18 +4773,6 @@ HRESULT InitCorDebugInterface()
         UninitCorDebugInterface();
     }
 
-    // Ensure that CLR and DAC are already loaded
-    if ((hr = CheckEEDll()) != S_OK)
-    {
-        EENotLoadedMessage(hr);
-        return hr;
-    }
-    if ((hr = LoadClrDebugDll()) != S_OK)
-    {
-        DACMessage(hr);
-        return hr;
-    }
-
     // SOS now has a statically linked version of the loader code that is normally found in mscoree/mscoreei.dll
     // Its not much code and takes a big step towards 0 install dependencies
     // Need to pick the appropriate SKU of CLR to detect

src/debug/daccess/daccess.cpp

@@ -7232,6 +7232,10 @@ ClrDataAccess::GetDacGlobals()
     {
         return CORDBG_E_MISSING_DEBUGGER_EXPORTS;
     }
+    if (g_dacGlobals.ThreadStore__s_pThreadStore == NULL)
+    {
+        return CORDBG_E_UNSUPPORTED;
+    }
     return S_OK;
 #else
     HRESULT status = E_FAIL;

src/vm/ceemain.cpp

@@ -1439,11 +1439,11 @@ HRESULT EEStartup(COINITIEE fFlags)
     PAL_TRY(COINITIEE *, pfFlags, &fFlags)
     {
 #ifndef CROSSGEN_COMPILE
+        InitializeClrNotifications();
 #ifdef FEATURE_PAL
-        DacGlobals::Initialize();
         InitializeJITNotificationTable();
+        DacGlobals::Initialize();
 #endif
-        InitializeClrNotifications();
 #endif // CROSSGEN_COMPILE
 
         EEStartupHelper(*pfFlags);