Switch over to using the LLVM archive parser for BSD archives.

[clayborg]

Summary

Our LLDB parser didn't correctly handle archives of all flavors on different systems, it currently only correctly handled BSD archives, normal and thin, on macOS, but I noticed that it was getting incorrect information when decoding a variety of archives on linux. There were subtle changes to how names were encoded that we didn't handle correctly and we also didn't set the result of GetObjectSize() correctly as there was some bad math. This didn't matter when exracting .o files from .a files for LLDB because the size was always way too big, but it was big enough to at least read enough bytes for each object within the archive.

This patch does the following:

• switch over to use LLVM's archive parser and avoids previous code duplication
• remove values from ObjectContainerBSDArchive::Object that we don't use like:
  • uid
  • gid
  • mode
• fix ths ObjectContainerBSDArchive::Object::file_size value to be correct
• adds tests to test that we get the correct module specifications

[JDevlieghere]

Overall this looks good, but instead of consuming all the errors, let's log them with LLDB_LOG_ERROR. Note that the API is a bit tricky and still requires you to provide a format string:

LLDB_LOG_ERROR(GetLog(LLDBLog::Object), something.takeError(), "context before printing the actual error: {0}");

[clayborg]

Log any errors we encounter during archive object parsing.

[JDevlieghere]

LGTM!

lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.h

struct Object {
     lldb::offset_t Extract(const lldb_private::DataExtractor &data,
                            lldb::offset_t offset);
     /// Object name in the archive.
     lldb_private::ConstString ar_name;
 
     /// Object modification time in the archive.
     uint32_t modification_time = 0;
 
-    /// Object user id in the archive.
-    uint16_t uid = 0;
-
-    /// Object group id in the archive.
-    uint16_t gid = 0;
-
-    /// Object octal file permissions in the archive.
-    uint16_t mode = 0;
-
     /// Object size in bytes in the archive.
     uint32_t size = 0;
 
     /// File offset in bytes from the beginning of the file of the object data.
     lldb::offset_t file_offset = 0;
 
