[lldb] [ObjectFileMachO] BSS segments are loadable segments

[jasonmolenda]

ObjectFileMachO::SetLoadAddress sets the address of each segment in a binary in a Target, but it ignores segments that are not loaded in the virtual address space. It was marking segments that were purely BSS -- having no content in the file, but in zero-initialized memory when running in the virtual address space -- as not-loadable, unless they were named "DATA". This works pretty well for typical userland binaries, but in less Darwin environments, there may be BSS segments with other names, that ARE loadable.

I looked at the origin of SectionIsLoadable's check for this, and it was a cleanup by Greg in 2018 where we had three different implementations of the idea in ObjectFileMachO and one of them skipped zero-file-size segments (BSS), which made it into the centralized SectionIsLoadable method.

Also add some logging to the DynamicLoader log channel when loading a binary - it's the first place I look when debugging segment address setting bugs, and it wasn't emitting anything.

rdar://129870649

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

Changes

ObjectFileMachO::SetLoadAddress sets the address of each segment in a binary in a Target, but it ignores segments that are not loaded in the virtual address space. It was marking segments that were purely BSS -- having no content in the file, but in zero-initialized memory when running in the virtual address space -- as not-loadable, unless they were named "DATA". This works pretty well for typical userland binaries, but in less Darwin environments, there may be BSS segments with other names, that ARE loadable.

I looked at the origin of SectionIsLoadable's check for this, and it was a cleanup by Greg in 2018 where we had three different implementations of the idea in ObjectFileMachO and one of them skipped zero-file-size segments (BSS), which made it into the centralized SectionIsLoadable method.

Also add some logging to the DynamicLoader log channel when loading a binary - it's the first place I look when debugging segment address setting bugs, and it wasn't emitting anything.

rdar://129870649

---

Full diff: https://github.com/llvm/llvm-project/pull/96983.diff

1 Files Affected:

• (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (+26-5)

diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 2979bf69bf762..164c4409747e0 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6159,10 +6159,6 @@ Section *ObjectFileMachO::GetMachHeaderSection() {
 bool ObjectFileMachO::SectionIsLoadable(const Section *section) {
   if (!section)
     return false;
-  const bool is_dsym = (m_header.filetype == MH_DSYM);
-  if (section->GetFileSize() == 0 && !is_dsym &&
-      section->GetName() != GetSegmentNameDATA())
-    return false;
   if (section->IsThreadSpecific())
     return false;
   if (GetModule().get() != section->GetModule().get())
@@ -6202,6 +6198,7 @@ lldb::addr_t ObjectFileMachO::CalculateSectionLoadAddressForMemoryImage(
 
 bool ObjectFileMachO::SetLoadAddress(Target &target, lldb::addr_t value,
                                      bool value_is_offset) {
+  Log *log(GetLog(LLDBLog::DynamicLoader));
   ModuleSP module_sp = GetModule();
   if (!module_sp)
     return false;
@@ -6217,17 +6214,37 @@ bool ObjectFileMachO::SetLoadAddress(Target &target, lldb::addr_t value,
   // malformed.
   const bool warn_multiple = true;
 
+  if (log) {
+    std::string binary_description;
+    if (GetFileSpec()) {
+      binary_description += "path='";
+      binary_description += GetFileSpec().GetPath();
+      binary_description += "' ";
+    }
+    if (GetUUID()) {
+      binary_description += "uuid=";
+      binary_description += GetUUID().GetAsString();
+    }
+    LLDB_LOGF(log, "ObjectFileMachO::SetLoadAddress %s",
+              binary_description.c_str());
+  }
   if (value_is_offset) {
     // "value" is an offset to apply to each top level segment
     for (size_t sect_idx = 0; sect_idx < num_sections; ++sect_idx) {
       // Iterate through the object file sections to find all of the
       // sections that size on disk (to avoid __PAGEZERO) and load them
       SectionSP section_sp(section_list->GetSectionAtIndex(sect_idx));
-      if (SectionIsLoadable(section_sp.get()))
+      if (SectionIsLoadable(section_sp.get())) {
+        LLDB_LOGF(log,
+                  "ObjectFileMachO::SetLoadAddress segment '%s' load addr is "
+                  "0x%" PRIx64,
+                  section_sp->GetName().AsCString(),
+                  section_sp->GetFileAddress() + value);
         if (target.GetSectionLoadList().SetSectionLoadAddress(
                 section_sp, section_sp->GetFileAddress() + value,
                 warn_multiple))
           ++num_loaded_sections;
+      }
     }
   } else {
     // "value" is the new base address of the mach_header, adjust each
@@ -6242,6 +6259,10 @@ bool ObjectFileMachO::SetLoadAddress(Target &target, lldb::addr_t value,
             CalculateSectionLoadAddressForMemoryImage(
                 value, mach_header_section, section_sp.get());
         if (section_load_addr != LLDB_INVALID_ADDRESS) {
+          LLDB_LOGF(log,
+                    "ObjectFileMachO::SetLoadAddress segment '%s' load addr is "
+                    "0x%" PRIx64,
+                    section_sp->GetName().AsCString(), section_load_addr);
           if (target.GetSectionLoadList().SetSectionLoadAddress(
                   section_sp, section_load_addr, warn_multiple))
             ++num_loaded_sections;

[jasonmolenda]

I was about to add a macosx/ API test to compile a program with only BSS data in the DATA segment, and I noticed I'd written TestBSSOnlyDataSectionSliding.py last year when I opted sections named "DATA" out of this "don't set a load address for BSS-only segments" behavior in ObjectFileMachO. This existing test is already covering the change I'm making in this PR.

[JDevlieghere]

LGTM!