Use dSYM SymbolFile Sections file addr instead of ObjectFile Section file addr when they differ in ObjectFileMachO

[jasonmolenda]

Summary

This is addressing a regression introduced by Greg's change in https://reviews.llvm.org/D86122 . His change was fine, but the original code was written unclearly as to what it was doing.

When a binary is in the system's "shared cache", the file addresses will be different from the dSYM's file addresses. The binary (both binary file & dSYM) usually start with a file address of 0, but when the binary is integrated into the "shared cache" of all common binaries, the segments are rearranged and now the TEXT segment has a non-0 file address. This is pretty unusual normally -- when the UUID matches for a binary and its dSYMs, they should have identical file addresses for their segments, for example.

When we add a dSYM SymbolFile to an ObjectFile that is in the shared cache, we're going to use the symbol table and debug information from the dSYM (SymbolFile) -- we need the Sections to have file addresses that match the symbol table and debug info in the dSYM. In other words, the dSYM's file addresses must win.

This is handled in ObjectFileMachO::ProcessSegmentCommand. This was previously behind a check of "does this section have a valid base address" or something that didn't make sense. Greg noticed this and changed it to "Is this ObjectFile a memory image". This seems like the right thing to do but "IsInMemory()" doesn't apply to the way we create shared cache binaries when lldb and the inferior are using the same shared cache. lldb thinks of these as the same as an on-disk ObjectFile that we've mmap'ed into our space, except it's contents are represented by a DataBufferUnowned. With this "file addr fixup" code behind a "IsInMemory()" check, now we weren't fixing the file addresses and none of the debug info was ever found because the fileaddrs were contained by any of the Sections.

I shouldn't intermix changes, but I was also annoyed by how many places we check if a binary is in the shared cache by checking flags in ObjectFileMachO, and put it in a little method.

[jasonmolenda]

I should note two more things. First, I make this code which was previously checking if the ObjectFile was a memory image unconditional if the dSYM's segment file address differs from the ObjectFile's segment file address. I can't see how this is possible in any case but a shared cache -- so I could check that the ObjectFile is in the shared cache -- but however we got to this point, the only correct thing to do is to use the dSYM (SymbolFile)'s file addresses, it's immaterial.

Second, man, I can't think of a way to test this. You need to be adding a dSYM to a binary in the system's shared cache on a darwin system. We have access to the dSYMs for system binaries inside Apple, but we can't use those for automated testing in any way, and which dSYM you need depends on the system. This is a drag because this regression has snuck past us at least once in the past too, looking over the history of the code.

[JDevlieghere]

Nice catch, LGTM but I'll defer to Greg to accept.

[clayborg]

So do the shared library files on disk that are exploded from a shared cache of an iOS device have the file addresses set to where they are in the shared cache? I am guessing this must the issue that you were running into where the module was not in memory by still it needed to have its file addresses adjusted.

Besides some indentation, and possibly logging when we modify the file address, this LGTM.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

1691–1692

This seems like a good thing to log to a log channel maybe?

1691–1714

unindent this code? Or is phabricator just not showing that this was unindented?

[jasonmolenda]

In D108889#2973300, @clayborg wrote:

So do the shared library files on disk that are exploded from a shared cache of an iOS device have the file addresses set to where they are in the shared cache? I am guessing this must the issue that you were running into where the module was not in memory by still it needed to have its file addresses adjusted.

Yes, when a shared cache is expanded into discrete binaries, they still have the file addresses from the shared cache. So a dylib starts with file address based at 0, then it gets a new file address when incorporated into the shared cache, and then when expanded out of the shared cache, it retains the non-zero base file address.

But also on macOS now, it behaves like iOS where the binaries don't exist on the filesystem any more, they only exist in the shared cache blob / in memory. When the lldb process & the inferior process are using the same shared cache (99% of the time this is true), lldb creates ObjectFiles in its own address space for the shared cache binaries. That's where this bug was especially hitting.

Adding a log message here is a good idea.

Besides some indentation, and possibly logging when we modify the file address, this LGTM.

[jasonmolenda]

