Page MenuHomePhabricator

[lldb/Module] Allow for the creation of memory-only modules
ClosedPublic

Authored by friss on Jul 9 2020, 2:36 PM.

Details

Summary

This patch extends the ModuleSpec class to include a
DataBufferSP which contains the module data. If this
data is provided, LLDB won't try to hit the filesystem
to create the Module, but use only the data stored in
the ModuleSpec.

Diff Detail

Event Timeline

friss created this revision.Jul 9 2020, 2:36 PM
Herald added a project: Restricted Project. · View Herald Transcript
friss added a comment.Jul 9 2020, 2:41 PM

This is the generic part of https://reviews.llvm.org/D83023

I modified to existing unittests to load their binary directly from memory, one ELF, one Mach-O. I tried to modify a PECOFF test to, but ObjectFilePECoff requires the file to be on the filesystem as it reopens it to create an LLVM PECoff object. I'm not sure about the testing strategy here. Should this just be a dedicated test instead, and remove the ugly InMemory thing that this patch adds to TestFile?

I like this.

The coff thingy is unfortunate, but it's not a hard limitation. It's just that nobody cared about making this work from memory (until now), and (re)loading the file from disk was the easiest solution. If my d372a8e8 sticks, then I think we should be able to load coff files from memory too. I would like if all tests loaded the files from memory, because it makes the code simpler, but maybe that does not need to be done here -- that can definitely be handled separately.

As for the testing strategy, I think that some dedicated tests would be nice in any case, if for nothing else, then for testing some of the edge cases (opening a corrupted data buffer, opening a buffer when a file with the given name exists on disk, etc.).

lldb/include/lldb/Core/ModuleSpec.h
38–39

I guess this GetByteSize call should also not fire if data is not set. In fact I'm not sure it should fire at all. I think at one point I tried to remove this and didn't get any failures on linux. If mac is the same then maybe we could delete it unconditionally.

52–53

You missed the copy constructor. I think both of these could be = default, or just omitted completely.

friss updated this revision to Diff 277657.Jul 13 2020, 9:52 PM

I went ahead and updated all unittests to use only in-memory modules.
This required tweaking both ObjectFileELF and ObjectFilePECOFF a little.

friss marked 2 inline comments as done.Jul 13 2020, 9:54 PM
friss added inline comments.
lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
543–544

This seems to work in the test, but I have to admit that I'm not 100% sure it's correct given the comment below about wanting to write this buffer.

565

I'm not sure why m_file was tested here, but this doesn't work with a pure in-memory file.

labath accepted this revision.Jul 14 2020, 1:51 AM
labath marked an inline comment as done.
labath added inline comments.
lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
543–544

I think this is fine. I can't find anything wanting to write to this data. Maybe that was the case once but perhaps that code was removed now.

lldb/unittests/Core/ModuleSpecTest.cpp
28

this is cool

31

s/anythign/anything

58

Maybe reduce these down? I'm not sure about others, but and elf file with a single .text section and no symbols should still perfectly valid.

lldb/unittests/TestingSupport/TestUtilities.h
39–43

Since we don't need to do cleanup anymore, we can make Buffer a regular std::string and rely on the compiler-generated move and copy operations. In fact, we may not even need the TestFile class at all as the yaml functions could return a ModuleSpec directly.

54

you'll need to add Buffer(std::move(Buffer)) for the && to do anything.

This revision is now accepted and ready to land.Jul 14 2020, 1:51 AM
This revision was automatically updated to reflect the committed changes.
friss marked an inline comment as done.Jul 14 2020, 8:49 AM
friss added inline comments.
lldb/unittests/TestingSupport/TestUtilities.h
39–43

I kept the TestFile class for now, as it owns the file memory buffer. I could change the tests to to a DataBufferHeap which would copy the memory, but be able to live on its own. Tell me if you feel strongly about this.