[Lldb-commits] [PATCH] D126435: NFC delay tilde expansion on source path remappings until we are opening a source file

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added reviewers: JDevlieghere, jingham.
jasonmolenda added a project: LLDB.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

Internal to Apple, we have dSYMs that include source path remappings from the build-system paths to the long-term storage, and the latter point to NFS mounted directories, using a tilde username component.  Currently, lldb expands those tilde paths to absolute paths when it is parsing the dSYMs, but this requires a stat of the full NFS filepath to the source directory -- and over a slow network, that delay can be significant.  These initial realpath & stat's would all happen at initial file load / process launch, which is already a huge perf bottleneck.  It's quite easy to move them out of there, and over a slow network connection, can yield a large time savings.

This patch keeps the source path remapping names in terms of tilde-username until we are actually opening one of the source files, at which point we realpath and test for the path existence.  We'll be opening the source file anyway in a second, so walking the inodes would be done for that - there's no additional perf penalty from doing the expansion-and-check here.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126435

Files:
  lldb/source/Core/SourceManager.cpp
  lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp


Index: lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
===================================================================
--- lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -237,13 +237,6 @@
                                   !original_DBGSourcePath_value.empty()) {
                                 DBGSourcePath = original_DBGSourcePath_value;
                               }
-                              if (DBGSourcePath[0] == '~') {
-                                FileSpec resolved_source_path(
-                                    DBGSourcePath.c_str());
-                                FileSystem::Instance().Resolve(
-                                    resolved_source_path);
-                                DBGSourcePath = resolved_source_path.GetPath();
-                              }
                               module_sp->GetSourceMappingList().Append(
                                   key.GetStringRef(), DBGSourcePath, true);
                               // With version 2 of DBGSourcePathRemapping, we
@@ -275,11 +268,6 @@
                                            DBGBuildSourcePath);
                     plist.GetValueAsString("DBGSourcePath", DBGSourcePath);
                     if (!DBGBuildSourcePath.empty() && !DBGSourcePath.empty()) {
-                      if (DBGSourcePath[0] == '~') {
-                        FileSpec resolved_source_path(DBGSourcePath.c_str());
-                        FileSystem::Instance().Resolve(resolved_source_path);
-                        DBGSourcePath = resolved_source_path.GetPath();
-                      }
                       module_sp->GetSourceMappingList().Append(
                           DBGBuildSourcePath, DBGSourcePath, true);
                     }
Index: lldb/source/Core/SourceManager.cpp
===================================================================
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -66,10 +66,16 @@
   if (!file_spec)
     return nullptr;
 
+  FileSpec resolved_fspec = file_spec;
+  if (!FileSystem::Instance().Exists(file_spec) &&
+      file_spec.GetDirectory().GetCString()[0] == '~') {
+    FileSystem::Instance().Resolve(resolved_fspec);
+  }
+
   DebuggerSP debugger_sp(m_debugger_wp.lock());
   FileSP file_sp;
   if (debugger_sp && debugger_sp->GetUseSourceCache())
-    file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
+    file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(resolved_fspec);
 
   TargetSP target_sp(m_target_wp.lock());
 
@@ -87,9 +93,9 @@
   // If file_sp is no good or it points to a non-existent file, reset it.
   if (!file_sp || !FileSystem::Instance().Exists(file_sp->GetFileSpec())) {
     if (target_sp)
-      file_sp = std::make_shared<File>(file_spec, target_sp.get());
+      file_sp = std::make_shared<File>(resolved_fspec, target_sp.get());
     else
-      file_sp = std::make_shared<File>(file_spec, debugger_sp);
+      file_sp = std::make_shared<File>(resolved_fspec, debugger_sp);
 
     if (debugger_sp && debugger_sp->GetUseSourceCache())
       debugger_sp->GetSourceFileCache().AddSourceFile(file_sp);
@@ -394,6 +400,7 @@
       m_mod_time(FileSystem::Instance().GetModificationTime(file_spec)),
       m_debugger_wp(target ? target->GetDebugger().shared_from_this()
                            : DebuggerSP()) {
+//  m_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
   CommonInitializer(file_spec, target);
 }
 
@@ -441,6 +448,11 @@
           }
         }
       }
+      // Try tilde expansion if the file does not exist
+      if (!FileSystem::Instance().Exists(m_file_spec) &&
+          m_file_spec.GetDirectory().GetCString()[0] == '~') {
+          FileSystem::Instance().Resolve(m_file_spec);
+      }
       // Try remapping if m_file_spec does not correspond to an existing file.
       if (!FileSystem::Instance().Exists(m_file_spec)) {
         // Check target specific source remappings (i.e., the


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D126435.432152.patch
Type: text/x-patch
Size: 4070 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220525/5736deb4/attachment.bin>