Skip to content

[lldb][NFC] Make the target's SectionLoadList private. #113278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 15, 2025

Conversation

clayborg
Copy link
Collaborator

@clayborg clayborg commented Oct 22, 2024

Lots of code around LLDB was directly accessing the target's section load list. This NFC patch makes the section load list private so the Target class can access it, but everyone else now uses accessor functions. This allows us to control the resolving of addresses and will allow for functionality in LLDB which can lazily resolve addresses in JIT plug-ins with a future patch.

@llvmbot llvmbot added the lldb label Oct 22, 2024
@clayborg clayborg changed the title Make the target's SectionLoadList private. [lldb][NFC] Make the target's SectionLoadList private. Oct 22, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 22, 2024

@llvm/pr-subscribers-lldb

Author: Greg Clayton (clayborg)

Changes

Lots of code around LLDB was directly accessing the target's section load list. This NFC patch makes the section load list private to the Target class can access it, but everyone else now uses accessor functions. This allows us to control the resolving of addresses and will allow for functionality in LLDB which can lazily resolve addresses in JIT plug-ins with a future patch.


Patch is 36.30 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/113278.diff

33 Files Affected:

  • (modified) lldb/include/lldb/Target/Target.h (+12-3)
  • (modified) lldb/source/API/SBBreakpoint.cpp (+2-2)
  • (modified) lldb/source/Breakpoint/BreakpointLocationList.cpp (+1-2)
  • (modified) lldb/source/Commands/CommandObjectDisassemble.cpp (+3-3)
  • (modified) lldb/source/Commands/CommandObjectRegister.cpp (+2-2)
  • (modified) lldb/source/Commands/CommandObjectSource.cpp (+4-5)
  • (modified) lldb/source/Commands/CommandObjectTarget.cpp (+7-8)
  • (modified) lldb/source/Core/Address.cpp (+3-5)
  • (modified) lldb/source/Core/Disassembler.cpp (+1-5)
  • (modified) lldb/source/Core/DumpDataExtractor.cpp (+4-6)
  • (modified) lldb/source/Core/FormatEntity.cpp (+1-1)
  • (modified) lldb/source/Core/Section.cpp (+2-2)
  • (modified) lldb/source/Core/Value.cpp (+2-3)
  • (modified) lldb/source/DataFormatters/CXXFunctionPointer.cpp (+2-3)
  • (modified) lldb/source/Expression/ObjectFileJIT.cpp (+2-2)
  • (modified) lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp (+1-2)
  • (modified) lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp (+3-3)
  • (modified) lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp (+1-1)
  • (modified) lldb/source/Plugins/DynamicLoader/Static/DynamicLoaderStatic.cpp (+2-2)
  • (modified) lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp (+3-6)
  • (modified) lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp (+1-1)
  • (modified) lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp (+8-10)
  • (modified) lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (+1-2)
  • (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (+5-5)
  • (modified) lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp (+1-1)
  • (modified) lldb/source/Plugins/ObjectFile/Placeholder/ObjectFilePlaceholder.cpp (+1-2)
  • (modified) lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp (+2-2)
  • (modified) lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp (+1-1)
  • (modified) lldb/source/Symbol/ObjectFile.cpp (+1-2)
  • (modified) lldb/source/Target/ProcessTrace.cpp (+1-1)
  • (modified) lldb/source/Target/Target.cpp (+14)
  • (modified) lldb/source/Target/ThreadPlanStepInRange.cpp (+1-2)
  • (modified) lldb/source/Target/ThreadPlanTracer.cpp (+1-2)
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index e4848f19e64d62..5f43b4f5060d0a 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1140,9 +1140,14 @@ class Target : public std::enable_shared_from_this<Target>,
                              Address &pointer_addr,
                              bool force_live_memory = false);
 
-  SectionLoadList &GetSectionLoadList() {
-    return m_section_load_history.GetCurrentSectionLoadList();
-  }
+
+  bool SectionLoadListIsEmpty() const;
+
+  lldb::addr_t GetSectionLoadAddress(const lldb::SectionSP &section_sp);
+
+  void ClearSectionLoadList();
+
+  void DumpSectionLoadList(Stream &s);
 
   static Target *GetTargetFromContexts(const ExecutionContext *exe_ctx_ptr,
                                        const SymbolContext *sc_ptr);
