[lldb] Don't process symlinks deep inside DWARFUnit

[labath]

Summary

This code is handling debug info paths starting with /proc/self/cwd,
which is one of the mechanisms people use to obtain "relocatable" debug
info (the idea being that one starts the debugger with an appropriate
cwd and things "just work").

Instead of resolving the symlinks inside DWARFUnit, we can do the same
thing more elegantly by hooking into the existing Module path remapping
code. Since llvm::DWARFUnit does not support any similar functionality,
doing things this way is also a step towards unifying llvm and lldb
dwarf parsers.

[aprantl]

Does the mean these mappings will show up in the settings command?

[labath]

In D71770#1793334, @aprantl wrote:

Does the mean these mappings will show up in the settings command?

No, these won't show up in the target.source-map setting because this is setting the "per-module" mapping list, and not the global target map. I don't thing we actually have a mechanism to display the per-module maps...

[labath]

Use a slightly different approach as the previous one didn't work on macos (the
reason for that is that each .o file gets a separate module -- the patch
populated the path map of the .o modules, but the remapping was being done by
the main module).

In this version I move the remapping code from SymbolFileDWARF into generic
code. I think this makes sense as the /proc/self/cwd trick is not really
specific to DWARF. It can be used e.g. with clang-cl and PDBs too (if
cross-compiling on a platform which supports symbolic links).

A small behavioral change here is that in order for a change in the setting
value to "kick in", one has to change it before the module gets created --
previously one could do it before debug info gets parsed. I don't think this is
particularly important as the only way to get reliable results was to change the
value before creating a target -- debug info parsing happens lazily, and a
pending breakpoing could cause it to get parsed immediately.

[labath]

Re-requesting review, as this is essentially a different patch.

[JDevlieghere]

LGTM

lldb/include/lldb/Core/Module.h

972

Nit: can you move the comment before the variable with ///? ///< rarely makes sense with the 80 col limit and it's already broken in the comment for m_old_symfiles...

[labath]

lldb/include/lldb/Core/Module.h

972

Good point.

[aprantl]

Looks like this broke the ASAN bot?

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-sanitized/783/testReport/junit/lldb-api/functionalities_breakpoint_comp_dir_symlink/TestCompDirSymLink_py/

lldb-api.functionalities/breakpoint/comp_dir_symlink.TestCompDirSymLink.py (from lldb-api):

=================================================================
==83604==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fff77668712 bp 0x7ffee0e29f10 sp 0x7ffee0e29f10 T0)
==83604==The signal is caused by a READ memory access.
==83604==Hint: address points to the zero page.
    #0 0x7fff77668712 in _platform_strlen+0x12 (libsystem_platform.dylib:x86_64+0x1712)
    #1 0x10ee1191b in wrap_readlink+0x5b (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x3991b)
    #2 0x119a61e57 in lldb_private::FileSystem::Readlink(lldb_private::FileSpec const&, lldb_private::FileSpec&) FileSystemPosix.cpp:46
    #3 0x1197f4b0d in lldb_private::ModuleListProperties::UpdateSymlinkMappings() ModuleList.cpp:122
    #4 0x119af5664 in lldb_private::OptionValueFileSpecList::SetValueFromString(llvm::StringRef, lldb_private::VarSetOperationType) OptionValueFileSpecList.cpp
    #5 0x119b01851 in lldb_private::OptionValueProperties::SetSubValue(lldb_private::ExecutionContext const*, lldb_private::VarSetOperationType, llvm::StringRef, llvm::StringRef) OptionValueProperties.cpp:209
    #6 0x119886a80 in lldb_private::Properties::SetPropertyValue(lldb_private::ExecutionContext const*, lldb_private::VarSetOperationType, llvm::StringRef, llvm::StringRef) UserSettingsController.cpp:49
    #7 0x1197234cb in lldb_private::Debugger::SetPropertyValue(lldb_private::ExecutionContext const*, lldb_private::VarSetOperationType, llvm::StringRef, llvm::StringRef) Debugger.cpp:275
    #8 0x11c2e19ef in CommandObjectSettingsSet::DoExecute(llvm::StringRef, lldb_private::CommandReturnObject&) CommandObjectSettings.cpp:222
    #9 0x119aba342 in lldb_private::CommandObjectRaw::Execute(char const*, lldb_private::CommandReturnObject&) CommandObject.cpp:1003
    #10 0x119a94b4a in lldb_private::CommandInterpreter::HandleCommand(char const*, lldb_private::LazyBool, lldb_private::CommandReturnObject&, lldb_private::ExecutionContext*, bool, bool) CommandInterpreter.cpp:1762
    #11 0x118c1b547 in lldb::SBCommandInterpreter::HandleCommand(char const*, lldb::SBExecutionContext&, lldb::SBCommandReturnObject&, bool) SBCommandInterpreter.cpp:281
    #12 0x118c1a83a in lldb::SBCommandInterpreter::HandleCommand(char const*, lldb::SBCommandReturnObject&, bool) SBCommandInterpreter.cpp:259
    #13 0x1193cfa28 in _wrap_SBCommandInterpreter_HandleCommand(_object*, _object*) LLDBWrapPython.cpp:14074

[aprantl]

Looks like a missing nullptr check.

[labath]

Thanks. I believe this should be fixed by 0157a74be. I'll check the bot to confirm.

lldb/include/lldb/Core/Module.h

 //===-- Module.h ------------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_Module_h_
 #define liblldb_Module_h_
 
 #include "lldb/Core/Address.h"
+#include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolContextScope.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/PathMappingList.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/FileSpec.h"
lldb::SymbolVendorUP
       m_symfile_up; ///< A pointer to the symbol vendor for this module.
   std::vector<lldb::SymbolVendorUP>
       m_old_symfiles; ///< If anyone calls Module::SetSymbolFileFileSpec() and
                       ///changes the symbol file,
   ///< we need to keep all old symbol files around in case anyone has type
   ///references to them
   TypeSystemMap m_type_system_map;   ///< A map of any type systems associated
                                      ///with this module
-  PathMappingList m_source_mappings; ///< Module specific source remappings for
-                                     ///when you have debug info for a module
-                                     ///that doesn't match where the sources
-                                     ///currently are
+  /// Module specific source remappings for when you have debug info for a
+  /// module that doesn't match where the sources currently are.
+  PathMappingList m_source_mappings =
+      ModuleList::GetGlobalModuleListProperties().GetSymlinkMappings();
   lldb::SectionListUP m_sections_up; ///< Unified section list for module that
                                      /// is used by the ObjectFile and and
                                      /// ObjectFile instances for the debug info
 
   std::atomic<bool> m_did_load_objfile{false};
   std::atomic<bool> m_did_load_symfile{false};
   std::atomic<bool> m_did_set_uuid{false};
   mutable bool m_file_has_changed : 1,

