labath updated this revision to Diff 135971.
labath added a comment.
Herald added subscribers: JDevlieghere, mgorny.

This version moves the logic for the building of unified section list into the
symbol vendor plugin.

The idea was to keep the construction of section lists of individual object
files inside the ObjectFile object, and then have the SymbolVendor do the
merging. This was easy to achieve for the SymbolVendorELF, and I'm quite happy
about how things look like there.

However, I had a lot of trouble with making this work for the MachO
objectfile+symbolvendor combo, because there the section building is
intertwined with the load command parsing and the building of the unified
sections list. In particular, the parse function seems to be doing some dodgy
things like taking a section from the "unified" list and inserting it into
per-object-file list. So, here I had to leave the building of the unified list
in the ObjectFile class, and the SymbolVendor just orchestrates the functions
to be called in the right order (for this I needed to cast the ObjectFile into
ObjectFileMachO, which seemingly adds a new dependency to the symbol vendor,
but this is not really true, as the vendor was already checking the type of the
object file via the GetPluginName() function). It would be great if someone
with more MachO knowledge could look at the CreateSections function and split
it up into more manageable chunks.

The other sub-optimal design decision I had to make was due to the mutual
recursion of the Module, SymbolVendor and SymbolFile classes: SymbolVendor
creates a SymbolFile class, which expect that the Module's section list is
already constructed, but SymbolVendor is the one constructing the list. This
means that I couldn't just construct the SymbolVendor and have the Module ask
it for the section list, but I had to make the vendor directly update the
section list within the module (before constructing the symbolfile class).

On the plus side, I really like that I was able to remove the
"update_module_section_list" argument from the ObjectFile::GetSectionList
function, which was very dodgy as well, because whether the module's section
list was updated, depended on the value of the argument of the *first* call of
that function.

All in all, I think patch cleans up the unified section list handling, but only
by a small bit. Let me know what you think.


https://reviews.llvm.org/D42955

Files:
  include/lldb/Core/Module.h
  include/lldb/Symbol/ObjectFile.h
  include/lldb/Symbol/SymbolVendor.h
  include/lldb/lldb-private-interfaces.h
  lit/Modules/Inputs/stripped.yaml
  lit/Modules/Inputs/unstripped.yaml
  lit/Modules/lit.local.cfg
  lit/Modules/unified-section-list.test
  lit/lit.cfg
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
  source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwp.cpp
  source/Plugins/SymbolVendor/CMakeLists.txt
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
  source/Plugins/SymbolVendor/MacOSX/CMakeLists.txt
  source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
  source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
  source/Symbol/ObjectFile.cpp
  source/Symbol/SymbolFile.cpp
  source/Symbol/SymbolVendor.cpp
  tools/lldb-test/lldb-test.cpp

