Fixed problem with bpmd not working sometimes.

Fix some too earlier SOS initialization problem. Now returns an error.

The DAC interface (IXCLRDataProcess) was being created everytime an command was run and the JIT and GC notification tables where being reinitialized each time losing any JIT notifications needed to resolve a breakpoint. Only create the DAC interface instance once. It does need to be flushed each time sos is entered though.

Enable the module load and unload and exception callbacks when the DAC instance is created. This is what windbg does on Windows (along with flushing the DAC each time the target is restarted). It simplifies the breakpoint code; it no longer needs to enable/disable these notification flags.

Cleaned up places where the DAC instance (direct calls to LoadClrDebugDll) and not released properly.

4 files changed, 69 insertions, 103 deletions

diff --git a/src/ToolBox/SOS/Strike/WatchCmd.cpp b/src/ToolBox/SOS/Strike/WatchCmd.cpp
index 9457485dc7..443f1dd6ef 100644
--- a/src/ToolBox/SOS/Strike/WatchCmd.cpp
+++ b/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);
diff --git a/src/ToolBox/SOS/Strike/exts.h b/src/ToolBox/SOS/Strike/exts.h
index 8cdba9fd75..2669ff39f9 100644
--- a/src/ToolBox/SOS/Strike/exts.h
+++ b/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); \
diff --git a/src/ToolBox/SOS/Strike/strike.cpp b/src/ToolBox/SOS/Strike/strike.cpp
index b7875d01b9..a40efb7cf8 100644
--- a/src/ToolBox/SOS/Strike/strike.cpp
+++ b/src/ToolBox/SOS/Strike/strike.cpp
@@ -6319,11 +6319,6 @@ public:
 #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 @@ private:
 
         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 @@ public:
             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.
diff --git a/src/ToolBox/SOS/Strike/util.cpp b/src/ToolBox/SOS/Strike/util.cpp
index 59ede3e72f..8722845a72 100644
--- a/src/ToolBox/SOS/Strike/util.cpp
+++ b/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