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

Thu Jul 2 09:42:30 PDT 2020

friss marked 4 inline comments as done.
friss added a comment.

In D83023#2128298 <https://reviews.llvm.org/D83023#2128298>, @labath wrote:

> 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.

I'll see what can be done. My main goal while working on this was to avoid changing the semantics outside of the shared cache usecase. I understand fairly well the codepath that I added and then just moved some other bits around to keep the existing semantics for the rest. Happy to rework this.

> 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.

I agree about the light testing. FWIW, this got significant living-on testing on Apple platforms, but some more targeted testing would be great.

> 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... :/

Tell me about it...

I'll do a couple experiments in response to your comments. Thanks for the review!

================
Comment at: lldb/include/lldb/Host/macosx/HostInfoMacOSX.h:22-43
+struct SharedCacheImageInfo {
+  lldb::offset_t unslidTextOffset;
+  UUID uuid;
+};
+
+class SharedCacheInfo {
+public:
----------------
labath wrote:
> 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.
Yeah. When I wrote this patch, GetXcodeSDKPath didn't exist and I did it this way to avoid putting a very non-generic API at the top-level. As we're fine with this, it's easy to change.

================
Comment at: lldb/include/lldb/Utility/DataBuffer.h:82

+class DataBufferHostMemory : public DataBuffer {
+public:
----------------
labath wrote:
> 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...
Good point. What about `DataBufferUnowned` ? (and adding a comment to explain what it's used for)

================
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();
+  }
----------------
labath wrote:
> 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 ?
So the shared cache is some kind of a container in some ways similar to a `.a`, file, in some ways very different. The main difference is that the contained dylibs are not each in their own subpart of the file. For example, the __LINKEDIT segment is shared by all of them. The load commands that describe the images are relative to the shared cache itself, not to one specific image.

So I reused the object_offset field in its "offset from the beginning of a container sense". 

I think I tried having the SharedCacheInfo return only the subpart of the cache that contains the image, and ran into issues with this. But I don't remember exactly what, and I have a much better understanding now, so I should try it again.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83023