Index: tools/lldb-test/lldb-test.cpp
===================================================================
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -72,7 +72,6 @@
 
   for (const auto &File : opts::module::InputFilenames) {
     ModuleSpec Spec{FileSpec(File, false)};
-    Spec.GetSymbolFileSpec().SetFile(File, false);
 
     auto ModulePtr = std::make_shared<lldb_private::Module>(Spec);
     SectionList *Sections = ModulePtr->GetSectionList();
Index: source/Symbol/SymbolVendor.cpp
===================================================================
--- source/Symbol/SymbolVendor.cpp
+++ source/Symbol/SymbolVendor.cpp
@@ -15,6 +15,7 @@
 // Project includes
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Section.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolFile.h"
@@ -31,15 +32,16 @@
 // also allow for finding separate debug information files.
 //----------------------------------------------------------------------
 SymbolVendor *SymbolVendor::FindPlugin(const lldb::ModuleSP &module_sp,
+                                       SectionList &unified_list,
                                        lldb_private::Stream *feedback_strm) {
   std::unique_ptr<SymbolVendor> instance_ap;
   SymbolVendorCreateInstance create_callback;
 
   for (size_t idx = 0;
        (create_callback = PluginManager::GetSymbolVendorCreateCallbackAtIndex(
             idx)) != nullptr;
        ++idx) {
-    instance_ap.reset(create_callback(module_sp, feedback_strm));
+    instance_ap.reset(create_callback(module_sp, unified_list, feedback_strm));
 
     if (instance_ap.get()) {
       return instance_ap.release();
@@ -51,7 +53,9 @@
   if (instance_ap.get()) {
     ObjectFile *objfile = module_sp->GetObjectFile();
     if (objfile)
-      instance_ap->AddSymbolFileRepresentation(objfile->shared_from_this());
+      unified_list = *objfile->GetSectionList();
+
+    instance_ap->AddSymbolFileRepresentation(objfile->shared_from_this());
   }
   return instance_ap.release();
 }
Index: source/Symbol/SymbolFile.cpp
===================================================================
--- source/Symbol/SymbolFile.cpp
+++ source/Symbol/SymbolFile.cpp
@@ -28,21 +28,6 @@
 SymbolFile *SymbolFile::FindPlugin(ObjectFile *obj_file) {
   std::unique_ptr<SymbolFile> best_symfile_ap;
   if (obj_file != nullptr) {
-
-    // We need to test the abilities of this section list. So create what it
-    // would
-    // be with this new obj_file.
-    lldb::ModuleSP module_sp(obj_file->GetModule());
-    if (module_sp) {
-      // Default to the main module section list.
-      ObjectFile *module_obj_file = module_sp->GetObjectFile();
-      if (module_obj_file != obj_file) {
-        // Make sure the main object file's sections are created
-        module_obj_file->GetSectionList();
-        obj_file->CreateSections(*module_sp->GetUnifiedSectionList());
-      }
-    }
-
     // TODO: Load any plug-ins in the appropriate plug-in search paths and
     // iterate over all of them to find the best one for the job.
 
Index: source/Symbol/ObjectFile.cpp
===================================================================
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -604,22 +604,6 @@
   }
 }
 
-SectionList *ObjectFile::GetSectionList(bool update_module_section_list) {
-  if (m_sections_ap.get() == nullptr) {
-    if (update_module_section_list) {
-      ModuleSP module_sp(GetModule());
-      if (module_sp) {
-        std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
-        CreateSections(*module_sp->GetUnifiedSectionList());
-      }
-    } else {
-      SectionList unified_section_list;
-      CreateSections(unified_section_list);
-    }
-  }
-  return m_sections_ap.get();
-}
-
 lldb::SymbolType
 ObjectFile::GetSymbolTypeFromName(llvm::StringRef name,
                                   lldb::SymbolType symbol_type_hint) {
Index: source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
===================================================================
--- source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
+++ source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
@@ -10,6 +10,7 @@
 #ifndef liblldb_SymbolVendorMacOSX_h_
 #define liblldb_SymbolVendorMacOSX_h_
 
+#include "lldb/Core/Section.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/lldb-private.h"
 
@@ -28,6 +29,7 @@
 
   static lldb_private::SymbolVendor *
   CreateInstance(const lldb::ModuleSP &module_sp,
+                 lldb_private::SectionList &unified_list,
                  lldb_private::Stream *feedback_strm);
 
   //------------------------------------------------------------------
@@ -40,11 +42,14 @@
   //------------------------------------------------------------------
   // PluginInterface protocol
   //------------------------------------------------------------------
-  virtual lldb_private::ConstString GetPluginName();
+  lldb_private::ConstString GetPluginName() override;
 
-  virtual uint32_t GetPluginVersion();
+  uint32_t GetPluginVersion() override;
 
 private:
+  void BuildUnifiedSectionList(lldb_private::ObjectFile &symfile,
+                               lldb_private::SectionList &unified_list);
+
   DISALLOW_COPY_AND_ASSIGN(SymbolVendorMacOSX);
 };
 
Index: source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
===================================================================
--- source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -11,10 +11,10 @@
 
 #include <string.h>
 
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
-#include "lldb/Core/Section.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/Symbols.h"
 #include "lldb/Host/XML.h"
@@ -100,6 +100,7 @@
 //----------------------------------------------------------------------
 SymbolVendor *
 SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP &module_sp,
+                                   SectionList &unified_list,
                                    lldb_private::Stream *feedback_strm) {
   if (!module_sp)
     return NULL;
@@ -298,19 +299,51 @@
           }
         }
 
+        symbol_vendor->BuildUnifiedSectionList(*dsym_objfile_sp, unified_list);
         symbol_vendor->AddSymbolFileRepresentation(dsym_objfile_sp);
         return symbol_vendor;
       }
     }
 
     // Just create our symbol vendor using the current objfile as this is either
     // an executable with no dSYM (that we could locate), an executable with
     // a dSYM that has a UUID that doesn't match.