@@ -1653,6 +1658,10 @@ class Target : public std::enable_shared_from_this<Target>,
 
   Target(const Target &) = delete;
   const Target &operator=(const Target &) = delete;
+
+  SectionLoadList &GetSectionLoadList() {
+    return m_section_load_history.GetCurrentSectionLoadList();
+  }
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/API/SBBreakpoint.cpp b/lldb/source/API/SBBreakpoint.cpp
index b2ed034d19983c..87fadbcec4f26b 100644
--- a/lldb/source/API/SBBreakpoint.cpp
+++ b/lldb/source/API/SBBreakpoint.cpp
@@ -137,7 +137,7 @@ SBBreakpointLocation SBBreakpoint::FindLocationByAddress(addr_t vm_addr) {
           bkpt_sp->GetTarget().GetAPIMutex());
       Address address;
       Target &target = bkpt_sp->GetTarget();
-      if (!target.GetSectionLoadList().ResolveLoadAddress(vm_addr, address)) {
+      if (!target.ResolveLoadAddress(vm_addr, address)) {
         address.SetRawAddress(vm_addr);
       }
       sb_bp_location.SetLocation(bkpt_sp->FindLocationByAddress(address));
@@ -157,7 +157,7 @@ break_id_t SBBreakpoint::FindLocationIDByAddress(addr_t vm_addr) {
         bkpt_sp->GetTarget().GetAPIMutex());
     Address address;
     Target &target = bkpt_sp->GetTarget();
-    if (!target.GetSectionLoadList().ResolveLoadAddress(vm_addr, address)) {
+    if (!target.ResolveLoadAddress(vm_addr, address)) {
       address.SetRawAddress(vm_addr);
     }
     break_id = bkpt_sp->FindLocationIDByAddress(address);
diff --git a/lldb/source/Breakpoint/BreakpointLocationList.cpp b/lldb/source/Breakpoint/BreakpointLocationList.cpp
index e0f1b9b2c80883..0240305d6f2920 100644
--- a/lldb/source/Breakpoint/BreakpointLocationList.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocationList.cpp
@@ -103,8 +103,7 @@ BreakpointLocationList::FindByAddress(const Address &addr) const {
       so_addr = addr;
     } else {
       // Try and resolve as a load address if possible.
-      m_owner.GetTarget().GetSectionLoadList().ResolveLoadAddress(
-          addr.GetOffset(), so_addr);
+      m_owner.GetTarget().ResolveLoadAddress(addr.GetOffset(), so_addr);
       if (!so_addr.IsValid()) {
         // The address didn't resolve, so just set to passed in addr.
         so_addr = addr;
diff --git a/lldb/source/Commands/CommandObjectDisassemble.cpp b/lldb/source/Commands/CommandObjectDisassemble.cpp
index 250f849ac04c55..22b29b1e8a8cdf 100644
--- a/lldb/source/Commands/CommandObjectDisassemble.cpp
+++ b/lldb/source/Commands/CommandObjectDisassemble.cpp
@@ -254,10 +254,10 @@ CommandObjectDisassemble::GetContainingAddressRanges() {
   };
 
   Target &target = GetTarget();
-  if (!target.GetSectionLoadList().IsEmpty()) {
+  if (!target.SectionLoadListIsEmpty()) {
     Address symbol_containing_address;
-    if (target.GetSectionLoadList().ResolveLoadAddress(
-            m_options.symbol_containing_addr, symbol_containing_address)) {
+    if (target.ResolveLoadAddress(m_options.symbol_containing_addr, 
+                                  symbol_containing_address)) {
       get_range(symbol_containing_address);
     }
   } else {
diff --git a/lldb/source/Commands/CommandObjectRegister.cpp b/lldb/source/Commands/CommandObjectRegister.cpp
index 4e047ccbc10b92..cb53d7c21b8cd9 100644
--- a/lldb/source/Commands/CommandObjectRegister.cpp
+++ b/lldb/source/Commands/CommandObjectRegister.cpp
@@ -95,8 +95,8 @@ class CommandObjectRegisterRead : public CommandObjectParsed {
         addr_t reg_addr = reg_value.GetAsUInt64(LLDB_INVALID_ADDRESS);
         if (reg_addr != LLDB_INVALID_ADDRESS) {
           Address so_reg_addr;
-          if (exe_ctx.GetTargetRef().GetSectionLoadList().ResolveLoadAddress(
-                  reg_addr, so_reg_addr)) {
+          if (exe_ctx.GetTargetRef().ResolveLoadAddress(reg_addr, 
+                                                        so_reg_addr)) {
             strm.PutCString("  ");
             so_reg_addr.Dump(&strm, exe_ctx.GetBestExecutionContextScope(),
                              Address::DumpStyleResolvedDescription);
diff --git a/lldb/source/Commands/CommandObjectSource.cpp b/lldb/source/Commands/CommandObjectSource.cpp
index 86c090f9f36c16..5a3b8c49f1cf33 100644
--- a/lldb/source/Commands/CommandObjectSource.cpp
+++ b/lldb/source/Commands/CommandObjectSource.cpp
@@ -302,7 +302,7 @@ class CommandObjectSourceInfo : public CommandObjectParsed {
     size_t num_matches = 0;
     assert(module_list.GetSize() > 0);
     Target &target = GetTarget();
-    if (target.GetSectionLoadList().IsEmpty()) {
+    if (target.SectionLoadListIsEmpty()) {
       // The target isn't loaded yet, we need to lookup the file address in all
       // modules.  Note: the module list option does not apply to addresses.
       const size_t num_modules = module_list.GetSize();
@@ -328,7 +328,7 @@ class CommandObjectSourceInfo : public CommandObjectParsed {
     } else {
       // The target has some things loaded, resolve this address to a compile
       // unit + file + line and display
-      if (target.GetSectionLoadList().ResolveLoadAddress(addr, so_addr)) {
+      if (target.ResolveLoadAddress(addr, so_addr)) {
         ModuleSP module_sp(so_addr.GetModule());
         // Check to make sure this module is in our list.
         if (module_sp && module_list.GetIndexForModule(module_sp.get()) !=
@@ -961,7 +961,7 @@ class CommandObjectSourceList : public CommandObjectParsed {
       StreamString error_strm;
       SymbolContextList sc_list;
 
-      if (target.GetSectionLoadList().IsEmpty()) {
+      if (target.SectionLoadListIsEmpty()) {
         // The target isn't loaded yet, we need to lookup the file address in
         // all modules
         const ModuleList &module_list = target.GetImages();
@@ -989,8 +989,7 @@ class CommandObjectSourceList : public CommandObjectParsed {
       } else {
         // The target has some things loaded, resolve this address to a compile
         // unit + file + line and display
-        if (target.GetSectionLoadList().ResolveLoadAddress(m_options.address,
-                                                           so_addr)) {
+        if (target.ResolveLoadAddress(m_options.address, so_addr)) {
           ModuleSP module_sp(so_addr.GetModule());
           if (module_sp) {
             SymbolContext sc;
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index e950fb346c253b..3642d22a9b644e 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -1522,8 +1522,8 @@ static bool LookupAddressInModule(CommandInterpreter &interpreter, Stream &strm,
     Address so_addr;
     SymbolContext sc;
     Target *target = interpreter.GetExecutionContext().GetTargetPtr();
-    if (target && !target->GetSectionLoadList().IsEmpty()) {
-      if (!target->GetSectionLoadList().ResolveLoadAddress(addr, so_addr))
+    if (target && !target->SectionLoadListIsEmpty()) {
+      if (!target->ResolveLoadAddress(addr, so_addr))
         return false;
       else if (so_addr.GetModule().get() != module)
         return false;
@@ -2974,8 +2974,8 @@ class CommandObjectTargetModulesLoad
                               sect_name);
                           break;
                         } else {
-                          if (target.GetSectionLoadList().SetSectionLoadAddress(
-                                  section_sp, load_addr))
+                          if (target.SetSectionLoadAddress(section_sp, 
+                                                           load_addr))
                             changed = true;
                           result.AppendMessageWithFormat(
                               "section '%s' loaded at 0x%" PRIx64 "\n",
@@ -3329,7 +3329,7 @@ class CommandObjectTargetModulesList : public CommandObjectParsed {
           if (objfile) {
             Address base_addr(objfile->GetBaseAddress());
             if (base_addr.IsValid()) {
-              if (!target.GetSectionLoadList().IsEmpty()) {
+              if (!target.SectionLoadListIsEmpty()) {
                 lldb::addr_t load_addr = base_addr.GetLoadAddress(&target);
                 if (load_addr == LLDB_INVALID_ADDRESS) {
                   base_addr.Dump(&strm, &target,
@@ -3544,8 +3544,7 @@ class CommandObjectTargetModulesShowUnwind : public CommandObjectParsed {
                                         function_options, sc_list);
     } else if (m_options.m_type == eLookupTypeAddress && target) {
       Address addr;
-      if (target->GetSectionLoadList().ResolveLoadAddress(m_options.m_addr,
-                                                          addr)) {
+      if (target->ResolveLoadAddress(m_options.m_addr, addr)) {
         SymbolContext sc;
         ModuleSP module_sp(addr.GetModule());
         module_sp->ResolveSymbolContextForAddress(addr,
@@ -5270,7 +5269,7 @@ class CommandObjectTargetDumpSectionLoadList : public CommandObjectParsed {
 protected:
   void DoExecute(Args &command, CommandReturnObject &result) override {
     Target &target = GetTarget();
-    target.GetSectionLoadList().Dump(result.GetOutputStream(), &target);
+    target.DumpSectionLoadList(result.GetOutputStream());
     result.SetStatus(eReturnStatusSuccessFinishResult);
   }
 };
diff --git a/lldb/source/Core/Address.cpp b/lldb/source/Core/Address.cpp
index 5a4751bd5256eb..e8363f0a7c077e 100644
--- a/lldb/source/Core/Address.cpp
+++ b/lldb/source/Core/Address.cpp
@@ -138,9 +138,8 @@ static bool ReadAddress(ExecutionContextScope *exe_scope,
     // If we have any sections that are loaded, try and resolve using the
     // section load list
     Target *target = exe_ctx.GetTargetPtr();
-    if (target && !target->GetSectionLoadList().IsEmpty()) {
-      if (target->GetSectionLoadList().ResolveLoadAddress(deref_addr,
-                                                          deref_so_addr))
+    if (target && !target->SectionLoadListIsEmpty()) {
+      if (target->ResolveLoadAddress(deref_addr, deref_so_addr))
         return true;
     } else {
       // If we were not running, yet able to read an integer, we must have a
@@ -1046,8 +1045,7 @@ AddressClass Address::GetAddressClass() const {
 
 bool Address::SetLoadAddress(lldb::addr_t load_addr, Target *target,
                              bool allow_section_end) {
-  if (target && target->GetSectionLoadList().ResolveLoadAddress(
-                    load_addr, *this, allow_section_end))
+  if (target && target->ResolveLoadAddress(load_addr, *this, allow_section_end))
     return true;
   m_section_wp.reset();
   m_offset = load_addr;
diff --git a/lldb/source/Core/Disassembler.cpp b/lldb/source/Core/Disassembler.cpp
index d071e3bfe4f77d..6c4f43ca06b6d0 100644
--- a/lldb/source/Core/Disassembler.cpp
+++ b/lldb/source/Core/Disassembler.cpp
@@ -102,11 +102,7 @@ static Address ResolveAddress(Target &target, const Address &addr) {
     Address resolved_addr;
     // If we weren't passed in a section offset address range, try and resolve
     // it to something
-    bool is_resolved = target.GetSectionLoadList().IsEmpty()
-                           ? target.GetImages().ResolveFileAddress(
-                                 addr.GetOffset(), resolved_addr)
-                           : target.GetSectionLoadList().ResolveLoadAddress(
-                                 addr.GetOffset(), resolved_addr);
+    bool is_resolved = target.SectionLoadListIsEmpty() ? target.GetImages().ResolveFileAddress(addr.GetOffset(), resolved_addr) : target.ResolveLoadAddress(addr.GetOffset(), resolved_addr);
 
     // We weren't able to resolve the address, just treat it as a raw address
     if (is_resolved && resolved_addr.IsValid())
diff --git a/lldb/source/Core/DumpDataExtractor.cpp b/lldb/source/Core/DumpDataExtractor.cpp
index 826edd7bab046e..d02ee63aeefa85 100644
--- a/lldb/source/Core/DumpDataExtractor.cpp
+++ b/lldb/source/Core/DumpDataExtractor.cpp
@@ -135,10 +135,10 @@ static lldb::offset_t DumpInstructions(const DataExtractor &DE, Stream *s,
       lldb::addr_t addr = base_addr + start_offset;
       lldb_private::Address so_addr;
       bool data_from_file = true;
-      if (target_sp->GetSectionLoadList().ResolveLoadAddress(addr, so_addr)) {
+      if (target_sp->ResolveLoadAddress(addr, so_addr)) {
         data_from_file = false;
       } else {
-        if (target_sp->GetSectionLoadList().IsEmpty() ||
+        if (target_sp->SectionLoadListIsEmpty() ||
             !target_sp->GetImages().ResolveFileAddress(addr, so_addr))
           so_addr.SetRawAddress(addr);
       }
@@ -706,8 +706,7 @@ lldb::offset_t lldb_private::DumpDataExtractor(
         TargetSP target_sp(exe_scope->CalculateTarget());
         lldb_private::Address so_addr;
         if (target_sp) {
-          if (target_sp->GetSectionLoadList().ResolveLoadAddress(addr,
-                                                                 so_addr)) {
+          if (target_sp->ResolveLoadAddress(addr, so_addr)) {
             s->PutChar(' ');
             so_addr.Dump(s, exe_scope, Address::DumpStyleResolvedDescription,
                          Address::DumpStyleModuleWithFileAddress);
@@ -718,8 +717,7 @@ lldb::offset_t lldb_private::DumpDataExtractor(
             if (ProcessSP process_sp = exe_scope->CalculateProcess()) {
               if (ABISP abi_sp = process_sp->GetABI()) {
                 addr_t addr_fixed = abi_sp->FixCodeAddress(addr);
-                if (target_sp->GetSectionLoadList().ResolveLoadAddress(
-                        addr_fixed, so_addr)) {
+                if (target_sp->ResolveLoadAddress(addr_fixed, so_addr)) {
                   s->PutChar(' ');
                   s->Printf("(0x%*.*" PRIx64 ")", (int)(2 * item_byte_size),
                             (int)(2 * item_byte_size), addr_fixed);
diff --git a/lldb/source/Core/FormatEntity.cpp b/lldb/source/Core/FormatEntity.cpp
index 8f28cd4b5fb255..e6ced91a9be369 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -411,7 +411,7 @@ static bool DumpAddressAndContent(Stream &s, const SymbolContext *sc,
                                   bool print_file_addr_or_load_addr) {
   Target *target = Target::GetTargetFromContexts(exe_ctx, sc);
   addr_t vaddr = LLDB_INVALID_ADDRESS;
-  if (exe_ctx && !target->GetSectionLoadList().IsEmpty())
+  if (exe_ctx && !target->SectionLoadListIsEmpty())
     vaddr = addr.GetLoadAddress(target);
   if (vaddr == LLDB_INVALID_ADDRESS)
     vaddr = addr.GetFileAddress();
diff --git a/lldb/source/Core/Section.cpp b/lldb/source/Core/Section.cpp
index 0763e88d4608f4..12a78ffea2721e 100644
--- a/lldb/source/Core/Section.cpp
+++ b/lldb/source/Core/Section.cpp
@@ -234,7 +234,7 @@ addr_t Section::GetLoadBaseAddress(Target *target) const {
       load_base_addr += GetOffset();
   }
   if (load_base_addr == LLDB_INVALID_ADDRESS) {
-    load_base_addr = target->GetSectionLoadList().GetSectionLoadAddress(
+    load_base_addr = target->GetSectionLoadAddress(
         const_cast<Section *>(this)->shared_from_this());
   }
   return load_base_addr;
@@ -638,7 +638,7 @@ bool SectionList::ContainsSection(user_id_t sect_id) const {
 void SectionList::Dump(llvm::raw_ostream &s, unsigned indent, Target *target,
                        bool show_header, uint32_t depth) const {
   bool target_has_loaded_sections =
-      target && !target->GetSectionLoadList().IsEmpty();
+      target && !target->SectionLoadListIsEmpty();
   if (show_header && !m_sections.empty()) {
     s.indent(indent);
     s << llvm::formatv(
diff --git a/lldb/source/Core/Value.cpp b/lldb/source/Core/Value.cpp
index bd93c04c16d24f..00647e75c38f3a 100644
--- a/lldb/source/Core/Value.cpp
+++ b/lldb/source/Core/Value.cpp
@@ -364,10 +364,9 @@ Status Value::GetValueAsData(ExecutionContext *exe_ctx, DataExtractor &data,
           // memory sections loaded. This allows you to use "target modules
           // load" to load your executable and any shared libraries, then
           // execute commands where you can look at types in data sections.
-          const SectionLoadList &target_sections = target->GetSectionLoadList();
-          if (!target_sections.IsEmpty()) {
+         if (!target->SectionLoadListIsEmpty()) {
             address = m_value.ULongLong(LLDB_INVALID_ADDRESS);
-            if (target_sections.ResolveLoadAddress(address, file_so_addr)) {
+            if (target->ResolveLoadAddress(address, file_so_addr)) {
               address_type = eAddressTypeLoad;
               data.SetByteOrder(target->GetArchitecture().GetByteOrder());
               data.SetAddressByteSize(
diff --git a/lldb/source/DataFormatters/CXXFunctionPointer.cpp b/lldb/source/DataFormatters/CXXFunctionPointer.cpp
index 6543433d17ff45..323b560b084b83 100644
--- a/lldb/source/DataFormatters/CXXFunctionPointer.cpp
+++ b/lldb/source/DataFormatters/CXXFunctionPointer.cpp
@@ -39,9 +39,8 @@ bool lldb_private::formatters::CXXFunctionPointerSummaryProvider(
 
       Address so_addr;
       Target *target = exe_ctx.GetTargetPtr();
-      if (target && !target->GetSectionLoadList().IsEmpty()) {
-        target->GetSectionLoadList().ResolveLoadAddress(func_ptr_address,
-                                                        so_addr);
+      if (target && !target->SectionLoadListIsEmpty()) {
+        target->ResolveLoadAddress(func_ptr_address, so_addr);
         if (so_addr.GetSection() == nullptr) {
           // If we have an address that doesn't correspond to any symbol,
           // it might have authentication bits.  Strip them & see if it
diff --git a/lldb/source/Expression/ObjectFileJIT.cpp b/lldb/source/Expression/ObjectFileJIT.cpp
index 9a839866096bdd..224f420787f491 100644
--- a/lldb/source/Expression/ObjectFileJIT.cpp
+++ b/lldb/source/Expression/ObjectFileJIT.cpp
@@ -178,8 +178,8 @@ bool ObjectFileJIT::SetLoadAddress(Target &target, lldb::addr_t value,
       SectionSP section_sp(section_list->GetSectionAtIndex(sect_idx));
       if (section_sp && section_sp->GetFileSize() > 0 &&
           !section_sp->IsThreadSpecific()) {
-        if (target.GetSectionLoadList().SetSectionLoadAddress(
-                section_sp, section_sp->GetFileAddress() + value))
+        if (target.SetSectionLoadAddress(section_sp, 
+                                         section_sp->GetFileAddress() + value))
           ++num_loaded_sections;
       }
     }
diff --git a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp
index a9f2e7af601db9..f56153e1f857ef 100644
--- a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp
+++ b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp
@@ -76,8 +76,7 @@ lldb::addr_t ArchitectureMips::GetBreakableLoadAddress(lldb::addr_t addr,
 
   Address resolved_addr;
 
-  SectionLoadList &section_load_list = target.GetSectionLoadList();
-  if (section_load_list.IsEmpty())
+  if (target.SectionLoadListIsEmpty())
     // No sections are loaded, so we must assume we are not running yet and
     // need to operate only on file address.
     target.ResolveFileAddress(addr, resolved_addr);
diff --git a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
index 31edd8d46c444e..6aba1c781da61b 100644
--- a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1780,9 +1780,9 @@ const char *DisassemblerLLVMC::SymbolLookup(uint64_t value, uint64_t *type_ptr,
           module_sp->ResolveFileAddress(value, value_so_addr);
           module_sp->ResolveFileAddress(pc, pc_so_addr);
         ...
[truncated]

Copy link

github-actions bot commented Oct 22, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protecting the section load list seems reasonable. I left one inline comment + this needs to be clang-format'ed.

@@ -5066,3 +5066,17 @@ llvm::json::Value
Target::ReportStatistics(const lldb_private::StatisticsOptions &options) {
return m_stats.ToJSON(*this, options);
}

bool Target::SectionLoadListIsEmpty() const {
return const_cast<Target *>(this)->GetSectionLoadList().IsEmpty();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the const_cast, can we have a const GetSectionLoadList overload? SectionLoadHistory::IsEmpty() is already const.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually can't because SectionLoadHistory::GetCurrentSectionLoadList() needs to be able to create an empty section load for the special stop ID of eStopIDNow if the section load list is empty. I don't thing we want to change section load list to have the load list be mutable. Let me know if you disagree or have another solution?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a way to fix this. We don't need the this function to be const

Copy link
Contributor

@jeffreytan81 jeffreytan81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Pending Jonas's comment and clang-format

@jimingham
Copy link
Collaborator

jimingham commented Oct 22, 2024

This is just naming, but it seems odd having hidden the SectionLoadList in Target, to then have Target::SectionLoadListIsEmpty() as a primary API. "What is this SectionLoadList thing of which you speak???"

Maybe "Target::HasLoadedSections"?

@jimingham
Copy link
Collaborator

jimingham commented Oct 22, 2024

BTW, the general change seems fine to me, none of the clients actually needed to know about the list, they were just asking "can I use load addresses" and "resolve this to a load address for me" so they didn't need to know how that was done.

@clayborg
Copy link
Collaborator Author

Ok, all comments were addressed.


lldb::addr_t GetSectionLoadAddress(const lldb::SectionSP &section_sp);

void ClearSectionLoadList();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like nobody but dynamic loaders should be calling these two. Is there some way to express that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetSectionLoadAddress(...) is called all over the place, but ClearSectionLoadList() is called only by DynamicLoaderMacOS, but it should be able to be called by any dynamic loader, so I wouldn't want to limit the ability to call it. And we can't tie it to a dynamic loader as I could see us wanting to add a command that would call this function for symbolication purposes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

Lots of code around LLDB was directly accessing the target's section load list. This NFC patch makes the section load list private to the Target class can access it, but everyone else now uses accessor functions. This allows us to control the resolving of addresses and will allow for functionality in LLDB which can lazily resolve addresses in JIT plug-ins with a future patch.
- Rename Target::SectionLoadListIsEmpty() to Target::HasLoadedSections() and invert all call sites
- Change Target::HasLoadedSections() to not be const and avoid the const cast.
Now that we are using Target::ResolveLoadAddress, we had an implicit conversion going on that I found that was causing many failures in the test suite. Fixed now.
@clayborg clayborg force-pushed the target-sect-load-list branch from 4edcd99 to 88db720 Compare December 6, 2024 00:50
@clayborg clayborg merged commit c4fb718 into llvm:main Jan 15, 2025
7 checks passed
@clayborg clayborg deleted the target-sect-load-list branch January 15, 2025 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants