SymbolVendor: Introduce Module::GetSymbolFile

[labath]

Summary

This is the next step in avoiding funneling all SymbolFile calls through
the SymbolVendor. Right now, it is just a convenience function, but it
allows us to update all calls to SymbolVendor functions to access the
SymbolFile directly. Once all call sites have been updated, we can
remove the GetSymbolVendor member function.

This patch just updates the calls to GetSymbolVendor, which were calling
it just so they could fetch the underlying symbol file. Other calls will
be done in follow-ups.

[clayborg]

Just add a few null checks and this will be good to go.

source/Commands/CommandObjectTarget.cpp

2240

check m->GetSymbolFile() for NULL

2265

check m->GetSymbolFile() for NULL

[clayborg]

Alternatively we can switch to using a reference for:

SymbolFile &Module::GetSymbolFile(bool can_create Stream *feedback_strm);

As I believe we always fall back to SymbolFileSymtab don't we?

[labath]

In D65435#1608276, @clayborg wrote:

Alternatively we can switch to using a reference for:

SymbolFile &Module::GetSymbolFile(bool can_create Stream *feedback_strm);

As I believe we always fall back to SymbolFileSymtab don't we?

That is mostly true, but not 100% true in all circumstances. If we manage to create an object file for the module, then yes, I believe we should also always be able to construct a symbol file. However, we currently can run into situations where we fail to create an object file for a module (because the constructor does not check that). Instead what people usually do is that after constructing a module object, they call GetObjectFile() to check whether the module is really valid.

One of my ideas for the future is to change how modules are constructed, so that once we get a module object, we can assume that it will always contain a valid ObjectFile. At that point we would be able to assume the existence of SymbolFile. Until then, I can add the null checks you requested, though I don't think they can be really triggered now as those objectfile-less modules should not end up in the module list of a target.

[labath]

• add two null checks
• replace GetSymbolVendor with GetSymbolFile in a couple of more places, where it is easy to do

include/lldb/Core/Module.h

public:
   ///     plug-in can be found that can use the object file, nullptr will
   ///     be returned, else a valid symbol vendor plug-in interface
   ///     will be returned. The returned pointer is owned by this
   ///     object and remains valid as long as the object is around.
   virtual SymbolVendor *
   GetSymbolVendor(bool can_create = true,
                   lldb_private::Stream *feedback_strm = nullptr);
 
+  SymbolFile *GetSymbolFile(bool can_create = true,
+                            Stream *feedback_strm = nullptr);
+
   /// Get a reference to the UUID value contained in this object.
   ///
   /// If the executable image file doesn't not have a UUID value built into
   /// the file format, an MD5 checksum of the entire file, or slice of the
   /// file for the current architecture should be used.
   ///
   /// \return
   ///     A const pointer to the internal copy of the UUID value in

source/Commands/CommandObjectTarget.cpp

if (command.GetArgumentCount() == 0) {
       // Dump all ASTs for all modules images
       result.GetOutputStream().Printf("Dumping clang ast for %" PRIu64
                                       " modules.\n",
                                       (uint64_t)num_modules);
       for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
         if (m_interpreter.WasInterrupted())
           break;
         Module *m = target->GetImages().GetModulePointerAtIndex(image_idx);
-        SymbolFile *sf = m->GetSymbolVendor()->GetSymbolFile();
-        sf->DumpClangAST(result.GetOutputStream());
+        m->GetSymbolFile()->DumpClangAST(result.GetOutputStream());
       }
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     }
 
     // Dump specified ASTs (by basename or fullpath)
     for (const Args::ArgEntry &arg : command.entries()) {
       ModuleList module_list;
       const size_t num_matches =
           FindModulesByName(target, arg.c_str(), module_list, true);
       if (num_matches == 0) {
         // Check the global list
         std::lock_guard<std::recursive_mutex> guard(
             Module::GetAllocationModuleCollectionMutex());
 
         result.AppendWarningWithFormat(
             "Unable to find an image that matches '%s'.\n", arg.c_str());
         continue;
       }
 
       for (size_t i = 0; i < num_matches; ++i) {
         if (m_interpreter.WasInterrupted())
           break;
         Module *m = module_list.GetModulePointerAtIndex(i);
-        SymbolFile *sf = m->GetSymbolVendor()->GetSymbolFile();
-        sf->DumpClangAST(result.GetOutputStream());
+        m->GetSymbolFile()->DumpClangAST(result.GetOutputStream());
       }
     }
     result.SetStatus(eReturnStatusSuccessFinishResult);
     return true;
   }
 };
 
 #pragma mark CommandObjectTargetModulesDumpSymfile