+    symbol_vendor->BuildUnifiedSectionList(*obj_file, unified_list);
     symbol_vendor->AddSymbolFileRepresentation(obj_file->shared_from_this());
   }
   return symbol_vendor;
 }
 
+void SymbolVendorMacOSX::BuildUnifiedSectionList(ObjectFile &symfile,
+                                                 SectionList &unified_list) {
+  ObjectFile *objfile = GetModule()->GetObjectFile();
+
+  // Remove any sections which do not belong the the Module's ObjectFile (i.e.,
+  // sections that were placed there by the Module's previous Symbol Vendor).
+  // TODO: Once ObjectFileMachO can create it's own sections without
+  // referencing the unified section list, the next two blocks can be replaced
+  // by:
+  // unified_list = *objfile->GetSections();
+  size_t num_sections = unified_list.GetNumSections(0);
+  for (size_t idx = num_sections; idx > 0; --idx) {
+    lldb::SectionSP section_sp(unified_list.GetSectionAtIndex(idx - 1));
+    if (section_sp->GetObjectFile() != objfile)
+      unified_list.DeleteSection(idx - 1);
+  }
+
+  // If there are no sections in the unified list, it means the main object
+  // file has not had a chance to create them yet. Do it now.
+  if (unified_list.GetNumSections(0) == 0) {
+    static_cast<ObjectFileMachO *>(objfile)->CreateSections(unified_list);
+  }
+
+  // If the symbol file is different from the object file, create the symbol
+  // file sections as well.
+  if (&symfile != GetModule()->GetObjectFile()) {
+    static_cast<ObjectFileMachO &>(symfile).CreateSections(unified_list);
+  }
+}
+
 //------------------------------------------------------------------
 // PluginInterface protocol
 //------------------------------------------------------------------
Index: source/Plugins/SymbolVendor/MacOSX/CMakeLists.txt
===================================================================
--- source/Plugins/SymbolVendor/MacOSX/CMakeLists.txt
+++ source/Plugins/SymbolVendor/MacOSX/CMakeLists.txt
@@ -1,10 +1,9 @@
-include_directories(${LIBXML2_INCLUDE_DIR})
-
 add_lldb_library(lldbPluginSymbolVendorMacOSX PLUGIN
   SymbolVendorMacOSX.cpp
 
   LINK_LIBS
     lldbCore
     lldbHost
     lldbSymbol
+    lldbPluginObjectFileMachO
   )
Index: source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
===================================================================
--- source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
+++ source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
@@ -39,6 +39,7 @@
 
   static lldb_private::SymbolVendor *
   CreateInstance(const lldb::ModuleSP &module_sp,
+                 lldb_private::SectionList &unified_list,
                  lldb_private::Stream *feedback_strm);
 
   //------------------------------------------------------------------
@@ -49,6 +50,8 @@
   uint32_t GetPluginVersion() override;
 
 private:
+  lldb_private::SectionList BuildSectionList(lldb_private::ObjectFile &obj);
+
   DISALLOW_COPY_AND_ASSIGN(SymbolVendorELF);
 };
 
