[Lldb-commits] [PATCH] D83512: [lldb/Module] Allow for the creation of memory-only modules

Fri Jul 10 03:06:30 PDT 2020

labath added a comment.

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 <https://reviews.llvm.org/rGd372a8e8bce266bb4043e6a0bfd76c7e5bf457a5> 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.).

================
Comment at: lldb/include/lldb/Core/ModuleSpec.h:40
         m_uuid(uuid), m_object_name(), m_object_offset(0),
         m_object_size(FileSystem::Instance().GetByteSize(file_spec)),
+        m_source_mappings(), m_data(data) {}
----------------
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.

================
Comment at: lldb/include/lldb/Core/ModuleSpec.h:55
         m_object_mod_time(rhs.m_object_mod_time),
         m_source_mappings(rhs.m_source_mappings) {}

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83512