Misc SOS fixes (#2746)

* Misc SOS fixes

Cleanup g_symbolStoreInitialized flag. DisplayStoreStore() wasn't displaying the symbol
path set from the dbgeng .sympath change notification. Simplified the symbol service
initialization logic.

* Make DebugServices tests more reliable

* Always use diasymreader if no managed symbol service

* Actually flush the clrprocess/cordebug instances

diff --git a/src/SOS/SOS.Hosting/RuntimeWrapper.cs b/src/SOS/SOS.Hosting/RuntimeWrapper.cs
index 71cf779bf4a3e18433c5030aa416a8e95806c0cf..62928a90df1f259216072a95abc29457b400047c 100644 (file)
--- a/src/SOS/SOS.Hosting/RuntimeWrapper.cs
+++ b/src/SOS/SOS.Hosting/RuntimeWrapper.cs
@@ -135,13 +135,13 @@ namespace SOS.Hosting
         private void Flush()
         {
             // TODO: there is a better way to flush _corDebugProcess with ICorDebugProcess4::ProcessStateChanged(FLUSH_ALL)
-            if (_corDebugProcess == IntPtr.Zero)
+            if (_corDebugProcess != IntPtr.Zero)
             {
                 COMHelper.Release(_corDebugProcess);
                 _corDebugProcess = IntPtr.Zero;
             }
             // TODO: there is a better way to flush _clrDataProcess with ICLRDataProcess::Flush()
-            if (_clrDataProcess == IntPtr.Zero)
+            if (_clrDataProcess != IntPtr.Zero)
             {
                 COMHelper.Release(_clrDataProcess);
                 _clrDataProcess = IntPtr.Zero;

diff --git a/src/SOS/SOS.Hosting/SymbolServiceWrapper.cs b/src/SOS/SOS.Hosting/SymbolServiceWrapper.cs
index 9b5f62dc09efaddbc5659802fd7fc9c2df925ead..25dd90b3abf72307437d7a112e3fd46153e833cd 100644 (file)
--- a/src/SOS/SOS.Hosting/SymbolServiceWrapper.cs
+++ b/src/SOS/SOS.Hosting/SymbolServiceWrapper.cs
@@ -273,52 +273,55 @@ namespace SOS.Hosting
             int moduleIndexSize,
             IntPtr moduleIndex)
         {
-            try
+            if (_symbolService.IsSymbolStoreEnabled)
             {
-                KeyTypeFlags flags = specialKeys ? KeyTypeFlags.DacDbiKeys : KeyTypeFlags.IdentityKey;
-                byte[] id = new byte[moduleIndexSize];
-                Marshal.Copy(moduleIndex, id, 0, moduleIndexSize);
-
-                IEnumerable<SymbolStoreKey> keys = null;
-                switch (config)
+                try
                 {
-                    case RuntimeConfiguration.UnixCore:
-                        keys = ELFFileKeyGenerator.GetKeys(flags, moduleFilePath, id, symbolFile: false, symbolFileName: null);
-                        break;
+                    KeyTypeFlags flags = specialKeys ? KeyTypeFlags.DacDbiKeys : KeyTypeFlags.IdentityKey;
+                    byte[] id = new byte[moduleIndexSize];
+                    Marshal.Copy(moduleIndex, id, 0, moduleIndexSize);
 
-                    case RuntimeConfiguration.OSXCore:
-                        keys = MachOFileKeyGenerator.GetKeys(flags, moduleFilePath, id, symbolFile: false, symbolFileName: null);
-                        break;
-
-                    case RuntimeConfiguration.WindowsCore:
-                    case RuntimeConfiguration.WindowsDesktop:
-                        uint timeStamp = BitConverter.ToUInt32(id, 0);
-                        uint fileSize = BitConverter.ToUInt32(id, 4);
-                        SymbolStoreKey key = PEFileKeyGenerator.GetKey(moduleFilePath, timeStamp, fileSize);
-                        keys = new SymbolStoreKey[] { key };
-                        break;
+                    IEnumerable<SymbolStoreKey> keys = null;
+                    switch (config)
+                    {
+                        case RuntimeConfiguration.UnixCore:
+                            keys = ELFFileKeyGenerator.GetKeys(flags, moduleFilePath, id, symbolFile: false, symbolFileName: null);
+                            break;
+
+                        case RuntimeConfiguration.OSXCore:
+                            keys = MachOFileKeyGenerator.GetKeys(flags, moduleFilePath, id, symbolFile: false, symbolFileName: null);
+                            break;
+
+                        case RuntimeConfiguration.WindowsCore:
+                        case RuntimeConfiguration.WindowsDesktop:
+                            uint timeStamp = BitConverter.ToUInt32(id, 0);
+                            uint fileSize = BitConverter.ToUInt32(id, 4);
+                            SymbolStoreKey key = PEFileKeyGenerator.GetKey(moduleFilePath, timeStamp, fileSize);
+                            keys = new SymbolStoreKey[] { key };
+                            break;
+
+                        default:
+                            Trace.TraceError("LoadNativeSymbolsFromIndex: unsupported platform {0}", config);
+                            return;
+                    }
+                    foreach (SymbolStoreKey key in keys)
+                    {
+                        string moduleFileName = Path.GetFileName(key.FullPathName);
+                        Trace.TraceInformation("{0} {1}", key.FullPathName, key.Index);
 
-                    default:
-                        Trace.TraceError("LoadNativeSymbolsFromIndex: unsupported platform {0}", config);
-                        return;
+                        string downloadFilePath = _symbolService.DownloadFile(key);
+                        if (downloadFilePath != null)
+                        {
+                            Trace.TraceInformation("{0}: {1}", moduleFileName, downloadFilePath);
+                            callback(parameter, moduleFileName, downloadFilePath);
+                        }
+                    }
                 }
-                foreach (SymbolStoreKey key in keys)
+                catch (Exception ex) when (ex is BadInputFormatException || ex is InvalidVirtualAddressException || ex is TaskCanceledException)
                 {
-                    string moduleFileName = Path.GetFileName(key.FullPathName);
-                    Trace.TraceInformation("{0} {1}", key.FullPathName, key.Index);
-
-                    string downloadFilePath = _symbolService.DownloadFile(key);
-                    if (downloadFilePath != null)
-                    {
-                        Trace.TraceInformation("{0}: {1}", moduleFileName, downloadFilePath);
-                        callback(parameter, moduleFileName, downloadFilePath);
-                    }
+                    Trace.TraceError("{0} - {1}", ex.Message, moduleFilePath);
                 }
             }
-            catch (Exception ex) when (ex is BadInputFormatException || ex is InvalidVirtualAddressException || ex is TaskCanceledException)
-            {
-                Trace.TraceError("{0} - {1}", ex.Message, moduleFilePath);
-            }
         }
 
         /// <summary>

diff --git a/src/SOS/Strike/platform/runtimeimpl.cpp b/src/SOS/Strike/platform/runtimeimpl.cpp
index 463b4ef758a4372acadc459f9ccf332e57b61f54..c0ec1143c4a72eab9dd21d9a14afe4507e58dff7 100644 (file)
--- a/src/SOS/Strike/platform/runtimeimpl.cpp
+++ b/src/SOS/Strike/platform/runtimeimpl.cpp
@@ -700,15 +700,13 @@ void Runtime::DisplayStatus()
     }
 }
 