Index: source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
===================================================================
--- source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
+++ source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
@@ -63,6 +63,7 @@
 //----------------------------------------------------------------------
 SymbolVendor *
 SymbolVendorELF::CreateInstance(const lldb::ModuleSP &module_sp,
+                                SectionList &unified_list,
                                 lldb_private::Stream *feedback_strm) {
   if (!module_sp)
     return NULL;
@@ -120,47 +121,44 @@
         dsym_objfile_sp->SetType(ObjectFile::eTypeDebugInfo);
 
         SymbolVendorELF *symbol_vendor = new SymbolVendorELF(module_sp);
-        if (symbol_vendor) {
-          // Get the module unified section list and add our debug sections to
-          // that.
-          SectionList *module_section_list = module_sp->GetSectionList();
-          SectionList *objfile_section_list = dsym_objfile_sp->GetSectionList();
-
-          static const SectionType g_sections[] = {
-              eSectionTypeDWARFDebugAbbrev,   eSectionTypeDWARFDebugAddr,
-              eSectionTypeDWARFDebugAranges,  eSectionTypeDWARFDebugCuIndex,
-              eSectionTypeDWARFDebugFrame,    eSectionTypeDWARFDebugInfo,
-              eSectionTypeDWARFDebugLine,     eSectionTypeDWARFDebugLoc,
-              eSectionTypeDWARFDebugMacInfo,  eSectionTypeDWARFDebugPubNames,
-              eSectionTypeDWARFDebugPubTypes, eSectionTypeDWARFDebugRanges,
-              eSectionTypeDWARFDebugStr,      eSectionTypeDWARFDebugStrOffsets,
-              eSectionTypeELFSymbolTable,
-          };
-          for (size_t idx = 0; idx < sizeof(g_sections) / sizeof(g_sections[0]);
-               ++idx) {
-            SectionType section_type = g_sections[idx];
-            SectionSP section_sp(
-                objfile_section_list->FindSectionByType(section_type, true));
-            if (section_sp) {
-              SectionSP module_section_sp(
-                  module_section_list->FindSectionByType(section_type, true));
-              if (module_section_sp)
-                module_section_list->ReplaceSection(module_section_sp->GetID(),
-                                                    section_sp);
-              else
-                module_section_list->AddSection(section_sp);
-            }
-          }
-
-          symbol_vendor->AddSymbolFileRepresentation(dsym_objfile_sp);
-          return symbol_vendor;
-        }
+        unified_list = symbol_vendor->BuildSectionList(*dsym_objfile_sp);
+        symbol_vendor->AddSymbolFileRepresentation(dsym_objfile_sp);
+        return symbol_vendor;
       }
     }
   }
   return NULL;
 }
 
+SectionList SymbolVendorELF::BuildSectionList(ObjectFile &obj) {
+  SectionList UnifiedList = *GetModule()->GetObjectFile()->GetSectionList();
+
+  SectionList *SymfileList = obj.GetSectionList();
+
+  static constexpr SectionType g_sections[] = {
+      eSectionTypeDWARFDebugAbbrev,   eSectionTypeDWARFDebugAddr,
+      eSectionTypeDWARFDebugAranges,  eSectionTypeDWARFDebugCuIndex,
+      eSectionTypeDWARFDebugFrame,    eSectionTypeDWARFDebugInfo,
+      eSectionTypeDWARFDebugLine,     eSectionTypeDWARFDebugLoc,
+      eSectionTypeDWARFDebugMacInfo,  eSectionTypeDWARFDebugPubNames,
+      eSectionTypeDWARFDebugPubTypes, eSectionTypeDWARFDebugRanges,
+      eSectionTypeDWARFDebugStr,      eSectionTypeDWARFDebugStrOffsets,
+      eSectionTypeELFSymbolTable,
+  };
+  for (SectionType ST : g_sections) {
+    SectionSP section_sp = SymfileList->FindSectionByType(ST, true);
+    if (!section_sp)
+      continue;
+
+    SectionSP module_section_sp = UnifiedList.FindSectionByType(ST, true);
+    if (module_section_sp)
+      UnifiedList.ReplaceSection(module_section_sp->GetID(), section_sp);
+    else
+      UnifiedList.AddSection(section_sp);
+  }
+  return UnifiedList;
+}
+
 //------------------------------------------------------------------
 // PluginInterface protocol
 //------------------------------------------------------------------
Index: source/Plugins/SymbolVendor/CMakeLists.txt
===================================================================
--- source/Plugins/SymbolVendor/CMakeLists.txt
+++ source/Plugins/SymbolVendor/CMakeLists.txt
@@ -1,5 +1,2 @@
-if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
-  add_subdirectory(MacOSX)
-endif()
-
 add_subdirectory(ELF)