lldb/include/lldb/Core/ModuleList.h

 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Iterable.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/Support/RWMutex.h"
 
 #include <functional>
 #include <list>
 #include <mutex>
 #include <vector>
 
 #include <stddef.h>
 #include <stdint.h>

 class SymbolContextList;
 class SymbolFile;
 class Target;
 class TypeList;
 class UUID;
 class VariableList;
 
 class ModuleListProperties : public Properties {
+  mutable llvm::sys::RWMutex m_symlink_paths_mutex;
+  PathMappingList m_symlink_paths;
+
+  void UpdateSymlinkMappings();
+
 public:
   ModuleListProperties();
 
   FileSpec GetClangModulesCachePath() const;
   bool SetClangModulesCachePath(llvm::StringRef path);
   bool GetEnableExternalLookup() const;
   bool SetEnableExternalLookup(bool new_value);
+
+  PathMappingList GetSymlinkMappings() const;
 };
 
 /// \class ModuleList ModuleList.h "lldb/Core/ModuleList.h"
 /// A collection class for Module objects.
 ///
 /// Modules in the module collection class are stored as reference counted
 /// shared pointers to Module objects.
 class ModuleList {

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py

 """
 Test breakpoint command with AT_comp_dir set to symbolic link.
 """
 
 
 import os
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 _EXE_NAME = 'CompDirSymLink'  # Must match Makefile
 _SRC_FILE = 'relative.cpp'
-_COMP_DIR_SYM_LINK_PROP = 'plugin.symbol-file.dwarf.comp-dir-symlink-paths'
+_COMP_DIR_SYM_LINK_PROP = 'symbols.debug-info-symlink-paths'
 
 
 class CompDirSymLinkTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line number to break inside main().
         self.line = line_number(
             os.path.join(self.getSourceDir(), "main.cpp"),
             '// Set break point at this line.')
 
     @skipIf(hostoslist=["windows"])
     def test_symlink_paths_set(self):
         pwd_symlink = self.create_src_symlink()
-        self.doBuild(pwd_symlink)
-        self.runCmd(
-            "settings set %s %s" %
-            (_COMP_DIR_SYM_LINK_PROP, pwd_symlink))
+        self.doBuild(pwd_symlink, pwd_symlink)
         src_path = self.getBuildArtifact(_SRC_FILE)
         lldbutil.run_break_set_by_file_and_line(self, src_path, self.line)
 
     @skipIf(hostoslist=no_match(["linux"]))
     def test_symlink_paths_set_procselfcwd(self):
         os.chdir(self.getBuildDir())
         pwd_symlink = '/proc/self/cwd'
-        self.doBuild(pwd_symlink)
-        self.runCmd(
-            "settings set %s %s" %
-            (_COMP_DIR_SYM_LINK_PROP, pwd_symlink))
+        self.doBuild(pwd_symlink, pwd_symlink)
         src_path = self.getBuildArtifact(_SRC_FILE)
         # /proc/self/cwd points to a realpath form of current directory.
         src_path = os.path.realpath(src_path)
         lldbutil.run_break_set_by_file_and_line(self, src_path, self.line)
 
     @skipIf(hostoslist=["windows"])
     def test_symlink_paths_unset(self):
         pwd_symlink = self.create_src_symlink()
-        self.doBuild(pwd_symlink)
-        self.runCmd('settings clear ' + _COMP_DIR_SYM_LINK_PROP)
+        self.doBuild(pwd_symlink, "")
         src_path = self.getBuildArtifact(_SRC_FILE)
         self.assertRaises(
             AssertionError,
             lldbutil.run_break_set_by_file_and_line,
             self,
             src_path,
             self.line)
 
     def create_src_symlink(self):
         pwd_symlink = self.getBuildArtifact('pwd_symlink')
         if os.path.exists(pwd_symlink):
             os.unlink(pwd_symlink)
         os.symlink(self.getBuildDir(), pwd_symlink)
         self.addTearDownHook(lambda: os.remove(pwd_symlink))
         return pwd_symlink
 
-    def doBuild(self, pwd_symlink):
+    def doBuild(self, pwd_symlink, setting_value):
         self.build(None, None, {'PWD': pwd_symlink})
 
+        self.runCmd(
+            "settings set %s '%s'" %
+            (_COMP_DIR_SYM_LINK_PROP, setting_value))
+
         exe = self.getBuildArtifact(_EXE_NAME)
         self.runCmd('file ' + exe, CURRENT_EXECUTABLE_SET)

lldb/source/Core/CoreProperties.td

 include "../../include/lldb/Core/PropertiesBase.td"
 
 let Definition = "modulelist" in {
   def EnableExternalLookup: Property<"enable-external-lookup", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"Control the use of external tools and repositories to locate symbol files. Directories listed in target.debug-file-search-paths and directory of the executable are always checked first for separate debug info files. Then depending on this setting: On macOS, Spotlight would be also used to locate a matching .dSYM bundle based on the UUID of the executable. On NetBSD, directory /usr/libdata/debug would be also searched. On platforms other than NetBSD directory /usr/lib/debug would be also searched.">;
   def ClangModulesCachePath: Property<"clang-modules-cache-path", "FileSpec">,
     Global,
     DefaultStringValue<"">,
     Desc<"The path to the clang modules cache directory (-fmodules-cache-path).">;
+  def SymLinkPaths: Property<"debug-info-symlink-paths", "FileSpecList">,
+    Global,
+    DefaultStringValue<"">,
+    Desc<"Debug info path which should be resolved while parsing, relative to the host filesystem.">;
 }
 
 let Definition = "debugger" in {
   def AutoConfirm: Property<"auto-confirm", "Boolean">,
     Global,
     DefaultFalse,
     Desc<"If true all confirmation prompts will receive their default reply.">;
   def DisassemblyFormat: Property<"disassembly-format", "FormatEntity">,

lldb/source/Core/ModuleList.cpp

 //===-- ModuleList.cpp ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Interpreter/OptionValueFileSpec.h"
+#include "lldb/Interpreter/OptionValueFileSpecList.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Interpreter/Property.h"
 #include "lldb/Symbol/LocateSymbolFile.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/TypeList.h"
 #include "lldb/Symbol/VariableList.h"
 #include "lldb/Utility/ArchSpec.h"

 };
 
 } // namespace
 
 ModuleListProperties::ModuleListProperties() {
   m_collection_sp =
       std::make_shared<OptionValueProperties>(ConstString("symbols"));
   m_collection_sp->Initialize(g_modulelist_properties);
+  m_collection_sp->SetValueChangedCallback(ePropertySymLinkPaths,
+                                           [this] { UpdateSymlinkMappings(); });
 
   llvm::SmallString<128> path;
   clang::driver::Driver::getDefaultModuleCachePath(path);
   SetClangModulesCachePath(path);
 }
 
 bool ModuleListProperties::GetEnableExternalLookup() const {
   const uint32_t idx = ePropertyEnableExternalLookup;
return m_collection_sp
       ->GetCurrentValue();
 }
 
 bool ModuleListProperties::SetClangModulesCachePath(llvm::StringRef path) {
   return m_collection_sp->SetPropertyAtIndexAsString(
       nullptr, ePropertyClangModulesCachePath, path);
 }
 