Update patch to incorporate Greg's suggestion.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

 //===-- ObjectFileMachO.h ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_MACH_O_OBJECTFILEMACHO_H
 #define LLDB_SOURCE_PLUGINS_OBJECTFILE_MACH_O_OBJECTFILEMACHO_H
 
 #include "lldb/Core/Address.h"
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Host/SafeMachO.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/UUID.h"
 
bool SetLoadAddress(lldb_private::Target &target, lldb::addr_t value,
                       bool value_is_offset) override;
 
   lldb::ByteOrder GetByteOrder() const override;
 
   bool IsExecutable() const override;
 
   bool IsDynamicLoader() const;
 
+  bool IsSharedCacheBinary() const;
+
   uint32_t GetAddressByteSize() const override;
 
   lldb_private::AddressClass GetAddressClass(lldb::addr_t file_addr) override;
 
   lldb_private::Symtab *GetSymtab() override;
 
   bool IsStripped() override;
 

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

 bool ObjectFileMachO::IsExecutable() const {
   return m_header.filetype == MH_EXECUTE;
 }
 
 bool ObjectFileMachO::IsDynamicLoader() const {
   return m_header.filetype == MH_DYLINKER;
 }
 
+bool ObjectFileMachO::IsSharedCacheBinary() const {
+  return m_header.flags & MH_DYLIB_IN_CACHE;
+}
+
 uint32_t ObjectFileMachO::GetAddressByteSize() const {
   return m_data.GetAddressByteSize();
 }
 
 AddressClass ObjectFileMachO::GetAddressClass(lldb::addr_t file_addr) {
   Symtab *symtab = GetSymtab();
   if (!symtab)
     return AddressClass::eUnknown;

   return result;
 }
 
 void ObjectFileMachO::SanitizeSegmentCommand(
     llvm::MachO::segment_command_64 &seg_cmd, uint32_t cmd_idx) {
   if (m_length == 0 || seg_cmd.filesize == 0)
     return;
 
-  if ((m_header.flags & MH_DYLIB_IN_CACHE) && !IsInMemory()) {
+  if (IsSharedCacheBinary() && !IsInMemory()) {
     // In shared cache images, the load commands are relative to the
     // shared cache file, and not the specific image we are
     // examining. Let's fix this up so that it looks like a normal
     // image.
     if (strncmp(seg_cmd.segname, "__TEXT", sizeof(seg_cmd.segname)) == 0)
       m_text_address = seg_cmd.vmaddr;
     if (strncmp(seg_cmd.segname, "__LINKEDIT", sizeof(seg_cmd.segname)) == 0)
       m_linkedit_original_offset = seg_cmd.fileoff;

         load_cmd.flags); // Flags for this section
 
     segment_sp->SetIsEncrypted(segment_is_encrypted);
     m_sections_up->AddSection(segment_sp);
     segment_sp->SetPermissions(segment_permissions);
     if (add_to_unified)
       context.UnifiedList.AddSection(segment_sp);
   } else if (unified_section_sp) {
+    // If this is a dSYM and the file addresses in the dSYM differ from the
+    // file addresses in the ObjectFile, we must use the file base address for
+    // the Section from the dSYM for the DWARF to resolve correctly.
+    // This only happens with binaries in the shared cache in practice;
+    // normally a mismatch like this would give a binary & dSYM that do not
+    // match UUIDs. When a binary is included in the shared cache, its
+    // segments are rearranged to optimize the shared cache, so its file
+    // addresses will differ from what the ObjectFile had originally,
+    // and what the dSYM has.
     if (is_dsym && unified_section_sp->GetFileAddress() != load_cmd.vmaddr) {
-      // Check to see if the module was read from memory?
-      if (module_sp->GetObjectFile()->IsInMemory()) {
-        // We have a module that is in memory and needs to have its file
-        // address adjusted. We need to do this because when we load a file
-        // from memory, its addresses will be slid already, yet the addresses
-        // in the new symbol file will still be unslid.  Since everything is
-        // stored as section offset, this shouldn't cause any problems.
-
-        // Make sure we've parsed the symbol table from the ObjectFile before
-        // we go around changing its Sections.
-        module_sp->GetObjectFile()->GetSymtab();
-        // eh_frame would present the same problems but we parse that on a per-
-        // function basis as-needed so it's more difficult to remove its use of
-        // the Sections.  Realistically, the environments where this code path
-        // will be taken will not have eh_frame sections.
-
-        unified_section_sp->SetFileAddress(load_cmd.vmaddr);
-
-        // Notify the module that the section addresses have been changed once
-        // we're done so any file-address caches can be updated.
-        context.FileAddressesChanged = true;
+      Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_SYMBOLS));
+      if (log) {
+        log->Printf(
+            "Installing dSYM's %s segment file address over ObjectFile's "
+            "so symbol table/debug info resolves correctly for %s",
+            const_segname.AsCString(),
+            module_sp->GetFileSpec().GetFilename().AsCString());
       }
+
+      // Make sure we've parsed the symbol table from the ObjectFile before
+      // we go around changing its Sections.
+      module_sp->GetObjectFile()->GetSymtab();
+      // eh_frame would present the same problems but we parse that on a per-
+      // function basis as-needed so it's more difficult to remove its use of
+      // the Sections.  Realistically, the environments where this code path
+      // will be taken will not have eh_frame sections.
+
+      unified_section_sp->SetFileAddress(load_cmd.vmaddr);
+
+      // Notify the module that the section addresses have been changed once
+      // we're done so any file-address caches can be updated.
+      context.FileAddressesChanged = true;
     }
     m_sections_up->AddSection(unified_section_sp);
   }
 
   llvm::MachO::section_64 sect64;
   ::memset(&sect64, 0, sizeof(sect64));
   // Push a section into our mach sections for the section at index zero
   // (NO_SECT) if we don't have any mach sections yet...
   if (m_mach_sections.empty())

                      sizeof(sect64.segname)) == nullptr)
       break;
     sect64.addr = m_data.GetAddress(&offset);
     sect64.size = m_data.GetAddress(&offset);
 
     if (m_data.GetU32(&offset, &sect64.offset, num_u32s) == nullptr)
       break;
 