     /// Length of the object data.
     lldb::offset_t file_size = 0;
+
+    void Dump() const;
   };
 
   class Archive {
   public:
     typedef std::shared_ptr<Archive> shared_ptr;
     typedef std::multimap<lldb_private::FileSpec, shared_ptr> Map;
 
     Archive(const lldb_private::ArchSpec &arch,

lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp

 //===-- ObjectContainerBSDArchive.cpp -------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "ObjectContainerBSDArchive.h"
 
 #if defined(_WIN32) || defined(__ANDROID__)
 // Defines from ar, missing on Windows
-#define ARMAG "!<arch>\n"
 #define SARMAG 8
 #define ARFMAG "`\n"
 
 typedef struct ar_hdr {
   char ar_name[16];
   char ar_date[12];
   char ar_uid[6], ar_gid[6];
   char ar_mode[8];
   char ar_size[10];
   char ar_fmag[2];
 } ar_hdr;
 #else
 #include <ar.h>
 #endif
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
 
+#include "llvm/Object/Archive.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 using namespace llvm::object;
 
 LLDB_PLUGIN_DEFINE(ObjectContainerBSDArchive)
 
 ObjectContainerBSDArchive::Object::Object() : ar_name() {}
 
 void ObjectContainerBSDArchive::Object::Clear() {
   ar_name.Clear();
   modification_time = 0;
-  uid = 0;
-  gid = 0;
-  mode = 0;
   size = 0;
   file_offset = 0;
   file_size = 0;
 }
 
-lldb::offset_t ObjectContainerBSDArchive::Object::ExtractFromThin(
-    const DataExtractor &data, lldb::offset_t offset,
-    llvm::StringRef stringTable) {
-  size_t ar_name_len = 0;
-  std::string str;
-  char *err;
-
-  // File header
-  //
-  // The common format is as follows.
-  //
-  //  Offset  Length  Name            Format
-  //  0       16      File name       ASCII right padded with spaces (no spaces
-  //  allowed in file name)
-  //  16      12      File mod        Decimal as cstring right padded with
-  //  spaces
-  //  28      6       Owner ID        Decimal as cstring right padded with
-  //  spaces
-  //  34      6       Group ID        Decimal as cstring right padded with
-  //  spaces
-  //  40      8       File mode       Octal   as cstring right padded with
-  //  spaces
-  //  48      10      File byte size  Decimal as cstring right padded with
-  //  spaces
-  //  58      2       File magic      0x60 0x0A
-
-  // Make sure there is enough data for the file header and bail if not
-  if (!data.ValidOffsetForDataOfSize(offset, 60))
-    return LLDB_INVALID_OFFSET;
-
-  str.assign((const char *)data.GetData(&offset, 16), 16);
-  if (!(llvm::StringRef(str).startswith("//") || stringTable.empty())) {
-    // Strip off any trailing spaces.
-    const size_t last_pos = str.find_last_not_of(' ');
-    if (last_pos != std::string::npos) {
-      if (last_pos + 1 < 16)
-        str.erase(last_pos + 1);
-    }
-    int start = strtoul(str.c_str() + 1, &err, 10);
-    int end = stringTable.find('\n', start);
-    str.assign(stringTable.data() + start, end - start - 1);
-    ar_name.SetCString(str.c_str());
-  }
-
-  str.assign((const char *)data.GetData(&offset, 12), 12);
-  modification_time = strtoul(str.c_str(), &err, 10);
-
-  str.assign((const char *)data.GetData(&offset, 6), 6);
-  uid = strtoul(str.c_str(), &err, 10);
-
-  str.assign((const char *)data.GetData(&offset, 6), 6);
-  gid = strtoul(str.c_str(), &err, 10);
-
-  str.assign((const char *)data.GetData(&offset, 8), 8);
-  mode = strtoul(str.c_str(), &err, 8);
-
-  str.assign((const char *)data.GetData(&offset, 10), 10);
-  size = strtoul(str.c_str(), &err, 10);
-
-  str.assign((const char *)data.GetData(&offset, 2), 2);
-  if (str == ARFMAG) {
-    file_offset = offset;
-    file_size = size - ar_name_len;
-    return offset;
-  }
-  return LLDB_INVALID_OFFSET;
-}
-
-lldb::offset_t
-ObjectContainerBSDArchive::Object::Extract(const DataExtractor &data,
-                                           lldb::offset_t offset) {
-  size_t ar_name_len = 0;
-  std::string str;
-  char *err;
-
-  // File header
-  //
-  // The common format is as follows.
-  //
-  //  Offset  Length  Name            Format
-  //  0       16      File name       ASCII right padded with spaces (no spaces
-  //  allowed in file name)
-  //  16      12      File mod        Decimal as cstring right padded with
-  //  spaces
-  //  28      6       Owner ID        Decimal as cstring right padded with
-  //  spaces
-  //  34      6       Group ID        Decimal as cstring right padded with
-  //  spaces
-  //  40      8       File mode       Octal   as cstring right padded with
-  //  spaces
-  //  48      10      File byte size  Decimal as cstring right padded with
-  //  spaces
-  //  58      2       File magic      0x60 0x0A
-
-  // Make sure there is enough data for the file header and bail if not
-  if (!data.ValidOffsetForDataOfSize(offset, 60))
-    return LLDB_INVALID_OFFSET;
-
-  str.assign((const char *)data.GetData(&offset, 16), 16);
-  if (llvm::StringRef(str).startswith("#1/")) {
-    // If the name is longer than 16 bytes, or contains an embedded space then
-    // it will use this format where the length of the name is here and the
-    // name characters are after this header.
-    ar_name_len = strtoul(str.c_str() + 3, &err, 10);
-  } else {
-    // Strip off any trailing spaces.
-    const size_t last_pos = str.find_last_not_of(' ');
-    if (last_pos != std::string::npos) {
-      if (last_pos + 1 < 16)
-        str.erase(last_pos + 1);
-    }
-    ar_name.SetCString(str.c_str());
-  }
-
-  str.assign((const char *)data.GetData(&offset, 12), 12);
-  modification_time = strtoul(str.c_str(), &err, 10);
-
-  str.assign((const char *)data.GetData(&offset, 6), 6);
-  uid = strtoul(str.c_str(), &err, 10);
-
-  str.assign((const char *)data.GetData(&offset, 6), 6);
-  gid = strtoul(str.c_str(), &err, 10);
-
-  str.assign((const char *)data.GetData(&offset, 8), 8);
-  mode = strtoul(str.c_str(), &err, 8);
-
-  str.assign((const char *)data.GetData(&offset, 10), 10);
-  size = strtoul(str.c_str(), &err, 10);
-
-  str.assign((const char *)data.GetData(&offset, 2), 2);
-  if (str == ARFMAG) {
-    if (ar_name_len > 0) {
-      const void *ar_name_ptr = data.GetData(&offset, ar_name_len);
-      // Make sure there was enough data for the string value and bail if not
-      if (ar_name_ptr == nullptr)
-        return LLDB_INVALID_OFFSET;
-      str.assign((const char *)ar_name_ptr, ar_name_len);
-      ar_name.SetCString(str.c_str());
-    }
-    file_offset = offset;
-    file_size = size - ar_name_len;
-    return offset;
-  }
-  return LLDB_INVALID_OFFSET;
+void ObjectContainerBSDArchive::Object::Dump() const {
+  printf("name        = \"%s\"\n", ar_name.GetCString());
+  printf("mtime       = 0x%8.8" PRIx32 "\n", modification_time);
+  printf("size        = 0x%8.8" PRIx32 " (%" PRIu32 ")\n", size, size);
+  printf("file_offset = 0x%16.16" PRIx64 " (%" PRIu64 ")\n", file_offset,
+         file_offset);
+  printf("file_size   = 0x%16.16" PRIx64 " (%" PRIu64 ")\n\n", file_size,
+         file_size);
 }
 
 ObjectContainerBSDArchive::Archive::Archive(const lldb_private::ArchSpec &arch,
                                             const llvm::sys::TimePoint<> &time,
                                             lldb::offset_t file_offset,
                                             lldb_private::DataExtractor &data,
                                             ArchiveType archive_type)
     : m_arch(arch), m_modification_time(time), m_file_offset(file_offset),
       m_objects(), m_data(data), m_archive_type(archive_type) {}
 
+Log *l = GetLog(LLDBLog::Object);
 ObjectContainerBSDArchive::Archive::~Archive() = default;
 
 size_t ObjectContainerBSDArchive::Archive::ParseObjects() {
   DataExtractor &data = m_data;
-  std::string str;
-  lldb::offset_t offset = 0;
-  str.assign((const char *)data.GetData(&offset, SARMAG), SARMAG);
-  if (str == ARMAG) {
-    Object obj;
-    do {
-      offset = obj.Extract(data, offset);
-      if (offset == LLDB_INVALID_OFFSET)
-        break;
-      size_t obj_idx = m_objects.size();
-      m_objects.push_back(obj);
-      // Insert all of the C strings out of order for now...
-      m_object_name_to_index_map.Append(obj.ar_name, obj_idx);
-      offset += obj.file_size;
-      obj.Clear();
-    } while (data.ValidOffset(offset));
+
+  std::unique_ptr<llvm::MemoryBuffer> mem_buffer =
+      llvm::MemoryBuffer::getMemBuffer(
+            llvm::StringRef((const char *)data.GetDataStart(),
+                            data.GetByteSize()),
+            llvm::StringRef(),
+            /*RequiresNullTerminator=*/false);
+
+  auto exp_ar = llvm::object::Archive::create(mem_buffer->getMemBufferRef());
+  if (!exp_ar) {
+    LLDB_LOG_ERROR(l, exp_ar.takeError(), "failed to create archive: {0}");
+    return 0;
+  }
+  auto llvm_archive = std::move(exp_ar.get());
+
+  llvm::Error iter_err = llvm::Error::success();
+  for (const auto &child: llvm_archive->children(iter_err)) {
+    auto exp_name = child.getName();
+    if (exp_name) {
+      obj.ar_name = ConstString(exp_name.get());
+    } else {
+      LLDB_LOG_ERROR(l, exp_name.takeError(),
+                     "failed to get archive object name: {0}");
+      continue;
+    }
 
-    // Now sort all of the object name pointers
-    m_object_name_to_index_map.Sort();
-  } else if (str == ThinArchiveMagic) {
-    Object obj;
-    size_t obj_idx;
+    auto exp_mtime = child.getLastModified();
+    if (exp_mtime) {
+      obj.modification_time =
+          std::chrono::duration_cast<std::chrono::seconds>(
+              std::chrono::time_point_cast<std::chrono::seconds>(
+                    exp_mtime.get()).time_since_epoch()).count();
+    } else {
+      LLDB_LOG_ERROR(l, exp_mtime.takeError(),
+                     "failed to get archive object time: {0}");
+      continue;
+    }
 
-    // Retrieve symbol table
-    offset = obj.ExtractFromThin(data, offset, "");
-    if (offset == LLDB_INVALID_OFFSET)
-      return m_objects.size();
-    obj_idx = m_objects.size();
-    m_objects.push_back(obj);
-    // Insert all of the C strings out of order for now...
-    m_object_name_to_index_map.Append(obj.ar_name, obj_idx);
-    offset += obj.file_size;
-    obj.Clear();
+    auto exp_size = child.getRawSize();
+    if (exp_size) {
+      obj.size = exp_size.get();
+    } else {
+      LLDB_LOG_ERROR(l, exp_size.takeError(),
+                     "failed to get archive object size: {0}");
+      continue;
+    }
 
-    // Retrieve string table
-    offset = obj.ExtractFromThin(data, offset, "");
-    if (offset == LLDB_INVALID_OFFSET)
-      return m_objects.size();
-    obj_idx = m_objects.size();
-    m_objects.push_back(obj);
-    // Insert all of the C strings out of order for now...
-    m_object_name_to_index_map.Append(obj.ar_name, obj_idx);
-    // Extract string table
-    llvm::StringRef strtab((const char *)data.GetData(&offset, obj.size),
-                           obj.size);
-    obj.Clear();
+    obj.file_offset = child.getDataOffset();
 
-    // Retrieve object files
-    do {
-      offset = obj.ExtractFromThin(data, offset, strtab);
-      if (offset == LLDB_INVALID_OFFSET)
-        break;
-      obj_idx = m_objects.size();
-      m_objects.push_back(obj);
-      // Insert all of the C strings out of order for now...
-      m_object_name_to_index_map.Append(obj.ar_name, obj_idx);
-      obj.Clear();
-    } while (data.ValidOffset(offset));
-
-    // Now sort all of the object name pointers
-    m_object_name_to_index_map.Sort();
-  }
+    auto exp_file_size = child.getSize();
+    if (exp_file_size) {
+      obj.file_size = exp_file_size.get();
+    } else {
+      LLDB_LOG_ERROR(l, exp_file_size.takeError(),
+                     "failed to get archive object file size: {0}");
+      continue;
+    }
+    m_object_name_to_index_map.Append(obj.ar_name, m_objects.size());
+  }
+  if (iter_err) {
+    LLDB_LOG_ERROR(l, std::move(iter_err),
+                   "failed to iterate over archive objects: {0}");
+  }
   return m_objects.size();
 }
 
 ObjectContainerBSDArchive::Object *
 ObjectContainerBSDArchive::Archive::FindObject(
     ConstString object_name, const llvm::sys::TimePoint<> &object_mod_time) {
   const ObjectNameToIndexMap::Entry *match =
       m_object_name_to_index_map.FindFirstValueForName(object_name);
if (data_sp) {
     }
   }
   return nullptr;
 }
 
 ArchiveType
 ObjectContainerBSDArchive::MagicBytesMatch(const DataExtractor &data) {
   uint32_t offset = 0;
-  const char *armag = (const char *)data.PeekData(offset, sizeof(ar_hdr));
+  const char *armag = (const char *)data.PeekData(offset,
+                                                  sizeof(ar_hdr) + SARMAG);
   if (armag == nullptr)
     return ArchiveType::Invalid;
-  if (::strncmp(armag, ARMAG, SARMAG) == 0) {
-    armag += offsetof(struct ar_hdr, ar_fmag) + SARMAG;
-    if (strncmp(armag, ARFMAG, 2) == 0)
-      return ArchiveType::Archive;
-  } else if (::strncmp(armag, ThinArchiveMagic, strlen(ThinArchiveMagic)) ==
-             0) {
-    armag += offsetof(struct ar_hdr, ar_fmag) + strlen(ThinArchiveMagic);
-    if (strncmp(armag, ARFMAG, 2) == 0) {
-      return ArchiveType::ThinArchive;
-    }
-  }
+  ArchiveType result = ArchiveType::Invalid;
+  if (strncmp(armag, ArchiveMagic, SARMAG) == 0)
+      result = ArchiveType::Archive;
+  else if (strncmp(armag, ThinArchiveMagic, SARMAG) == 0)
+      result = ArchiveType::ThinArchive;
+  else
+      return ArchiveType::Invalid;
+
+      return result;
   return ArchiveType::Invalid;
 }
 
 ObjectContainerBSDArchive::ObjectContainerBSDArchive(
     const lldb::ModuleSP &module_sp, DataBufferSP &data_sp,
     lldb::offset_t data_offset, const lldb_private::FileSpec *file,
     lldb::offset_t file_offset, lldb::offset_t size, ArchiveType archive_type)
     : ObjectContainer(module_sp, file, file_offset, size, data_sp, data_offset),
for (size_t idx = 0; idx < num_objects; ++idx) {
                   file, object_file_offset, file_size - object_file_offset,
                   specs)) {
             ModuleSpec &spec =
                 specs.GetModuleSpecRefAtIndex(specs.GetSize() - 1);
             llvm::sys::TimePoint<> object_mod_time(
                 std::chrono::seconds(object->modification_time));
             spec.GetObjectName() = object->ar_name;
             spec.SetObjectOffset(object_file_offset);
-            spec.SetObjectSize(file_size - object_file_offset);
+            spec.SetObjectSize(object->file_size);
             spec.GetObjectModificationTime() = object_mod_time;
           }
         }
       }
     }
   }
   const size_t end_count = specs.GetSize();
   size_t num_specs_added = end_count - initial_count;

