Cache negative symbol lookups (dotnet#5729)

Fixes issue #675, but also just improves the amount of symbol lookups we
do.

Author: leculver

src/SOS/Strike/dbgengservices.cpp

@@ -6,6 +6,7 @@
 #include <stdint.h>
 #include "hostservices.h"
 #include "exts.h"
+#include "symbols.h"
 
 extern IMachine* GetTargetMachine(ULONG processorType);
 
@@ -860,6 +861,9 @@ DbgEngServices::SetCurrentThreadIdFromSystemId(
 void 
 DbgEngServices::InitializeSymbolStoreFromSymPath()
 {
+    // Clear the negative symbol lookup cache so modules are retried with the new path.
+    ClearSymbolLookupCache();
+
     ISymbolService* symbolService = GetSymbolService();
     if (symbolService != nullptr)
     {

src/SOS/Strike/managedcommands.cpp

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 #include "exts.h"
+#include "symbols.h"
 
 // Windows host only managed command stubs
 
@@ -142,6 +143,10 @@ DECLARE_API(ObjSize)
 DECLARE_API(SetSymbolServer)
 {
     INIT_API_EXT();
+
+    // Clear negative symbol lookup cache so modules are retried with the new configuration.
+    ClearSymbolLookupCache();
+
     return ExecuteManagedOnlyCommand("setsymbolserver", args);
 }
 

src/SOS/Strike/symbols.cpp

@@ -31,6 +31,16 @@
 #undef IfFailRet
 #define IfFailRet(EXPR) do { Status = (EXPR); if(FAILED(Status)) { return (Status); } } while (0)
 
+// Tracks module PE addresses for which symbol loading has already failed,
+// so we don't repeatedly hit symbol servers for the same module.
+// Cleared when the symbol path changes (see ClearSymbolLookupCache).
+static std::set<ULONG64> g_failedSymbolLookups;
+
+void ClearSymbolLookupCache()
+{
+    g_failedSymbolLookups.clear();
+}
+
 #ifndef FEATURE_PAL
 HMODULE g_hmoduleSymBinder = nullptr;
 ISymUnmanagedBinder3 *g_pSymBinder = nullptr;
@@ -314,6 +324,14 @@ HRESULT SymbolReader::LoadSymbols(___in IMetaDataImport* pMD, ___in IXCLRDataMod
             ExtOut("LoadSymbols GetClrModuleImages FAILED 0x%08x\n", hr);
             return hr;
         }
+
+        // Skip modules that have already failed symbol loading to avoid repeated
+        // network round-trips to symbol servers (see dotnet/diagnostics#675).
+        if (moduleBase != 0 && g_failedSymbolLookups.find(moduleBase) != g_failedSymbolLookups.end())
+        {
+            return E_FAIL;
+        }
+
         if (GetSymbolService() == nullptr || !HasPortablePDB(moduleBase))
         {
             hr = LoadSymbolsForWindowsPDB(pMD, moduleBase, pModuleName, FALSE);
@@ -328,6 +346,13 @@ HRESULT SymbolReader::LoadSymbols(___in IMetaDataImport* pMD, ___in IXCLRDataMod
 #endif
     }
 
+    // Skip modules that have already failed symbol loading to avoid repeated
+    // network round-trips to symbol servers (see dotnet/diagnostics#675).
+    if (moduleData.LoadedPEAddress != 0 && g_failedSymbolLookups.find(moduleData.LoadedPEAddress) != g_failedSymbolLookups.end())
+    {
+        return E_FAIL;
+    }
+
 #ifndef FEATURE_PAL
     if (GetSymbolService() == nullptr || !HasPortablePDB(moduleData.LoadedPEAddress))
     {
@@ -340,14 +365,21 @@ HRESULT SymbolReader::LoadSymbols(___in IMetaDataImport* pMD, ___in IXCLRDataMod
     }
 #endif // FEATURE_PAL
 
-    return LoadSymbolsForPortablePDB(
+    hr = LoadSymbolsForPortablePDB(
         pModuleName, 
         moduleData.IsInMemory,
         moduleData.IsFileLayout,
         moduleData.LoadedPEAddress,
         moduleData.LoadedPESize, 
         moduleData.InMemoryPdbAddress,
         moduleData.InMemoryPdbSize);
+
+    if (FAILED(hr) && moduleData.LoadedPEAddress != 0)
+    {
+        g_failedSymbolLookups.insert(moduleData.LoadedPEAddress);
+    }
+    
+    return hr;
 }
 
 #ifndef FEATURE_PAL

src/SOS/Strike/symbols.h

@@ -79,3 +79,7 @@ GetLineByOffset(
     __out_ecount(cchFileName) WCHAR* pwszFileName,
     ___in ULONG cchFileName,
     ___in BOOL bAdjustOffsetForLineNumber = FALSE);
+
+// Clears the negative cache for failed symbol lookups, allowing modules
+// to be retried. Call when the symbol path changes.
+void ClearSymbolLookupCache();

src/tests/Microsoft.Diagnostics.DebugServices.UnitTests/SymbolServiceTests.cs

@@ -91,6 +91,35 @@ public void SymbolPathTests()
             symbolService.DisableSymbolStore();
         }
 
+        [Fact]
+        public void OpenSymbolFile_ReturnsNull_ForInvalidPEStream()
+        {
+            SymbolService symbolService = new(this);
+
+            // OpenSymbolFile should return null (not throw) for non-PE data.
+            // The native SymbolReader::LoadSymbols layer caches these failures
+            // to avoid repeated symbol server lookups (dotnet/diagnostics#675),
+            // but that caching is in native C++ and cannot be tested here.
+            byte[] bogusData = new byte[] { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07 };
+            using System.IO.MemoryStream stream = new(bogusData);
+
+            ISymbolFile result = symbolService.OpenSymbolFile("bogus.dll", isFileLayout: false, stream);
+            Assert.Null(result);
+        }
+
+        [Fact]
+        public void OpenSymbolFile_ReturnsNull_ForInvalidPdbStream()
+        {
+            SymbolService symbolService = new(this);
+
+            // A stream that is not a valid portable PDB should return null.
+            byte[] bogusData = new byte[] { 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00 };
+            using System.IO.MemoryStream stream = new(bogusData);
+
+            ISymbolFile result = symbolService.OpenSymbolFile(stream);
+            Assert.Null(result);
+        }
+
         #region IHost
 
         public IServiceEvent OnShutdownEvent { get; } = new ServiceEvent();