-    if ((m_header.flags & MH_DYLIB_IN_CACHE) && !IsInMemory()) {
+    if (IsSharedCacheBinary() && !IsInMemory()) {
       sect64.offset = sect64.addr - m_text_address;
     }
 
     // Keep a list of mach sections around in case we need to get at data that
     // isn't stored in the abstracted Sections.
     m_mach_sections.push_back(sect64);
 
     if (add_section) {

       symtab_load_command.nsyms * nlist_byte_size;
   const addr_t strtab_data_byte_size = symtab_load_command.strsize;
   addr_t strtab_addr = LLDB_INVALID_ADDRESS;
 
   ProcessSP process_sp(m_process_wp.lock());
   Process *process = process_sp.get();
 
   uint32_t memory_module_load_level = eMemoryModuleLoadLevelComplete;
-  bool is_shared_cache_image = m_header.flags & MH_DYLIB_IN_CACHE;
+  bool is_shared_cache_image = IsSharedCacheBinary();
   bool is_local_shared_cache_image = is_shared_cache_image && !IsInMemory();
   SectionSP linkedit_section_sp(
       section_list->FindSectionByName(GetSegmentNameLINKEDIT()));
 
   if (process && m_header.filetype != llvm::MachO::MH_OBJECT &&
       !is_local_shared_cache_image) {
     Target &target = process->GetTarget();
 

   // the DSC. The symbol information has all been retained, but it isn't
   // available in the normal nlist data. However, there *are* duplicate
   // entries of *some*
   // LOCAL symbols in the normal nlist data. To handle this situation
   // correctly, we must first attempt
   // to parse any DSC unmapped symbol information. If we find any, we set a
   // flag that tells the normal nlist parser to ignore all LOCAL symbols.
 
-  if (m_header.flags & MH_DYLIB_IN_CACHE) {
+  if (IsSharedCacheBinary()) {
     // Before we can start mapping the DSC, we need to make certain the
     // target process is actually using the cache we can find.
 
     // Next we need to determine the correct path for the dyld shared cache.
 
     ArchSpec header_arch = GetArchitecture();
     char dsc_path[PATH_MAX];
     char dsc_path_development[PATH_MAX];