lldb/test/API/functionalities/archives/TestBSDArchives.py

def test(self):
             STOPPED_DUE_TO_BREAKPOINT,
             substrs=["stopped", "stop reason = breakpoint"],
         )
         self.expect(
             "frame variable", VARIABLES_DISPLAYED_CORRECTLY, substrs=["(int) arg = 2"]
         )
         self.expect_var_path("__b_global", type="int", value="2")
 
-        # Test loading thin archives
-        archive_path = self.getBuildArtifact("libbar.a")
-        module_specs = lldb.SBModuleSpecList.GetModuleSpecifications(archive_path)
-        num_specs = module_specs.GetSize()
-        self.assertEqual(num_specs, 1)
-        self.assertEqual(module_specs.GetSpecAtIndex(0).GetObjectName(), "c.o")
 
     def check_frame_variable_errors(self, thread, error_strings):
         command_result = lldb.SBCommandReturnObject()
         interp = self.dbg.GetCommandInterpreter()
         result = interp.HandleCommand("frame variable", command_result)
         self.assertEqual(
             result, lldb.eReturnStatusFailed, "frame var succeeded unexpectedly"
         )
def test_frame_var_errors_when_archive_missing(self):
 
         error_strings = [
             'debug map object file "',
             'libfoo.a(a.o)" containing debug info does not exist, debug info will not be loaded',
         ]
         self.check_frame_variable_errors(thread, error_strings)
 
     @skipIfRemote