+add_subdirectory(MacOSX)
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwp.cpp
===================================================================
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwp.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwp.cpp
@@ -125,8 +125,7 @@
     return true;
   }
 
-  const lldb_private::SectionList *section_list =
-      m_obj_file->GetSectionList(false /* update_module_section_list */);
+  const lldb_private::SectionList *section_list = m_obj_file->GetSectionList();
   if (section_list) {
     lldb::SectionSP section_sp(
         section_list->FindSectionByType(sect_type, true));
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
===================================================================
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -29,8 +29,7 @@
 
 void SymbolFileDWARFDwo::LoadSectionData(lldb::SectionType sect_type,
                                          DWARFDataExtractor &data) {
-  const SectionList *section_list =
-      m_obj_file->GetSectionList(false /* update_module_section_list */);
+  const SectionList *section_list = m_obj_file->GetSectionList();
   if (section_list) {
     SectionSP section_sp(section_list->FindSectionByType(sect_type, true));
     if (section_sp) {
@@ -63,7 +62,7 @@
 DWARFCompileUnit *SymbolFileDWARFDwo::GetCompileUnit() {
   // A clang module is found via a skeleton CU, but is not a proper DWO.
   // Clang modules have a .debug_info section instead of the *_dwo variant.
-  if (auto *section_list = m_obj_file->GetSectionList(false))
+  if (auto *section_list = m_obj_file->GetSectionList())
     if (auto section_sp =
             section_list->FindSectionByType(eSectionTypeDWARFDebugInfo, true))
       if (!section_sp->GetName().GetStringRef().endswith("dwo"))
Index: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
===================================================================
--- source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
+++ source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -109,7 +109,7 @@
 
   bool IsStripped() override;
 
-  void CreateSections(lldb_private::SectionList &unified_section_list) override;
+  lldb_private::SectionList *GetSectionList() override;
 
   void Dump(lldb_private::Stream *s) override;
 
Index: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===================================================================
--- source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -662,7 +662,7 @@
   return false;
 }
 
-void ObjectFilePECOFF::CreateSections(SectionList &unified_section_list) {
+SectionList *ObjectFilePECOFF::GetSectionList() {
   if (!m_sections_ap.get()) {
     m_sections_ap.reset(new SectionList());
 
@@ -784,11 +784,11 @@
 
         // section_sp->SetIsEncrypted (segment_is_encrypted);
 
-        unified_section_list.AddSection(section_sp);
         m_sections_ap->AddSection(section_sp);
       }
     }
   }
+  return m_sections_ap.get();
 }
 
 bool ObjectFilePECOFF::GetUUID(UUID *uuid) { return false; }
Index: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===================================================================
--- source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -92,7 +92,11 @@
 
   bool IsStripped() override;
 
-  void CreateSections(lldb_private::SectionList &unified_section_list) override;
+  lldb_private::SectionList *GetSectionList() override;
+
+  /// Create sections for this object file and populate the unified section list
+  /// of the module.
+  void CreateSections(lldb_private::SectionList &unified_section_list);
 
   void Dump(lldb_private::Stream *s) override;
 
Index: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===================================================================
--- source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1349,8 +1349,16 @@
   return false;
 }
 
+SectionList *ObjectFileMachO::GetSectionList() {
+  if (!m_sections_ap) {
+    // Resolving the module's symbol vendor will populate our section list as
+    // the vendor will call back into the CreateSections funcion below.
+    GetModule()->GetSymbolVendor();
+  }
+  return m_sections_ap.get();
+}
+
 void ObjectFileMachO::CreateSections(SectionList &unified_section_list) {
-  if (!m_sections_ap.get()) {
     m_sections_ap.reset(new SectionList());
 
     const bool is_dsym = (m_header.filetype == MH_DSYM);
@@ -1919,7 +1927,6 @@
     if (section_file_addresses_changed && module_sp.get()) {
       module_sp->SectionFileAddressesChanged();
     }
-  }
 }
 
 class MachSymtabSectionInfo {
Index: source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
===================================================================
--- source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
+++ source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
@@ -73,7 +73,7 @@
 
   bool IsStripped() override;
 
-  void CreateSections(lldb_private::SectionList &unified_section_list) override;
+  lldb_private::SectionList *GetSectionList() override;
 
   void Dump(lldb_private::Stream *s) override;
 
Index: source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
===================================================================
--- source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
+++ source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
@@ -134,15 +134,15 @@
   return false; // JIT code that is in a module is never stripped
 }
 
-void ObjectFileJIT::CreateSections(SectionList &unified_section_list) {
+SectionList *ObjectFileJIT::GetSectionList() {
   if (!m_sections_ap.get()) {
     m_sections_ap.reset(new SectionList());
     ObjectFileJITDelegateSP delegate_sp(m_delegate_wp.lock());
     if (delegate_sp) {
       delegate_sp->PopulateSectionList(this, *m_sections_ap);
-      unified_section_list = *m_sections_ap;
     }
   }
+  return m_sections_ap.get();
 }
 
 void ObjectFileJIT::Dump(Stream *s) {
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.h
===================================================================
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.h
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.h
@@ -119,7 +119,7 @@
 
   bool IsStripped() override;
 
-  void CreateSections(lldb_private::SectionList &unified_section_list) override;
+  lldb_private::SectionList *GetSectionList() override;
 
   void Dump(lldb_private::Stream *s) override;
 
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===================================================================
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1770,7 +1770,7 @@
   return 0;
 }
 
-void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
+SectionList *ObjectFileELF::GetSectionList() {
   if (!m_sections_ap.get() && ParseSectionHeaders()) {
     m_sections_ap.reset(new SectionList());
 
@@ -1985,39 +1985,7 @@
       m_sections_ap->AddSection(section_sp);
     }
   }
-
-  if (m_sections_ap.get()) {
-    if (GetType() == eTypeDebugInfo) {
-      static const SectionType g_sections[] = {
-          eSectionTypeDWARFDebugAbbrev,   eSectionTypeDWARFDebugAddr,
-          eSectionTypeDWARFDebugAranges,  eSectionTypeDWARFDebugCuIndex,
-          eSectionTypeDWARFDebugFrame,    eSectionTypeDWARFDebugInfo,
-          eSectionTypeDWARFDebugLine,     eSectionTypeDWARFDebugLoc,
-          eSectionTypeDWARFDebugMacInfo,  eSectionTypeDWARFDebugPubNames,
-          eSectionTypeDWARFDebugPubTypes, eSectionTypeDWARFDebugRanges,
-          eSectionTypeDWARFDebugStr,      eSectionTypeDWARFDebugStrOffsets,
-          eSectionTypeELFSymbolTable,
-      };
-      SectionList *elf_section_list = m_sections_ap.get();
-      for (size_t idx = 0; idx < sizeof(g_sections) / sizeof(g_sections[0]);
-           ++idx) {
-        SectionType section_type = g_sections[idx];
-        SectionSP section_sp(
-            elf_section_list->FindSectionByType(section_type, true));
-        if (section_sp) {
-          SectionSP module_section_sp(
-              unified_section_list.FindSectionByType(section_type, true));
-          if (module_section_sp)
-            unified_section_list.ReplaceSection(module_section_sp->GetID(),
-                                                section_sp);
-          else
-            unified_section_list.AddSection(section_sp);
-        }
-      }
-    } else {
-      unified_section_list = *m_sections_ap;
-    }
-  }
+  return m_sections_ap.get();
 }
 
 // Find the arm/aarch64 mapping symbol character in the given symbol name.
Index: source/Core/Module.cpp
===================================================================
--- source/Core/Module.cpp
+++ source/Core/Module.cpp
@@ -1039,20 +1039,27 @@
 
 SymbolVendor *Module::GetSymbolVendor(bool can_create,
                                       lldb_private::Stream *feedback_strm) {
+  LoadSymbolVendorAndSectionList(can_create, feedback_strm);
+  return m_symfile_ap.get();
+}
+
+void Module::LoadSymbolVendorAndSectionList(
+    bool can_create, lldb_private::Stream *feedback_strm) {
   if (!m_did_load_symbol_vendor.load()) {
     std::lock_guard<std::recursive_mutex> guard(m_mutex);
     if (!m_did_load_symbol_vendor.load() && can_create) {
       ObjectFile *obj_file = GetObjectFile();
       if (obj_file != nullptr) {
+        m_did_load_symbol_vendor = true;
         static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
         Timer scoped_timer(func_cat, LLVM_PRETTY_FUNCTION);
-        m_symfile_ap.reset(
-            SymbolVendor::FindPlugin(shared_from_this(), feedback_strm));
-        m_did_load_symbol_vendor = true;
+        if (!m_sections_ap)
+          m_sections_ap = llvm::make_unique<SectionList>();
+        m_symfile_ap.reset(SymbolVendor::FindPlugin(
+            shared_from_this(), *m_sections_ap, feedback_strm));
       }
     }
   }
-  return m_symfile_ap.get();
 }
 
 void Module::SetFileSpecAndObjectName(const FileSpec &file,
@@ -1278,12 +1285,7 @@
 }
 
 SectionList *Module::GetSectionList() {
-  // Populate m_unified_sections_ap with sections from objfile.
-  if (!m_sections_ap) {
-    ObjectFile *obj_file = GetObjectFile();
-    if (obj_file != nullptr)
-      obj_file->CreateSections(*GetUnifiedSectionList());
-  }
+  LoadSymbolVendorAndSectionList();
   return m_sections_ap.get();
 }
 
@@ -1296,13 +1298,6 @@
     sym_vendor->SectionFileAddressesChanged();
 }
 
-SectionList *Module::GetUnifiedSectionList() {
-  // Populate m_unified_sections_ap with sections from objfile.
-  if (!m_sections_ap)
-    m_sections_ap = llvm::make_unique<SectionList>();
-  return m_sections_ap.get();
-}
-
 const Symbol *Module::FindFirstSymbolWithNameAndType(const ConstString &name,
                                                      SymbolType symbol_type) {
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
@@ -1459,17 +1454,6 @@
             return;
           }
         }
-
-        if (obj_file != m_objfile_sp.get()) {
-          size_t num_sections = section_list->GetNumSections(0);
-          for (size_t idx = num_sections; idx > 0; --idx) {
-            lldb::SectionSP section_sp(
-                section_list->GetSectionAtIndex(idx - 1));
-            if (section_sp->GetObjectFile() == obj_file) {
-              section_list->DeleteSection(idx - 1);
-            }
-          }
-        }
       }
     }
     // Keep all old symbol files around in case there are any lingering type
Index: lit/lit.cfg
===================================================================
--- lit/lit.cfg
+++ lit/lit.cfg
@@ -28,6 +28,8 @@
 # suffixes: We only support unit tests
 config.suffixes = []
 
+config.excludes = ['Inputs']
+
 # test_source_root: The root path where tests are located.
 config.test_source_root = os.path.dirname(__file__)
 
Index: lit/Modules/unified-section-list.test
===================================================================
--- /dev/null
+++ lit/Modules/unified-section-list.test
@@ -0,0 +1,8 @@
+RUN: mkdir -p %t/.build-id/1b
+RUN: yaml2obj %S/Inputs/stripped.yaml > %t/stripped.out
+RUN: yaml2obj %S/Inputs/unstripped.yaml > %t/.build-id/1b/8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9.debug
+RUN: lldb-test module-sections %t/stripped.out | FileCheck %s
+
+CHECK: Name: .debug_frame
+CHECK-NEXT: VM size: 0
+CHECK-NEXT: File size: 8
Index: lit/Modules/lit.local.cfg
===================================================================
--- lit/Modules/lit.local.cfg
+++ lit/Modules/lit.local.cfg
@@ -1 +1 @@
-config.suffixes = ['.yaml']
+config.suffixes = ['.yaml', '.test']
Index: lit/Modules/Inputs/unstripped.yaml
===================================================================
--- /dev/null
+++ lit/Modules/Inputs/unstripped.yaml
@@ -0,0 +1,32 @@
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x00000000004003D0
+Sections:
+  - Name:            .debug_frame
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000008
+    Content:         DEADBEEFBAADF00D
+  - Name:            .note.gnu.build-id
+    Type:            SHT_NOTE
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x0000000000400274
+    AddressAlign:    0x0000000000000004
+    Content:         040000001400000003000000474E55001B8A73AC238390E32A7FF4AC8EBE4D6A41ECF5C9
+  - Name:            .text
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x00000000004003D0
+    AddressAlign:    0x0000000000000010
+    Size:            0x0000000000000008
+Symbols:
+  Local:
+    - Name:            main
+      Type:            STT_FUNC
+      Section:         .text
+      Value:           0x00000000004003D0
+      Size:            0x0000000000000008
+...
Index: lit/Modules/Inputs/stripped.yaml
===================================================================
--- /dev/null
+++ lit/Modules/Inputs/stripped.yaml
@@ -0,0 +1,25 @@
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x00000000004003D0
+Sections:
+  - Name:            .note.gnu.build-id
+    Type:            SHT_NOTE
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x0000000000400274
+    AddressAlign:    0x0000000000000004
+    Content:         040000001400000003000000474E55001B8A73AC238390E32A7FF4AC8EBE4D6A41ECF5C9
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x00000000004003D0
+    AddressAlign:    0x0000000000000010
+    Content:         DEADBEEFBAADF00D
+  - Name:            .gnu_debuglink
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000004
+    Content:         38613733616332333833393065333261376666346163386562653464366134316563663563392E646562756700000000ADEE50C1
+...
Index: include/lldb/lldb-private-interfaces.h
===================================================================
--- include/lldb/lldb-private-interfaces.h
+++ include/lldb/lldb-private-interfaces.h
@@ -70,7 +70,7 @@
     CommandInterpreter &interpreter);
 typedef SymbolFile *(*SymbolFileCreateInstance)(ObjectFile *obj_file);
 typedef SymbolVendor *(*SymbolVendorCreateInstance)(
-    const lldb::ModuleSP &module_sp,
+    const lldb::ModuleSP &module_sp, SectionList &unified_list,
     lldb_private::Stream
         *feedback_strm); // Module can be NULL for default system symbol vendor
 typedef bool (*BreakpointHitCallback)(void *baton,
Index: include/lldb/Symbol/SymbolVendor.h
===================================================================
--- include/lldb/Symbol/SymbolVendor.h
+++ include/lldb/Symbol/SymbolVendor.h
@@ -33,7 +33,12 @@
 //----------------------------------------------------------------------
 class SymbolVendor : public ModuleChild, public PluginInterface {
 public:
+  /// Creates a SymbolVendor plugin for the module_sp argument. The
+  /// unified_list argument is a reference to the unified section list of the
+  /// module and it should be filled out by the symbol vendor before
+  /// constructing the SymbolFile object.
   static SymbolVendor *FindPlugin(const lldb::ModuleSP &module_sp,
+                                  SectionList &unified_list,
                                   Stream *feedback_strm);
 
   //------------------------------------------------------------------
Index: include/lldb/Symbol/ObjectFile.h
===================================================================
--- include/lldb/Symbol/ObjectFile.h
+++ include/lldb/Symbol/ObjectFile.h
@@ -329,9 +329,7 @@
   /// @return
   ///     The list of sections contained in this object file.
   //------------------------------------------------------------------
-  virtual SectionList *GetSectionList(bool update_module_section_list = true);
-
-  virtual void CreateSections(SectionList &unified_section_list) = 0;
+  virtual SectionList *GetSectionList() = 0;
 
   //------------------------------------------------------------------
   /// Notify the ObjectFile that the file addresses in the Sections
Index: include/lldb/Core/Module.h
===================================================================
--- include/lldb/Core/Module.h
+++ include/lldb/Core/Module.h
@@ -698,7 +698,7 @@
   /// @return
   ///     Unified module section list.
   //------------------------------------------------------------------
-  virtual SectionList *GetSectionList();
+  SectionList *GetSectionList();
 
   //------------------------------------------------------------------
   /// Notify the module that the file addresses for the Sections have
@@ -1196,7 +1196,9 @@
 
   bool SetArchitecture(const ArchSpec &new_arch);
 
-  SectionList *GetUnifiedSectionList();
+  void
+  LoadSymbolVendorAndSectionList(bool can_create = true,
+                                 lldb_private::Stream *feedback_strm = nullptr);
 
   friend class ModuleList;
   friend class ObjectFile;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to