[PATCH] D83023: [lldb/ObjectFileMachO] Fetch shared cache images from our own shared cache

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I think this is a very interesting feature (lldb being able to load modules from memory; the mac shared cache thingy is interesting too, but in a different way). We have a feature request from people who are downloading modules from a network (from a proprietary symbol server, etc.) and would like to pass them to lldb without having to serialize them to disk. This would be a step towards making that happen. It could also be useful for our own unit tests which now have to do a similar thing.

However, I think this could use some clean up. There's a lot of juggling of data/file/object offsets going on, and it all seems inconsistent and avoidable to me. Please see inline comments for details.

The patch is also quite light on testing. If done right, I believe this should make it possible to yaml2obj a file into memory in a unit test and then create a Module from that. That would enable us to test the Module(Spec) changes in isolation, and move them to a separate patch.

In D83023#2127232 <https://reviews.llvm.org/D83023#2127232>, @friss wrote:

>   I don't suppose you care a lot about `ObjectFileMachO.cpp`


I care about ObjectFileMachO to the extent that I need to occasionally touch it when working on generic interfaces. And I gotta say that changing anything in there is pretty hard right now... :/



================
Comment at: lldb/include/lldb/Host/macosx/HostInfoMacOSX.h:22-43
+struct SharedCacheImageInfo {
+  lldb::offset_t unslidTextOffset;
+  UUID uuid;
+};
+
+class SharedCacheInfo {
+public:
----------------
The way we've done this elsewhere is to add the interface to the base class with a default stubbed-out implementation. That way, you don't have to put `#ifdef __APPLE__` into all of the code which tries to use this. `HostInfo::GetXcodeSDKPath` is the latest example of that.


================
Comment at: lldb/include/lldb/Host/macosx/HostInfoMacOSX.h:38
+  UUID m_uuid;
+  lldb::DataBufferSP m_data;
+
----------------
I think this could just be an ArrayRef<uint8_t>, or a void* or something, and then you could create an appropriately sized DataBufferHostMemory (or whatever ends up called) when working with a specific module.


================
Comment at: lldb/include/lldb/Utility/DataBuffer.h:82
 
+class DataBufferHostMemory : public DataBuffer {
+public:
----------------
All of our other DataBuffers also point to host memory (how could they not do that?). I guess what really makes this one special is that it does not own the storage that it points to...


================
Comment at: lldb/source/Core/Module.cpp:154-159
+  // If the ModuleSpec provided a DataBuffer, let's respect the ModuleSpec's
+  // file offset when reading in this buffer.
+  if (data_sp) {
+    file_offset = module_spec.GetObjectOffset();
+    file_size = module_spec.GetData()->GetByteSize();
+  }
----------------
I think this overloads the meaning of `module_spec.GetObjectOffset()` in a fairly confusing way. Normally, I believe `ModuleSpec::GetObject{Name,Offset,Size}` is used to refer to the name/offse/size of a object file located in an archive (.a). However, here it seems you are reusing it for something else. It seems unfortunate that the meaning of this field should change depending on the "data" field being present.

What exactly is the purpose of this field? Could we avoid this by just creating an appropriately-sized DataBuffer ?


================
Comment at: lldb/source/Core/Module.cpp:1265-1272
+      if (m_object_size)
+        file_size = m_object_size;
+      else
+        file_size =
+            FileSystem::Instance().GetByteSize(m_file) - m_object_offset;
+
+      if (m_data_sp)
----------------
With an appropriately sized data_sp, I'm hoping most if this could go away...


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:935-948
+  // In the shared cache, the load command file offsets are relative to the
+  // base of the shared cache, not the dylib image.
+  Section *segment = section->GetParent().get();
+  if (!segment)
+    segment = section;
+
+  // We know __TEXT is at offset 0 of the image. Compute the offset of the
----------------
Wouldn't this be better handled by adjusting the file offsets during Section creation?


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:42-44
+#include <Plugins/ObjectFile/Mach-O/ObjectFileMachO.h>
+
+#include <mach/mach.h>
----------------
Leftovers from an earlier implementation?


================
Comment at: lldb/source/Symbol/ObjectFile.cpp:217
+  else
+    data_offset = file_offset;
+
----------------
this data/file_offset business would be nice to get rid of too...


================
Comment at: lldb/unittests/Host/HostInfoTest.cpp:74
+  for (const auto& image : shared_cache_info.GetImages()) {
+    EXPECT_TRUE(image.getValue().unslidTextOffset < shared_cache_info.GetData()->GetByteSize());
+  }
----------------
EXPECT_LT


================
Comment at: llvm/include/llvm/BinaryFormat/MachO.h:86
+  MH_NLIST_OUTOFSYNC_WITH_DYLDINFO = 0x04000000u,
+  MH_DYLIB_IN_CACHE = 0x80000000u,
 };
----------------
Maybe just commit this, and all the 0x80000000u replacements straight away.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83023