[NFC] two small perf fixes for when using a DebugSymbols DBGShellCommands to find a dSYM

[jasonmolenda]

Summary

SymbolVendorMacOSX::CreateInstance() calls LocateMacOSXFilesUsingDebugSymbols(), to the DebugSymbols framework on macOS, to find a dSYM on the local computer or from a remote server. This method constructs a Module, and it finds a dSYM, creates an ObjectFile for it, then calls SymbolVendor::AddSymbolFileRepresentation with that ObjectFile. The location of the dSYM hasn't been recorded in the Module anywhere, so AddSymbolFileRepresentation eventually (under many layers) calls LocateMacOSXFilesUsingDebugSymbols() again. So we call into the DebugSymbols framework twice. It's easy to fix; have SymbolVendorMacOSX::CreateInstance() add the dSYM path we found from the first call into LocateMacOSXFilesUsingDebugSymbols() in the Module before we call AddSymbolFileRepresentation().

The second perf fix is to LocateMacOSXFilesUsingDebugSymbols() so when it is handed a ModuleSpec that has a binary file path, and that file exists, one of the keys we get back from the DebugSymbols framework gives us a "symbol rich binary" (possibly unstripped). This isn't adding anything when we have the dSYM - we can use the original (possibly stripped) binary that we had to begin with. The symbol rich binary may even be over an NFS filesystem, introducing a large performance cost to using it over the local stripped binary.

Pretty straightforward stuff, not sure who to ask for review, this is more my area than anyone else I can think of these days. If anyone has comments, please let me know.

rdar://84576917

[JDevlieghere]

lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp

155

lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

152–153

I assume an empty file spec wouldn't exist. Can this be simplified?

163–167

The macro checks if log is NULL.

183–189

195–197

[jasonmolenda]

lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

152–153

I was worried that FileSystem::Exists might return true for an empty string filepath; it looks like this devolves down to a access() call, it's probably fine.

163–167

Ah good catch. I was copy & pasting from nearby code that did the same, I'll fix both.

[jasonmolenda]

Update to address Jonas' comments.

[JDevlieghere]

LGTM

lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp

if (dsym_fspec) {
       dsym_root = pos != std::string::npos ? dsym_root.substr(0, pos) : "";
 
       DataBufferSP dsym_file_data_sp;
       lldb::offset_t dsym_file_data_offset = 0;
       dsym_objfile_sp =
           ObjectFile::FindPlugin(module_sp, &dsym_fspec, 0,
                                  FileSystem::Instance().GetByteSize(dsym_fspec),
                                  dsym_file_data_sp, dsym_file_data_offset);
+      // Important to save the dSYM FileSpec so we don't call
+      // Symbols::LocateExecutableSymbolFile a second time while trying to
+      // add the symbol ObjectFile to this Module.
+      if (dsym_objfile_sp && !module_sp->GetSymbolFileFileSpec()) {
+        module_sp->SetSymbolFileFileSpec(dsym_fspec);
+      }
       if (UUIDsMatch(module_sp.get(), dsym_objfile_sp.get(), feedback_strm)) {
         // We need a XML parser if we hope to parse a plist...
         if (XMLDocument::XMLEnabled()) {
           if (module_sp->GetSourceMappingList().IsEmpty()) {
             lldb_private::UUID dsym_uuid = dsym_objfile_sp->GetUUID();
             if (dsym_uuid) {
               std::string uuid_str = dsym_uuid.GetAsString();
               if (!uuid_str.empty() && !dsym_root.empty()) {

lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

 #include <pwd.h>
 
 #include <CoreFoundation/CoreFoundation.h>
 
 #include "Host/macosx/cfcpp/CFCBundle.h"
 #include "Host/macosx/cfcpp/CFCData.h"
 #include "Host/macosx/cfcpp/CFCReleaser.h"
 #include "Host/macosx/cfcpp/CFCString.h"
+#include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/Host.h"
+#include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
if (module_uuid.size() == 16) {
 
         CFCReleaser<CFURLRef> dsym_url(g_dlsym_DBGCopyFullDSYMURLForUUID(
             module_uuid_ref.get(), exec_url.get()));
         char path[PATH_MAX];
 
         if (dsym_url.get()) {
           if (::CFURLGetFileSystemRepresentation(
                   dsym_url.get(), true, (UInt8 *)path, sizeof(path) - 1)) {
-            if (log) {
-              LLDB_LOGF(log,
-                        "DebugSymbols framework returned dSYM path of %s for "
-                        "UUID %s -- looking for the dSYM",
-                        path, uuid->GetAsString().c_str());
-            }
             FileSpec dsym_filespec(path);
             if (path[0] == '~')
               FileSystem::Instance().Resolve(dsym_filespec);
 
             if (FileSystem::Instance().IsDirectory(dsym_filespec)) {
               dsym_filespec =
                   Symbols::FindSymbolFileInBundle(dsym_filespec, uuid, arch);
               ++items_found;
if (module_uuid.size() == 16) {
           CFCReleaser<CFDictionaryRef> dict(
               g_dlsym_DBGCopyDSYMPropertyLists(dsym_url.get()));
           CFDictionaryRef uuid_dict = NULL;
           if (dict.get()) {
             CFCString uuid_cfstr(uuid->GetAsString().c_str());
             uuid_dict = static_cast<CFDictionaryRef>(
                 ::CFDictionaryGetValue(dict.get(), uuid_cfstr.get()));
           }
-          if (uuid_dict) {
+
+          // Check to see if we have the file on the local filesystem.
+          if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
+            ModuleSpec exe_spec;
+            exe_spec.GetFileSpec() = module_spec.GetFileSpec();
+            exe_spec.GetUUID() = module_spec.GetUUID();
+            ModuleSP module_sp;
+            module_sp.reset(new Module(exe_spec));
+            if (module_sp && module_sp->GetObjectFile() &&
+                module_sp->MatchesModuleSpec(exe_spec)) {
+              success = true;
+              return_module_spec.GetFileSpec() = module_spec.GetFileSpec();
+              LLDB_LOGF(log, "using original binary filepath %s for UUID %s",
+                        module_spec.GetFileSpec().GetPath().c_str(),
+                        uuid->GetAsString().c_str());
+              ++items_found;
+            }
+          }
+
+          // Check if the requested image is in our shared cache.
+          if (!success) {
+            SharedCacheImageInfo image_info = HostInfo::GetSharedCacheImageInfo(
+                module_spec.GetFileSpec().GetPath());
+
+            // If we found it and it has the correct UUID, let's proceed with
+            // creating a module from the memory contents.
+            if (image_info.uuid && (!module_spec.GetUUID() ||
+                                    module_spec.GetUUID() == image_info.uuid)) {
+              success = true;
+              return_module_spec.GetFileSpec() = module_spec.GetFileSpec();
+              LLDB_LOGF(log,
+                        "using binary from shared cache for filepath %s for "
+                        "UUID %s",
+                        module_spec.GetFileSpec().GetPath().c_str(),
+                        uuid->GetAsString().c_str());
+              ++items_found;
+            }
+          }
+
+          // Use the DBGSymbolRichExecutable filepath if present
+          if (!success && uuid_dict) {
             CFStringRef exec_cf_path =
                 static_cast<CFStringRef>(::CFDictionaryGetValue(
                     uuid_dict, CFSTR("DBGSymbolRichExecutable")));
             if (exec_cf_path && ::CFStringGetFileSystemRepresentation(
                                     exec_cf_path, path, sizeof(path))) {
-              if (log) {
-                LLDB_LOGF(log, "plist bundle has exec path of %s for UUID %s",
-                          path, uuid->GetAsString().c_str());
-              }
               ++items_found;
               FileSpec exec_filespec(path);
               if (path[0] == '~')
                 FileSystem::Instance().Resolve(exec_filespec);
               if (FileSystem::Instance().Exists(exec_filespec)) {
                 success = true;
                 return_module_spec.GetFileSpec() = exec_filespec;
               }
             }
           }
 
+          // Look next to the dSYM for the binary file.
           if (!success) {
-            // No dictionary, check near the dSYM bundle for an executable that
-            // matches...
             if (::CFURLGetFileSystemRepresentation(
                     dsym_url.get(), true, (UInt8 *)path, sizeof(path) - 1)) {
               char *dsym_extension_pos = ::strstr(path, ".dSYM");
               if (dsym_extension_pos) {
                 *dsym_extension_pos = '\0';
-                if (log) {
-                  LLDB_LOGF(log,
-                            "Looking for executable binary next to dSYM "
-                            "bundle with name with name %s",
-                            path);
-                }
                 FileSpec file_spec(path);
                 FileSystem::Instance().Resolve(file_spec);
                 ModuleSpecList module_specs;
                 ModuleSpec matched_module_spec;
                 using namespace llvm::sys::fs;
                 switch (get_file_type(file_spec.GetPath())) {
 
                 case file_type::directory_file: // Bundle directory?
if (module_uuid.size() == 16) {
                       if (ObjectFile::GetModuleSpecifications(
                               bundle_exe_file_spec, 0, 0, module_specs) &&
                           module_specs.FindMatchingModuleSpec(
                               module_spec, matched_module_spec))
 
                       {
                         ++items_found;
                         return_module_spec.GetFileSpec() = bundle_exe_file_spec;
-                        if (log) {
-                          LLDB_LOGF(log,
-                                    "Executable binary %s next to dSYM is "
-                                    "compatible; using",
-                                    path);
-                        }
-                      }
-                    }
-                  }
                 } break;
 
                 case file_type::fifo_file:      // Forget pipes
                 case file_type::socket_file:    // We can't process socket files
                 case file_type::file_not_found: // File doesn't exist...
                 case file_type::status_error:
                   break;
 
                 case file_type::type_unknown:
                 case file_type::regular_file:
                 case file_type::symlink_file:
                 case file_type::block_file:
                 case file_type::character_file:
                   if (ObjectFile::GetModuleSpecifications(file_spec, 0, 0,
                                                           module_specs) &&
                       module_specs.FindMatchingModuleSpec(module_spec,
                                                           matched_module_spec))
 
                   {
                     ++items_found;
                     return_module_spec.GetFileSpec() = file_spec;
-                    if (log) {
-                      LLDB_LOGF(log,
-                                "Executable binary %s next to dSYM is "
-                                "compatible; using",
-                                path);
-                    }
-                  }
                   break;
                 }
               }
             }
           }
         }
       }
     }
if (uuid_dict != NULL && CFGetTypeID(uuid_dict) == CFDictionaryGetTypeID()) {
     }
 
     cf_str = (CFStringRef)CFDictionaryGetValue(
         (CFDictionaryRef)uuid_dict, CFSTR("DBGSymbolRichExecutable"));
     if (cf_str && CFGetTypeID(cf_str) == CFStringGetTypeID()) {
       if (CFCString::FileSystemRepresentation(cf_str, str)) {
         module_spec.GetFileSpec().SetFile(str.c_str(), FileSpec::Style::native);
         FileSystem::Instance().Resolve(module_spec.GetFileSpec());
-        if (log) {
-          LLDB_LOGF(log,
-                    "From dsymForUUID plist: Symbol rich executable is at '%s'",
-                    str.c_str());
-        }
-      }
-    }
 
     cf_str = (CFStringRef)CFDictionaryGetValue((CFDictionaryRef)uuid_dict,
                                                CFSTR("DBGDSYMPath"));
     if (cf_str && CFGetTypeID(cf_str) == CFStringGetTypeID()) {
       if (CFCString::FileSystemRepresentation(cf_str, str)) {
         module_spec.GetSymbolFileSpec().SetFile(str.c_str(),
                                                 FileSpec::Style::native);
         FileSystem::Instance().Resolve(module_spec.GetFileSpec());
         success = true;
-        if (log) {
-          LLDB_LOGF(log, "From dsymForUUID plist: dSYM is at '%s'",
-                    str.c_str());
-        }
+        LLDB_LOGF(log, "From dsymForUUID plist: dSYM is at '%s'", str.c_str());
       }
     }
 
     std::string DBGBuildSourcePath;
     std::string DBGSourcePath;
 
     // If DBGVersion 1 or DBGVersion missing, ignore DBGSourcePathRemapping.
     // If DBGVersion 2, strip last two components of path remappings from
if (g_dsym_for_uuid_exe_exists) {
                       }
                     }
                   }
                 }
               }
             }
           }
         } else {
-          if (log) {
-            if (!uuid_str.empty())
-              LLDB_LOGF(log, "Called %s on %s, no matches",
-                        g_dsym_for_uuid_exe_path, uuid_str.c_str());
-            else if (file_path[0] != '\0')
-              LLDB_LOGF(log, "Called %s on %s, no matches",
-                        g_dsym_for_uuid_exe_path, file_path);
-          }
-        }
-      }
-    }
-  }
   return success;
 }