for (size_t i = 0; i < num_entries; ++i) {
         if (width)
           strm.Printf("{%*" PRIu64 "}", width, (uint64_t)ref_count);
         else
           strm.Printf("{%" PRIu64 "}", (uint64_t)ref_count);
       } break;
 
       case 's':
       case 'S': {
-        const SymbolVendor *symbol_vendor = module->GetSymbolVendor();
-        if (symbol_vendor) {
-          const FileSpec symfile_spec = symbol_vendor->GetMainFileSpec();
+        if (const SymbolFile *symbol_file = module->GetSymbolFile()) {
+          const FileSpec symfile_spec =
+              symbol_file->GetObjectFile()->GetFileSpec();
           if (format_char == 'S') {
             // Dump symbol file only if different from module file
             if (!symfile_spec || symfile_spec == module->GetFileSpec()) {
               print_space = false;
               break;
             }
             // Add a newline and indent past the index
             strm.Printf("\n%*s", indent, "");
if (symbol_fspec) {
       } else if (num_matches == 1) {
         ModuleSP module_sp(matching_module_list.GetModuleAtIndex(0));
 
         // The module has not yet created its symbol vendor, we can just give
         // the existing target module the symfile path to use for when it
         // decides to create it!
         module_sp->SetSymbolFileFileSpec(symbol_fspec);
 
-        SymbolVendor *symbol_vendor =
-            module_sp->GetSymbolVendor(true, &result.GetErrorStream());
-        if (symbol_vendor) {
-          SymbolFile *symbol_file = symbol_vendor->GetSymbolFile();
-
-          if (symbol_file) {
-            ObjectFile *object_file = symbol_file->GetObjectFile();
+        SymbolFile *symbol_file =
+            module_sp->GetSymbolFile(true, &result.GetErrorStream());
 
-            if (object_file && object_file->GetFileSpec() == symbol_fspec) {
-              // Provide feedback that the symfile has been successfully added.
-              const FileSpec &module_fs = module_sp->GetFileSpec();
-              result.AppendMessageWithFormat(
-                  "symbol file '%s' has been added to '%s'\n", symfile_path,
-                  module_fs.GetPath().c_str());
 
-              // Let clients know something changed in the module if it is
-              // currently loaded
-              ModuleList module_list;
-              module_list.Append(module_sp);
-              target->SymbolsDidLoad(module_list);
 
-              // Make sure we load any scripting resources that may be embedded
-              // in the debug info files in case the platform supports that.
-              Status error;
-              StreamString feedback_stream;
-              module_sp->LoadScriptingResourceInTarget(target, error,
-                                                       &feedback_stream);
-              if (error.Fail() && error.AsCString())
-                result.AppendWarningWithFormat(
-                    "unable to load scripting data for module %s - error "
-                    "reported was %s",
-                    module_sp->GetFileSpec()
-                        .GetFileNameStrippingExtension()
-                        .GetCString(),
-                    error.AsCString());
-              else if (feedback_stream.GetSize())
-                result.AppendWarningWithFormat("%s", feedback_stream.GetData());
 
-              flush = true;
-              result.SetStatus(eReturnStatusSuccessFinishResult);
-              return true;
-            }
-          }
-        }
         // Clear the symbol file spec if anything went wrong
         module_sp->SetSymbolFileFileSpec(FileSpec());
       }
 
       namespace fs = llvm::sys::fs;
       if (module_spec.GetUUID().IsValid()) {
         StreamString ss_symfile_uuid;
         module_spec.GetUUID().Dump(&ss_symfile_uuid);

source/Core/Module.cpp

uint32_t Module::ResolveSymbolContextForAddress(
 
   // Make sure the section matches this module before we try and match anything
   if (section_sp && section_sp->GetModule().get() == this) {
     // If the section offset based address resolved itself, then this is the
     // right module.
     sc.module_sp = shared_from_this();
     resolved_flags |= eSymbolContextModule;
 
-    SymbolVendor *sym_vendor = GetSymbolVendor();
-    if (!sym_vendor)
+    SymbolFile *symfile = GetSymbolFile();
+    if (!symfile)
       return resolved_flags;
 
     // Resolve the compile unit, function, block, line table or line entry if
     // requested.
     if (resolve_scope & eSymbolContextCompUnit ||
         resolve_scope & eSymbolContextFunction ||
         resolve_scope & eSymbolContextBlock ||
         resolve_scope & eSymbolContextLineEntry ||
         resolve_scope & eSymbolContextVariable) {
       resolved_flags |=
-          sym_vendor->ResolveSymbolContext(so_addr, resolve_scope, sc);
+          symfile->ResolveSymbolContext(so_addr, resolve_scope, sc);
     }
 
     // Resolve the symbol if requested, but don't re-look it up if we've
     // already found it.
     if (resolve_scope & eSymbolContextSymbol &&
         !(resolved_flags & eSymbolContextSymbol)) {
-      Symtab *symtab = sym_vendor->GetSymtab();
+      Symtab *symtab = symfile->GetSymtab();
       if (symtab && so_addr.IsSectionOffset()) {
         Symbol *matching_symbol = nullptr;
 
         symtab->ForEachSymbolContainingFileAddress(
             so_addr.GetFileAddress(),
             [&matching_symbol](Symbol *symbol) -> bool {
               if (symbol->GetType() != eSymbolTypeInvalid) {
                 matching_symbol = symbol;
if (resolve_scope & eSymbolContextSymbol &&
           if (sc.symbol->IsSynthetic()) {
             // We have a synthetic symbol so lets check if the object file from
             // the symbol file in the symbol vendor is different than the
             // object file for the module, and if so search its symbol table to
             // see if we can come up with a better symbol. For example dSYM
             // files on MacOSX have an unstripped symbol table inside of them.
             ObjectFile *symtab_objfile = symtab->GetObjectFile();
             if (symtab_objfile && symtab_objfile->IsStripped()) {
-              SymbolFile *symfile = sym_vendor->GetSymbolFile();
-              if (symfile) {
-                ObjectFile *symfile_objfile = symfile->GetObjectFile();
-                if (symfile_objfile != symtab_objfile) {
-                  Symtab *symfile_symtab = symfile_objfile->GetSymtab();
-                  if (symfile_symtab) {
-                    Symbol *symbol =
-                        symfile_symtab->FindSymbolContainingFileAddress(
-                            so_addr.GetFileAddress());
-                    if (symbol && !symbol->IsSynthetic()) {
-                      sc.symbol = symbol;
-                    }
-                  }
-                }
-              }
-            }
-          }
           resolved_flags |= eSymbolContextSymbol;
         }
       }
     }
 
     // For function symbols, so_addr may be off by one.  This is a convention
     // consistent with FDE row indices in eh_frame sections, but requires extra
     // logic here to permit symbol lookup for disassembly and unwind.
if (!m_did_load_symbol_vendor.load() && can_create) {
             SymbolVendor::FindPlugin(shared_from_this(), feedback_strm));
         m_did_load_symbol_vendor = true;
       }
     }
   }
   return m_symfile_up.get();
 }
 
+SymbolFile *Module::GetSymbolFile(bool can_create, Stream *feedback_strm) {
+  if (SymbolVendor *vendor = GetSymbolVendor(can_create, feedback_strm))
+    return vendor->GetSymbolFile();
+  return nullptr;
+}
+
 void Module::SetFileSpecAndObjectName(const FileSpec &file,
                                       ConstString object_name) {
   // Container objects whose paths do not specify a file directly can call this
   // function to correct the file and object names.
   m_file = file;
   m_mod_time = FileSystem::Instance().GetModificationTime(file);
   m_object_name = object_name;
 }
if (symtab) {
       SymbolIndicesToSymbolContextList(symtab, symbol_indexes, sc_list);
     }
   }
   return sc_list.GetSize() - initial_size;
 }
 
 void Module::PreloadSymbols() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  SymbolVendor * sym_vendor = GetSymbolVendor();
-  if (!sym_vendor) {
+  SymbolFile *sym_file = GetSymbolFile();
+  if (!sym_file)
     return;
-  }
+
   // Prime the symbol file first, since it adds symbols to the symbol table.
-  if (SymbolFile *symbol_file = sym_vendor->GetSymbolFile()) {
-    symbol_file->PreloadSymbols();
-  }
+  sym_file->PreloadSymbols();
+
   // Now we can prime the symbol table.
-  if (Symtab * symtab = sym_vendor->GetSymtab()) {
+  if (Symtab *symtab = sym_file->GetSymtab())
     symtab->PreloadSymbols();
-  }
-}
 
 void Module::SetSymbolFileFileSpec(const FileSpec &file) {
   if (!FileSystem::Instance().Exists(file))
     return;
   if (m_symfile_up) {
     // Remove any sections in the unified section list that come from the
     // current symbol vendor.
     SectionList *section_list = GetSectionList();
-    SymbolFile *symbol_file = m_symfile_up->GetSymbolFile();
+    SymbolFile *symbol_file = GetSymbolFile();
     if (section_list && symbol_file) {
       ObjectFile *obj_file = symbol_file->GetObjectFile();
       // Make sure we have an object file and that the symbol vendor's objfile
       // isn't the same as the module's objfile before we remove any sections
       // for it...
       if (obj_file) {
         // Check to make sure we aren't trying to specify the file we already
         // have

source/Expression/IRExecutionUnit.cpp

FindBestAlternateMangledName(ConstString demangled,
   std::string scope_qualified_name = cpp_name.GetScopeQualifiedName();
 
   if (!scope_qualified_name.size())
     return ConstString();
 
   if (!sym_ctx.module_sp)
     return ConstString();
 
-  SymbolVendor *sym_vendor = sym_ctx.module_sp->GetSymbolVendor();
-  if (!sym_vendor)
-    return ConstString();
-
-  lldb_private::SymbolFile *sym_file = sym_vendor->GetSymbolFile();
+  lldb_private::SymbolFile *sym_file = sym_ctx.module_sp->GetSymbolFile();
   if (!sym_file)
     return ConstString();
 
   std::vector<ConstString> alternates;
   sym_file->GetMangledNamesForFunction(scope_qualified_name, alternates);
 
   std::vector<ConstString> param_and_qual_matches;
   std::vector<ConstString> param_matches;

source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

if (target &&
     // should not lose ".file" but GetFileNameStrippingExtension() will do
     // precisely that. Ideally, we should have a per-platform list of
     // extensions (".exe", ".app", ".dSYM", ".framework") which should be
     // stripped while leaving "this.binary.file" as-is.
 
     FileSpec module_spec = module.GetFileSpec();
 
     if (module_spec) {
-      SymbolVendor *symbols = module.GetSymbolVendor();
-      if (symbols) {
-        SymbolFile *symfile = symbols->GetSymbolFile();
-        if (symfile) {
-          ObjectFile *objfile = symfile->GetObjectFile();
-          if (objfile) {
-            FileSpec symfile_spec(objfile->GetFileSpec());
-            if (symfile_spec && 
-                strcasestr (symfile_spec.GetPath().c_str(), 
+      if (SymbolFile *symfile = module.GetSymbolFile()) {
+          if (symfile_spec &&
+              strcasestr(symfile_spec.GetPath().c_str(),
                          ".dSYM/Contents/Resources/DWARF") != nullptr &&
-                FileSystem::Instance().Exists(symfile_spec)) {
-              while (module_spec.GetFilename()) {
-                std::string module_basename(
-                    module_spec.GetFilename().GetCString());
-                std::string original_module_basename(module_basename);
 
-                bool was_keyword = false;
 
-                // FIXME: for Python, we cannot allow certain characters in
-                // module
-                // filenames we import. Theoretically, different scripting
-                // languages may have different sets of forbidden tokens in
-                // filenames, and that should be dealt with by each
-                // ScriptInterpreter. For now, we just replace dots with
-                // underscores, but if we ever support anything other than
-                // Python we will need to rework this
-                std::replace(module_basename.begin(), module_basename.end(),
-                             '.', '_');
-                std::replace(module_basename.begin(), module_basename.end(),
-                             ' ', '_');
-                std::replace(module_basename.begin(), module_basename.end(),
-                             '-', '_');
-                ScriptInterpreter *script_interpreter =
-                    target->GetDebugger().GetScriptInterpreter();
-                if (script_interpreter &&
-                    script_interpreter->IsReservedWord(
-                        module_basename.c_str())) {
-                  module_basename.insert(module_basename.begin(), '_');
-                  was_keyword = true;
-                }
+              std::replace(module_basename.begin(), module_basename.end(), '.',
+                           '_');
+              std::replace(module_basename.begin(), module_basename.end(), ' ',
+                           '_');
+              std::replace(module_basename.begin(), module_basename.end(), '-',
+                           '_');
+                  script_interpreter->IsReservedWord(module_basename.c_str())) {
 
-                StreamString path_string;
-                StreamString original_path_string;
-                // for OSX we are going to be in
-                // .dSYM/Contents/Resources/DWARF/<basename> let us go to
-                // .dSYM/Contents/Resources/Python/<basename>.py and see if the
-                // file exists
-                path_string.Printf("%s/../Python/%s.py",
-                                   symfile_spec.GetDirectory().GetCString(),
-                                   module_basename.c_str());
-                original_path_string.Printf(
-                    "%s/../Python/%s.py",
-                    symfile_spec.GetDirectory().GetCString(),
-                    original_module_basename.c_str());
-                FileSpec script_fspec(path_string.GetString());
-                FileSystem::Instance().Resolve(script_fspec);
-                FileSpec orig_script_fspec(original_path_string.GetString());
-                FileSystem::Instance().Resolve(orig_script_fspec);
 
-                // if we did some replacements of reserved characters, and a
-                // file with the untampered name exists, then warn the user
-                // that the file as-is shall not be loaded
-                if (feedback_stream) {
-                  if (module_basename != original_module_basename &&
-                      FileSystem::Instance().Exists(orig_script_fspec)) {
-                    const char *reason_for_complaint =
-                        was_keyword ? "conflicts with a keyword"
-                                    : "contains reserved characters";
-                    if (FileSystem::Instance().Exists(script_fspec))
-                      feedback_stream->Printf(
-                          "warning: the symbol file '%s' contains a debug "
-                          "script. However, its name"
-                          " '%s' %s and as such cannot be loaded. LLDB will"
-                          " load '%s' instead. Consider removing the file with "
-                          "the malformed name to"
-                          " eliminate this warning.\n",
-                          symfile_spec.GetPath().c_str(),
-                          original_path_string.GetData(), reason_for_complaint,
-                          path_string.GetData());
-                    else
-                      feedback_stream->Printf(
-                          "warning: the symbol file '%s' contains a debug "
-                          "script. However, its name"
-                          " %s and as such cannot be loaded. If you intend"
-                          " to have this script loaded, please rename '%s' to "
-                          "'%s' and retry.\n",
-                          symfile_spec.GetPath().c_str(), reason_for_complaint,
-                          original_path_string.GetData(),
-                          path_string.GetData());
-                  }
-                }
+                        original_path_string.GetData(), path_string.GetData());
 
-                if (FileSystem::Instance().Exists(script_fspec)) {
-                  file_list.Append(script_fspec);
-                  break;
-                }
 
-                // If we didn't find the python file, then keep stripping the
-                // extensions and try again
-                ConstString filename_no_extension(
-                    module_spec.GetFileNameStrippingExtension());
-                if (module_spec.GetFilename() == filename_no_extension)
-                  break;
 
-                module_spec.GetFilename() = filename_no_extension;
-              }
-            }
-          }
-        }
-      }
-    }
-  }
   return file_list;
 }
 
 Status PlatformDarwin::ResolveSymbolFile(Target &target,
                                          const ModuleSpec &sym_spec,
                                          FileSpec &sym_file) {
   sym_file = sym_spec.GetSymbolFileSpec();
   if (FileSystem::Instance().IsDirectory(sym_file)) {

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

if (!clang)
     return;
   clang->Dump(s);
 }
 
 SymbolFileDWARFDebugMap *SymbolFileDWARF::GetDebugMapSymfile() {
   if (m_debug_map_symfile == nullptr && !m_debug_map_module_wp.expired()) {
     lldb::ModuleSP module_sp(m_debug_map_module_wp.lock());
     if (module_sp) {
-      SymbolVendor *sym_vendor = module_sp->GetSymbolVendor();
-      if (sym_vendor)
-        m_debug_map_symfile =
-            (SymbolFileDWARFDebugMap *)sym_vendor->GetSymbolFile();
+          (SymbolFileDWARFDebugMap *)module_sp->GetSymbolFile();
     }
   }
   return m_debug_map_symfile;
 }
 
 DWARFExpression::LocationListFormat
 SymbolFileDWARF::GetLocationListFormat() const {
   if (m_data_debug_loclists.m_data.GetByteSize() > 0)

source/Symbol/Block.cpp

if (parent_block)
           can_create, get_parent_variables, stop_if_block_is_inlined_function,
           filter, variable_list);
   }
   return num_variables_added;
 }
 
 SymbolFile *Block::GetSymbolFile() {
   if (ModuleSP module_sp = CalculateSymbolContextModule())
-    if (SymbolVendor *sym_vendor = module_sp->GetSymbolVendor())
-      return sym_vendor->GetSymbolFile();
+    return module_sp->GetSymbolFile();
   return nullptr;
 }
 
 CompilerDeclContext Block::GetDeclContext() {
   if (SymbolFile *sym_file = GetSymbolFile())
     return sym_file->GetDeclContextForUID(GetID());
   return CompilerDeclContext();
 }

source/Symbol/Function.cpp

 ConstString Function::GetDisplayName() const {
   return m_mangled.GetDisplayDemangledName(GetLanguage());
 }
 
 CompilerDeclContext Function::GetDeclContext() {
   ModuleSP module_sp = CalculateSymbolContextModule();
 
   if (module_sp) {
-    SymbolVendor *sym_vendor = module_sp->GetSymbolVendor();
-
-    if (sym_vendor) {
-      SymbolFile *sym_file = sym_vendor->GetSymbolFile();
-
-      if (sym_file)
-        return sym_file->GetDeclContextForUID(GetID());
-    }
-  }
+    if (SymbolFile *sym_file = module_sp->GetSymbolFile())
   return CompilerDeclContext();
 }
 
 Type *Function::GetType() {
   if (m_type == nullptr) {
     SymbolContext sc;
 
     CalculateSymbolContext(&sc);
 
     if (!sc.module_sp)
       return nullptr;
 
-    SymbolVendor *sym_vendor = sc.module_sp->GetSymbolVendor();
-
-    if (sym_vendor == nullptr)
-      return nullptr;
-
-    SymbolFile *sym_file = sym_vendor->GetSymbolFile();
+    SymbolFile *sym_file = sc.module_sp->GetSymbolFile();
 
     if (sym_file == nullptr)
       return nullptr;
 
     m_type = sym_file->ResolveTypeUID(m_type_uid);
   }
   return m_type;
 }

source/Symbol/UnwindTable.cpp

CompactUnwindInfo *UnwindTable::GetCompactUnwindInfo() {
   return m_compact_unwind_up.get();
 }
 
 ArmUnwindInfo *UnwindTable::GetArmUnwindInfo() {
   Initialize();
   return m_arm_unwind_up.get();
 }
 
-SymbolFile *UnwindTable::GetSymbolFile() {
-  if (SymbolVendor *vendor = m_module.GetSymbolVendor())
-    return vendor->GetSymbolFile();
-  return nullptr;
-}
+SymbolFile *UnwindTable::GetSymbolFile() { return m_module.GetSymbolFile(); }
 
 ArchSpec UnwindTable::GetArchitecture() { return m_module.GetArchitecture(); }
 
 bool UnwindTable::GetAllowAssemblyEmulationUnwindPlans() {
   if (ObjectFile *object_file = m_module.GetObjectFile())
     return object_file->AllowAssemblyEmulationUnwindPlans();
   return false;
 }