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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