+void ModuleListProperties::UpdateSymlinkMappings() {
+  FileSpecList list = m_collection_sp
+                          ->GetPropertyAtIndexAsOptionValueFileSpecList(
+                              nullptr, false, ePropertySymLinkPaths)
+                          ->GetCurrentValue();
+  llvm::sys::ScopedWriter lock(m_symlink_paths_mutex);
+  const bool notify = false;
+  m_symlink_paths.Clear(notify);
+  for (FileSpec symlink : list) {
+    FileSpec resolved;
+    Status status = FileSystem::Instance().Readlink(symlink, resolved);
+    if (status.Success())
+      m_symlink_paths.Append(ConstString(symlink.GetPath()),
+                             ConstString(resolved.GetPath()), notify);
+  }
+}
+
+PathMappingList ModuleListProperties::GetSymlinkMappings() const {
+  llvm::sys::ScopedReader lock(m_symlink_paths_mutex);
+  return m_symlink_paths;
+}
+
 ModuleList::ModuleList()
     : m_modules(), m_modules_mutex(), m_notifier(nullptr) {}
 
 ModuleList::ModuleList(const ModuleList &rhs)
     : m_modules(), m_modules_mutex(), m_notifier(nullptr) {
   std::lock_guard<std::recursive_mutex> lhs_guard(m_modules_mutex);
   std::lock_guard<std::recursive_mutex> rhs_guard(rhs.m_modules_mutex);
   m_modules = rhs.m_modules;

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

static void SetupModuleHeaderPaths(CompilerInstance *compiler,
   HeaderSearchOptions &search_opts = compiler->getHeaderSearchOpts();
 
   for (const std::string &dir : include_directories) {
     search_opts.AddPath(dir, frontend::System, false, true);
     LLDB_LOG(log, "Added user include dir: {0}", dir);
   }
 
   llvm::SmallString<128> module_cache;
-  auto props = ModuleList::GetGlobalModuleListProperties();
+  const auto &props = ModuleList::GetGlobalModuleListProperties();
   props.GetClangModulesCachePath().GetPath(module_cache);
   search_opts.ModuleCachePath = module_cache.str();
   LLDB_LOG(log, "Using module cache path: {0}", module_cache.c_str());
 
   search_opts.ResourceDir = GetClangResourceDir().GetPath();
 
   search_opts.ImplicitModuleMaps = true;
 }

lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

target.GetPlatform()->AddClangModuleCompilationOptions(
       &target, compiler_invocation_arguments);
 
   compiler_invocation_arguments.push_back(ModuleImportBufferName);
 
   // Add additional search paths with { "-I", path } or { "-F", path } here.
 
   {
     llvm::SmallString<128> path;
-    auto props = ModuleList::GetGlobalModuleListProperties();
+    const auto &props = ModuleList::GetGlobalModuleListProperties();
     props.GetClangModulesCachePath().GetPath(path);
     std::string module_cache_argument("-fmodules-cache-path=");
     module_cache_argument.append(path.str());
     compiler_invocation_arguments.push_back(module_cache_argument);
   }
 
   FileSpecList module_search_paths = target.GetClangModuleSearchPaths();
 

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

removeHostnameFromPathname(llvm::StringRef path_from_dwarf) {
   // check whether we have a windows path, and so the first character is a
   // drive-letter not a hostname.
   if (host.size() == 1 && llvm::isAlpha(host[0]) && path.startswith("\\"))
     return path_from_dwarf;
 
   return path;
 }
 
-static FileSpec resolveCompDir(const FileSpec &path) {
-  bool is_symlink = SymbolFileDWARF::GetSymlinkPaths().FindFileIndex(
-                        0, path, /*full*/ true) != UINT32_MAX;
-
-  if (!is_symlink)
-    return path;
-
-  namespace fs = llvm::sys::fs;
-  if (fs::get_file_type(path.GetPath(), false) != fs::file_type::symlink_file)
-    return path;
-
-  FileSpec resolved_symlink;
-  const auto error = FileSystem::Instance().Readlink(path, resolved_symlink);
-  if (error.Success())
-    return resolved_symlink;
-
-  return path;
-}
-
 void DWARFUnit::ComputeCompDirAndGuessPathStyle() {
   m_comp_dir = FileSpec();
   const DWARFDebugInfoEntry *die = GetUnitDIEPtrOnly();
   if (!die)
     return;
 
   llvm::StringRef comp_dir = removeHostnameFromPathname(
       die->GetAttributeValueAsString(this, DW_AT_comp_dir, nullptr));
   if (!comp_dir.empty()) {
     FileSpec::Style comp_dir_style =
         FileSpec::GuessPathStyle(comp_dir).getValueOr(FileSpec::Style::native);
-    m_comp_dir = resolveCompDir(FileSpec(comp_dir, comp_dir_style));
+    m_comp_dir = FileSpec(comp_dir, comp_dir_style);
   } else {
     // Try to detect the style based on the DW_AT_name attribute, but just store
     // the detected style in the m_comp_dir field.
     const char *name =
         die->GetAttributeValueAsString(this, DW_AT_name, nullptr);
     m_comp_dir = FileSpec(
         "", FileSpec::GuessPathStyle(name).getValueOr(FileSpec::Style::native));
   }

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

public:
 
   static lldb_private::ConstString GetPluginNameStatic();
 
   static const char *GetPluginDescriptionStatic();
 
   static lldb_private::SymbolFile *
   CreateInstance(lldb::ObjectFileSP objfile_sp);
 
-  static lldb_private::FileSpecList GetSymlinkPaths();
-
   // Constructors and Destructors
 
   SymbolFileDWARF(lldb::ObjectFileSP objfile_sp,
                   lldb_private::SectionList *dwo_section_list);
 
   ~SymbolFileDWARF() override;
 
   uint32_t CalculateAbilities() override;

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

static ConstString GetSettingName() {
     return SymbolFileDWARF::GetPluginNameStatic();
   }
 
   PluginProperties() {
     m_collection_sp = std::make_shared<OptionValueProperties>(GetSettingName());
     m_collection_sp->Initialize(g_symbolfiledwarf_properties);
   }
 
-  FileSpecList GetSymLinkPaths() {
-    const OptionValueFileSpecList *option_value =
-        m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(
-            nullptr, true, ePropertySymLinkPaths);
-    assert(option_value);
-    return option_value->GetCurrentValue();
-  }
-
   bool IgnoreFileIndexes() const {
     return m_collection_sp->GetPropertyAtIndexAsBoolean(
         nullptr, ePropertyIgnoreIndexes, false);
   }
 };
 
 typedef std::shared_ptr<PluginProperties> SymbolFileDWARFPropertiesSP;
 
for (size_t idx = first_file; idx <= number_of_files; ++idx) {
 
     // Unconditionally add an entry, so the indices match up.
     support_files.EmplaceBack(remapped_file, style);
   }
 
   return support_files;
 }
 
-FileSpecList SymbolFileDWARF::GetSymlinkPaths() {
-  return GetGlobalPluginProperties()->GetSymLinkPaths();
-}
-
 void SymbolFileDWARF::Initialize() {
   LogChannelDWARF::Initialize();
   PluginManager::RegisterPlugin(GetPluginNameStatic(),
                                 GetPluginDescriptionStatic(), CreateInstance,
                                 DebuggerInitialize);
 }
 
 void SymbolFileDWARF::DebuggerInitialize(Debugger &debugger) {

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFProperties.td

 include "../../../../include/lldb/Core/PropertiesBase.td"
 
 let Definition = "symbolfiledwarf" in {
-  def SymLinkPaths: Property<"comp-dir-symlink-paths", "FileSpecList">,
-    Global,
-    DefaultStringValue<"">,
-    Desc<"If the DW_AT_comp_dir matches any of these paths the symbolic links will be resolved at DWARF parse time.">;
   def IgnoreIndexes: Property<"ignore-file-indexes", "Boolean">,
     Global,
     DefaultFalse,
     Desc<"Ignore indexes present in the object files and always index DWARF manually.">;
 }