+    def test_archive_specifications(self):
+        """
+        Create archives and make sure the information we get when retrieving
+        the modules specifications is correct.
+        """
+        self.build()
+        libbar_path = self.getBuildArtifact("libbar.a")
+        libfoo_path = self.getBuildArtifact("libfoo.a")
+        libfoothin_path = self.getBuildArtifact("libfoo-thin.a")
+        objfile_a = self.getBuildArtifact("a.o")
+        objfile_b = self.getBuildArtifact("b.o")
+        objfile_c = self.getBuildArtifact("c.o")
+        size_a = os.path.getsize(objfile_a)
+        size_b = os.path.getsize(objfile_b)
+        size_c = os.path.getsize(objfile_c)
+
+        # Test loading normal archives
+        module_specs = lldb.SBModuleSpecList.GetModuleSpecifications(libfoo_path)
+        num_specs = module_specs.GetSize()
+        self.assertEqual(num_specs, 2)
+        spec = module_specs.GetSpecAtIndex(0)
+        self.assertEqual(spec.GetObjectName(), "a.o")
+        self.assertEqual(spec.GetObjectSize(), size_a)
+        spec = module_specs.GetSpecAtIndex(1)
+        self.assertEqual(spec.GetObjectName(), "b.o")
+        self.assertEqual(spec.GetObjectSize(), size_b)
+
+        # Test loading thin archives
+        module_specs = lldb.SBModuleSpecList.GetModuleSpecifications(libbar_path)
+        num_specs = module_specs.GetSize()
+        self.assertEqual(num_specs, 1)
+        spec = module_specs.GetSpecAtIndex(0)
+        self.assertEqual(spec.GetObjectName(), "c.o")
+        self.assertEqual(spec.GetObjectSize(), size_c)
+
+        module_specs = lldb.SBModuleSpecList.GetModuleSpecifications(libfoothin_path)
+        num_specs = module_specs.GetSize()
+        self.assertEqual(num_specs, 2)
+        spec = module_specs.GetSpecAtIndex(0)
+        self.assertEqual(spec.GetObjectName(), "a.o")
+        self.assertEqual(spec.GetObjectSize(), size_a)
+        spec = module_specs.GetSpecAtIndex(1)
+        self.assertEqual(spec.GetObjectName(), "b.o")
+        self.assertEqual(spec.GetObjectSize(), size_b, libfoothin_path)
+
+
+    @skipIfRemote
     @skipUnlessDarwin
     def test_frame_var_errors_when_thin_archive_malformed(self):
         """
         Create thin archive libfoo.a and make it malformed to make sure
         we don't crash and report an appropriate error when resolving
         breakpoint using debug map.
         """
         self.build()
         exe = self.getBuildArtifact("a.out")
         libfoo_path = self.getBuildArtifact("libfoo.a")
         libthin_path = self.getBuildArtifact("libfoo-thin.a")
         objfile_a = self.getBuildArtifact("a.o")