-extern bool g_symbolStoreInitialized;
-
 /**********************************************************************\
  * Attempt to download the runtime modules (runtime, DAC and DBI)
 \**********************************************************************/
 void Runtime::LoadRuntimeModules()
 {
     HRESULT hr = InitializeSymbolService();
-    if (SUCCEEDED(hr) && g_symbolStoreInitialized)
+    if (SUCCEEDED(hr))
     {
         if (m_runtimeInfo != nullptr)
         {

diff --git a/src/SOS/Strike/symbols.cpp b/src/SOS/Strike/symbols.cpp
index 20e3bd50899e25f76fad8d700bb7cf469f57f0fb..649f369b124d06b8ac090251be173d64764e9dac 100644 (file)
--- a/src/SOS/Strike/symbols.cpp
+++ b/src/SOS/Strike/symbols.cpp
@@ -34,8 +34,6 @@
 #define IfFailRet(EXPR) do { Status = (EXPR); if(FAILED(Status)) { return (Status); } } while (0)
 #endif
 
-bool g_symbolStoreInitialized = false;
-
 #ifndef FEATURE_PAL
 HMODULE g_hmoduleSymBinder = nullptr;
 ISymUnmanagedBinder3 *g_pSymBinder = nullptr;
@@ -78,15 +76,16 @@ extern "C" void STDMETHODCALLTYPE SOSUninitializeByHost()
 \**********************************************************************/
 HRESULT InitializeSymbolService()
 {
-    if (!g_symbolStoreInitialized)
+    static bool initialized = false;
+    if (!initialized)
     {
         ISymbolService* symbolService = GetSymbolService();
         if (symbolService == nullptr) {
             return E_NOINTERFACE;
         }
-        g_symbolStoreInitialized = symbolService->IsSymbolStoreEnabled();
+        initialized = true;
 #ifndef FEATURE_PAL
-        // When SOS is hosted on dotnet-dump, the ExtensionApis are not set so 
+        // When SOS is hosted on dotnet-dump on Windows, the ExtensionApis are not set so 
         // the expression evaluation function needs to be supplied.
         if (GetExpression == nullptr)
         {
@@ -96,6 +95,7 @@ HRESULT InitializeSymbolService()
         }
 #endif
         OnUnloadTask::Register([]() {
+            initialized = false;
             DisableSymbolStore();
         });
     }
@@ -137,7 +137,6 @@ HRESULT InitializeSymbolStore(
             return E_FAIL;
         }
     }
-    g_symbolStoreInitialized = true;
     return S_OK;
 }
 
@@ -175,9 +174,13 @@ static void LoadNativeSymbolsCallback(void* param, const char* moduleFilePath, U
 \**********************************************************************/
 HRESULT LoadNativeSymbols(bool runtimeOnly)
 {
-    if (g_symbolStoreInitialized)
+    ISymbolService* symbolService = GetSymbolService();
+    if (symbolService != nullptr)
     {
-        return g_ExtServices2->LoadNativeSymbols(runtimeOnly, LoadNativeSymbolsCallback);
+        if (symbolService->IsSymbolStoreEnabled())
+        {
+            return g_ExtServices2->LoadNativeSymbols(runtimeOnly, LoadNativeSymbolsCallback);
+        }
     }
     return E_FAIL;
 }
@@ -189,9 +192,10 @@ HRESULT LoadNativeSymbols(bool runtimeOnly)
 \**********************************************************************/
 void DisplaySymbolStore()
 {
-    if (g_symbolStoreInitialized)
+    ISymbolService* symbolService = GetSymbolService();
+    if (symbolService != nullptr)
     {
-        GetSymbolService()->DisplaySymbolStore([] (const char* message) {
+        symbolService->DisplaySymbolStore([] (const char* message) {
             ExtOut(message);
             ExtOut("\n");
         });
@@ -203,10 +207,10 @@ void DisplaySymbolStore()
 \**********************************************************************/
 void DisableSymbolStore()
 {
-    if (g_symbolStoreInitialized)
+    ISymbolService* symbolService = GetSymbolService();
+    if (symbolService != nullptr)
     {
-        g_symbolStoreInitialized = false;
-        GetSymbolService()->DisableSymbolStore();
+        symbolService->DisableSymbolStore();
     }
 }
 
@@ -226,7 +230,6 @@ HRESULT GetMetadataLocator(
 {
     HRESULT Status = S_OK;
     IfFailRet(InitializeSymbolService());
-
     return GetSymbolService()->GetMetadataLocator(imagePath, imageTimestamp, imageSize, mvid, mdRva, flags, bufferSize, buffer, dataSize);
 }
 
@@ -438,7 +441,7 @@ HRESULT SymbolReader::LoadSymbols(___in IMetaDataImport* pMD, ___in IXCLRDataMod
             ExtOut("LoadSymbols GetClrModuleImages FAILED 0x%08x\n", hr);
             return hr;
         }
-        if (!HasPortablePDB(moduleBase))
+        if (GetSymbolService() == nullptr || !HasPortablePDB(moduleBase))
         {
             hr = LoadSymbolsForWindowsPDB(pMD, moduleBase, pModuleName, FALSE);
             if (SUCCEEDED(hr))
@@ -453,9 +456,9 @@ HRESULT SymbolReader::LoadSymbols(___in IMetaDataImport* pMD, ___in IXCLRDataMod
     }
 
 #ifndef FEATURE_PAL
-    // TODO: in-memory windows PDB not supported
-    if (!HasPortablePDB(moduleData.LoadedPEAddress))
+    if (GetSymbolService() == nullptr || !HasPortablePDB(moduleData.LoadedPEAddress))
     {
+           // TODO: in-memory windows PDB not supported
         hr = LoadSymbolsForWindowsPDB(pMD, moduleData.LoadedPEAddress, pModuleName, moduleData.IsFileLayout);
         if (SUCCEEDED(hr))
         {

diff --git a/src/tests/Microsoft.Diagnostics.DebugServices.UnitTests/TestDataReader.cs b/src/tests/Microsoft.Diagnostics.DebugServices.UnitTests/TestDataReader.cs
index 99f49d6ddce1252ed5237965a946677deb97ad43..f7af9dd2df395b92248b8f8453b8ad6737905be5 100644 (file)
--- a/src/tests/Microsoft.Diagnostics.DebugServices.UnitTests/TestDataReader.cs
+++ b/src/tests/Microsoft.Diagnostics.DebugServices.UnitTests/TestDataReader.cs
@@ -71,6 +71,7 @@ namespace Microsoft.Diagnostics.DebugServices.UnitTests
             /// <param name="result">resulting object</param>
             public static void GetValue(Type type, string valueString, ref object result)
             {
+                valueString = valueString.Trim();
                 if (type == typeof(string))
                 {
                     result = valueString ?? "";
@@ -287,24 +288,24 @@ namespace Microsoft.Diagnostics.DebugServices.UnitTests
 
                             if (nullableType != null && memberValue == null)
                             {
-                                memberValue = "";
+                                memberValue = string.Empty;
                             }
                             else if (memberType == typeof(string))
                             {
-                                memberValue ??= "";
+                                memberValue ??= string.Empty;
                             }
                             else if (memberValue is ImmutableArray<byte> buildId)
                             {
                                 memberType = typeof(string);
-                                memberValue = !buildId.IsDefaultOrEmpty ? string.Concat(buildId.Select((b) => b.ToString("x2"))) : "";
+                                memberValue = !buildId.IsDefaultOrEmpty ? string.Concat(buildId.Select((b) => b.ToString("x2"))) : string.Empty;
                             }
                             else if (!memberType.IsPrimitive && !memberType.IsEnum)
                             {
                                 memberType = typeof(string);
-                                memberValue = memberValue?.ToString() ?? "";
+                                memberValue = memberValue?.ToString() ?? string.Empty;
                             }
                             object testDataValue = testData.Value.GetValue(memberType);
-                            Trace.TraceInformation($"CompareMembers {testData.Key}: {memberValue} == {testDataValue}");
+                            Trace.TraceInformation($"CompareMembers {testData.Key}: '{memberValue}' == '{testDataValue}'");
                             Assert.Equal(memberValue, testDataValue);
                         }
                     }