[Lldb-commits] [PATCH] D126464: [lldb] Add support to load object files from thin archives

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

A few things to fix but nothing serious. Nice patch! We also need a test for this. I would add a test to lldb/test/API/functionalities/archives/TestBSDArchives.py



================
Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:236-237
     m_object_name_to_index_map.Sort();
   }
+  if (str == ThinArchiveMagic) {
+    Object obj;
----------------



================
Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:261
+    // Extract string table
+    str.assign((const char *)data.GetData(&offset, obj.size), obj.size);
+    obj.Clear();
----------------
No need to make a copy of this data, just make a string ref to it:



================
Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:266
+    do {
+      offset = obj.ExtractFromThin(data, offset, llvm::StringRef(str));
+      if (offset == LLDB_INVALID_OFFSET)
----------------



================
Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:429
                                         length));
+      container_up->m_archive_type = archive_type;
 
----------------
Add "archive_type" as a constructor argument for ObjectContainerBSDArchive instead of setting this afterward.


================
Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:453
         container_up->SetArchive(archive_sp);
+        container_up->m_archive_type = archive_sp->GetArchiveType();
         return container_up.release();
----------------
This shouldn't be needed right? If we already had the container cached, it should have the archive type set correctly already.


================
Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:465
   const char *armag = (const char *)data.PeekData(offset, sizeof(ar_hdr));
   if (armag && ::strncmp(armag, ARMAG, SARMAG) == 0) {
     armag += offsetof(struct ar_hdr, ar_fmag) + SARMAG;
----------------
Now that we are checking two things, we can clean up this code with an early return if "armag" is null:




================
Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:469-471
   }
-  return false;
+  if (armag &&
+      ::strncmp(armag, ThinArchiveMagic, strlen(ThinArchiveMagic)) == 0) {
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126464/new/

https://reviews.llvm.org/D126464