+        objfile_b = self.getBuildArtifact("b.o")
+        objfile_c = self.getBuildArtifact("c.o")
         # Replace the libfoo.a file with a thin archive containing the same
         # debug information (a.o, b.o). Then remove a.o from the file system
         # so we force an error when we set a breakpoint on a() function.
         # Since the a.o is missing, the debug info won't be loaded and we
         # should see an error when trying to break into a().
         os.remove(libfoo_path)
         shutil.copyfile(libthin_path, libfoo_path)
         os.remove(objfile_a)

lldb/test/API/functionalities/archives/b.c

 static int __b_global = 2;
+char __extra[4096]; // Make sure sizeof b.o differs from a.o and c.o
 
 int b(int arg) {
     int result = arg + __b_global;
     return result;
 }
 
 int bb(int arg1) {
     int result2 = arg1 - __b_global;
     return result2;
 }

lldb/test/API/functionalities/archives/c.c

 static int __c_global = 3;
-
+char __extra[4096]; // Make sure sizeof b.o differs from a.o and c.o
+char __extra2[4096]; // Make sure sizeof b.o differs from a.o and c.o
 int c(int arg) {
   int result = arg + __c_global;
   return result;
 }
 
 int cc(int arg1) {
   int result3 = arg1 - __c_global;
   return result3;
 }