[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:361
   // Update the data to contain the entire file if it doesn't already
   if (data_sp->GetByteSize() < length) {
+    data_sp = MapFileDataWritable(*file, length, file_offset);
----------------
JDevlieghere wrote:
> labath wrote:
> > I guess this should be done unconditionally now.
> No, I tried doing that actually and it broke a bunch of unit tests. I can take a look at it later this week if you think this is important. 
Right, I think I know what's going on. The unit tests use a (relatively new) object file feature where you can construct an ObjectFile instance without a backing file by providing all of its data upfront.

I think it is important to have a story for all entry points into the ELF code, since that is what makes the cast in `ApplyRelocations` sound. There are several ways to handle this. In my order of preference, they would be:
- In the file backed case, the initial 512-byte buffer will be heap-based (hence, effectively writable). If it is acceptable (and statically type-safe) to make the the buffer for the buffer-backed case writable, then we could change the `CreateInstance` prototype to take a `WritableDataBuffer`. Only the full mappings done by individual subclasses would be read-only. The only (non-test) use case for this feature is for some mach-o shenanigans so you'd be best qualified to determine if this would work.
- If we can be sure that the buffer-backed buffer is always writable, but we cannot enforce that in the type system, then we could add a comment documenting that the buffers dynamic type must be writable in one does not provide a backing file.
- Failing all that, we could have ObjectFileELF make a heap copy of the incoming data buffer in the buffer-backed case. Since there's no production use case for buffer-backed ELF files I think that should be fine. We could also have the ELF class refuse to open such files, but that would mean changing the unit tests back to using temporary files, which would make me sad.


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

https://reviews.llvm.org/D122856