[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGfc54427e76c8: [lldb] Refactor DataBuffer so we can map files as read-only (authored by JDevlieghere). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D122856?vs=420589=420625#toc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122856/new/ https://reviews.llvm.org/D122856 Files: lldb/include/lldb/Core/ValueObject.h lldb/include/lldb/Host/FileSystem.h lldb/include/lldb/Symbol/CompactUnwindInfo.h lldb/include/lldb/Symbol/ObjectFile.h lldb/include/lldb/Target/ProcessStructReader.h lldb/include/lldb/Target/RegisterCheckpoint.h lldb/include/lldb/Target/RegisterContext.h lldb/include/lldb/Target/RegisterContextUnwind.h lldb/include/lldb/Utility/DataBuffer.h lldb/include/lldb/Utility/DataBufferHeap.h lldb/include/lldb/Utility/DataBufferLLVM.h lldb/include/lldb/lldb-forward.h lldb/source/Commands/CommandObjectMemory.cpp lldb/source/Core/SourceManager.cpp lldb/source/Core/ValueObject.cpp lldb/source/DataFormatters/StringPrinter.cpp lldb/source/DataFormatters/TypeFormat.cpp lldb/source/Expression/IRExecutionUnit.cpp lldb/source/Host/common/FileSystem.cpp lldb/source/Host/common/Host.cpp lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp lldb/source/Plugins/Language/ObjC/CF.cpp lldb/source/Plugins/Language/ObjC/NSDictionary.cpp lldb/source/Plugins/Language/ObjC/NSSet.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.h lldb/source/Plugins/Process/Utility/RegisterContextDummy.cpp lldb/source/Plugins/Process/Utility/RegisterContextDummy.h lldb/source/Plugins/Process/Utility/RegisterContextHistory.cpp lldb/source/Plugins/Process/Utility/RegisterContextHistory.h lldb/source/Plugins/Process/Utility/RegisterContextMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextMemory.h lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_32.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp lldb/source/Symbol/ObjectFile.cpp lldb/source/Target/Platform.cpp lldb/source/Target/RegisterContextUnwind.cpp
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks good. Thanks for your patience. Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2636-2638 + assert(llvm::isa(data_buffer_sp.get())); + WritableDataBuffer *data_buffer = + static_cast(data_buffer_sp.get()); use llvm::cast instead. 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
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
JDevlieghere updated this revision to Diff 420589. JDevlieghere added a comment. Herald added a subscriber: mgorny. - Copy the buffer if ObjectFileELF didn't map it - Use LLVM style RTTI CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122856/new/ https://reviews.llvm.org/D122856 Files: lldb/include/lldb/Core/ValueObject.h lldb/include/lldb/Host/FileSystem.h lldb/include/lldb/Symbol/CompactUnwindInfo.h lldb/include/lldb/Symbol/ObjectFile.h lldb/include/lldb/Target/ProcessStructReader.h lldb/include/lldb/Target/RegisterCheckpoint.h lldb/include/lldb/Target/RegisterContext.h lldb/include/lldb/Target/RegisterContextUnwind.h lldb/include/lldb/Utility/DataBuffer.h lldb/include/lldb/Utility/DataBufferHeap.h lldb/include/lldb/Utility/DataBufferLLVM.h lldb/include/lldb/lldb-forward.h lldb/source/Commands/CommandObjectMemory.cpp lldb/source/Core/SourceManager.cpp lldb/source/Core/ValueObject.cpp lldb/source/DataFormatters/StringPrinter.cpp lldb/source/DataFormatters/TypeFormat.cpp lldb/source/Expression/IRExecutionUnit.cpp lldb/source/Host/common/FileSystem.cpp lldb/source/Host/common/Host.cpp lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp lldb/source/Plugins/Language/ObjC/CF.cpp lldb/source/Plugins/Language/ObjC/NSDictionary.cpp lldb/source/Plugins/Language/ObjC/NSSet.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.h lldb/source/Plugins/Process/Utility/RegisterContextDummy.cpp lldb/source/Plugins/Process/Utility/RegisterContextDummy.h lldb/source/Plugins/Process/Utility/RegisterContextHistory.cpp lldb/source/Plugins/Process/Utility/RegisterContextHistory.h lldb/source/Plugins/Process/Utility/RegisterContextMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextMemory.h lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_32.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp lldb/source/Symbol/ObjectFile.cpp lldb/source/Target/Platform.cpp lldb/source/Target/RegisterContextUnwind.cpp lldb/source/Utility/DataBufferHeap.cpp lldb/source/Utility/DataBufferLLVM.cpp lldb/unittests/Utility/CMakeLists.txt lldb/unittests/Utility/DataBufferTest.cpp Index: lldb/unittests/Utility/DataBufferTest.cpp
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
JDevlieghere updated this revision to Diff 420540. JDevlieghere marked an inline comment as done. JDevlieghere added a comment. I don't really like the idea of making the argument to createInstance writable. That seems like an implementation detail. Instead I added a virtual method IsWritable() to the DataBuffer and an assert to ObjectFileELF that ensures we always have a writable buffer. @labath: does that cover your concerns? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122856/new/ https://reviews.llvm.org/D122856 Files: lldb/include/lldb/Core/ValueObject.h lldb/include/lldb/Host/FileSystem.h lldb/include/lldb/Symbol/CompactUnwindInfo.h lldb/include/lldb/Symbol/ObjectFile.h lldb/include/lldb/Target/ProcessStructReader.h lldb/include/lldb/Target/RegisterCheckpoint.h lldb/include/lldb/Target/RegisterContext.h lldb/include/lldb/Target/RegisterContextUnwind.h lldb/include/lldb/Utility/DataBuffer.h lldb/include/lldb/Utility/DataBufferHeap.h lldb/include/lldb/Utility/DataBufferLLVM.h lldb/include/lldb/lldb-forward.h lldb/source/Commands/CommandObjectMemory.cpp lldb/source/Core/SourceManager.cpp lldb/source/Core/ValueObject.cpp lldb/source/DataFormatters/StringPrinter.cpp lldb/source/DataFormatters/TypeFormat.cpp lldb/source/Expression/IRExecutionUnit.cpp lldb/source/Host/common/FileSystem.cpp lldb/source/Host/common/Host.cpp lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp lldb/source/Plugins/Language/ObjC/CF.cpp lldb/source/Plugins/Language/ObjC/NSDictionary.cpp lldb/source/Plugins/Language/ObjC/NSSet.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.h lldb/source/Plugins/Process/Utility/RegisterContextDummy.cpp lldb/source/Plugins/Process/Utility/RegisterContextDummy.h lldb/source/Plugins/Process/Utility/RegisterContextHistory.cpp lldb/source/Plugins/Process/Utility/RegisterContextHistory.h lldb/source/Plugins/Process/Utility/RegisterContextMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextMemory.h lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_32.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp lldb/source/Symbol/ObjectFile.cpp lldb/source/Target/Platform.cpp lldb/source/Target/RegisterContextUnwind.cpp
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
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
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
JDevlieghere added inline comments.
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:382
ObjectFile *ObjectFileELF::CreateMemoryInstance(
const lldb::ModuleSP _sp, DataBufferSP _sp,
const lldb::ProcessSP _sp, lldb::addr_t header_addr) {
JDevlieghere wrote:
> labath wrote:
> > I am assuming this will always point to a writable kind of a data buffer.
> > Could we change the prototype to reflect that?
> Are you okay with making that a separate patch?
https://reviews.llvm.org/D123073
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
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
JDevlieghere 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);
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.
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:382
ObjectFile *ObjectFileELF::CreateMemoryInstance(
const lldb::ModuleSP _sp, DataBufferSP _sp,
const lldb::ProcessSP _sp, lldb::addr_t header_addr) {
labath wrote:
> I am assuming this will always point to a writable kind of a data buffer.
> Could we change the prototype to reflect that?
Are you okay with making that a separate patch?
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
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
JDevlieghere updated this revision to Diff 420262. JDevlieghere marked 7 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122856/new/ https://reviews.llvm.org/D122856 Files: lldb/include/lldb/Core/ValueObject.h lldb/include/lldb/Host/FileSystem.h lldb/include/lldb/Symbol/CompactUnwindInfo.h lldb/include/lldb/Symbol/ObjectFile.h lldb/include/lldb/Target/ProcessStructReader.h lldb/include/lldb/Target/RegisterCheckpoint.h lldb/include/lldb/Target/RegisterContext.h lldb/include/lldb/Target/RegisterContextUnwind.h lldb/include/lldb/Utility/DataBuffer.h lldb/include/lldb/Utility/DataBufferHeap.h lldb/include/lldb/Utility/DataBufferLLVM.h lldb/include/lldb/lldb-forward.h lldb/source/Commands/CommandObjectMemory.cpp lldb/source/Core/SourceManager.cpp lldb/source/Core/ValueObject.cpp lldb/source/DataFormatters/StringPrinter.cpp lldb/source/DataFormatters/TypeFormat.cpp lldb/source/Expression/IRExecutionUnit.cpp lldb/source/Host/common/FileSystem.cpp lldb/source/Host/common/Host.cpp lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp lldb/source/Plugins/Language/ObjC/CF.cpp lldb/source/Plugins/Language/ObjC/NSDictionary.cpp lldb/source/Plugins/Language/ObjC/NSSet.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.h lldb/source/Plugins/Process/Utility/RegisterContextDummy.cpp lldb/source/Plugins/Process/Utility/RegisterContextDummy.h lldb/source/Plugins/Process/Utility/RegisterContextHistory.cpp lldb/source/Plugins/Process/Utility/RegisterContextHistory.h lldb/source/Plugins/Process/Utility/RegisterContextMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextMemory.h lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_32.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp lldb/source/Symbol/ObjectFile.cpp lldb/source/Target/Platform.cpp lldb/source/Target/RegisterContextUnwind.cpp lldb/source/Utility/DataBufferHeap.cpp lldb/source/Utility/DataBufferLLVM.cpp Index: lldb/source/Utility/DataBufferLLVM.cpp === --- lldb/source/Utility/DataBufferLLVM.cpp +++ lldb/source/Utility/DataBufferLLVM.cpp @@ -14,8 +14,7 @@ using namespace lldb_private;
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
labath added inline comments.
Comment at: lldb/include/lldb/Utility/DataBuffer.h:58
+ /// if the object contains no bytes.
+ virtual const uint8_t *GetBytesImpl() const = 0;
+
Are you sure this should be public?
Comment at: lldb/include/lldb/Utility/DataBuffer.h:98-103
/// Get a const pointer to the data.
///
/// \return
/// A const pointer to the bytes owned by this object, or NULL
/// if the object contains no bytes.
+ const uint8_t *GetBytes() const { return GetBytesImpl(); }
Replace with `using DataBuffer::GetBytes` ?
Comment at: lldb/include/lldb/Utility/DataBuffer.h:109-111
llvm::ArrayRef GetData() const {
return llvm::ArrayRef(GetBytes(), GetByteSize());
}
ditto
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);
I guess this should be done unconditionally now.
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:382
ObjectFile *ObjectFileELF::CreateMemoryInstance(
const lldb::ModuleSP _sp, DataBufferSP _sp,
const lldb::ProcessSP _sp, lldb::addr_t header_addr) {
I am assuming this will always point to a writable kind of a data buffer. Could
we change the prototype to reflect that?
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2624
DataBufferSP _buffer_sp = debug_data.GetSharedDataBuffer();
+ WritableDataBuffer *data_buffer =
+ static_cast(data_buffer_sp.get());
add `// ObjectFileELF creates a WritableDataBuffer in CreateInstance`
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
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
shafik added inline comments. Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp:236 if (count) -m_name = ConstString((char *)buffer_sp->GetBytes()); +m_name = ConstString((const char *)buffer_sp->GetBytes()); else nitpick `reinterpret_cast` instead of C-style cast. 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
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
JDevlieghere updated this revision to Diff 419881. JDevlieghere edited the summary of this revision. JDevlieghere added a comment. - Implement Pavel's suggestion of mapping ELF files as writable and casting to a WritableMemoryBuffer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122856/new/ https://reviews.llvm.org/D122856 Files: lldb/include/lldb/Core/ValueObject.h lldb/include/lldb/Host/FileSystem.h lldb/include/lldb/Symbol/CompactUnwindInfo.h lldb/include/lldb/Symbol/ObjectFile.h lldb/include/lldb/Target/ProcessStructReader.h lldb/include/lldb/Target/RegisterCheckpoint.h lldb/include/lldb/Target/RegisterContext.h lldb/include/lldb/Target/RegisterContextUnwind.h lldb/include/lldb/Utility/DataBuffer.h lldb/include/lldb/Utility/DataBufferHeap.h lldb/include/lldb/Utility/DataBufferLLVM.h lldb/include/lldb/lldb-forward.h lldb/source/Commands/CommandObjectMemory.cpp lldb/source/Core/SourceManager.cpp lldb/source/Core/ValueObject.cpp lldb/source/DataFormatters/StringPrinter.cpp lldb/source/DataFormatters/TypeFormat.cpp lldb/source/Expression/IRExecutionUnit.cpp lldb/source/Host/common/FileSystem.cpp lldb/source/Host/common/Host.cpp lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp lldb/source/Plugins/Language/ObjC/CF.cpp lldb/source/Plugins/Language/ObjC/NSDictionary.cpp lldb/source/Plugins/Language/ObjC/NSSet.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.h lldb/source/Plugins/Process/Utility/RegisterContextDummy.cpp lldb/source/Plugins/Process/Utility/RegisterContextDummy.h lldb/source/Plugins/Process/Utility/RegisterContextHistory.cpp lldb/source/Plugins/Process/Utility/RegisterContextHistory.h lldb/source/Plugins/Process/Utility/RegisterContextMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextMemory.h lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_32.cpp lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp lldb/source/Symbol/ObjectFile.cpp lldb/source/Target/Platform.cpp lldb/source/Target/RegisterContextUnwind.cpp lldb/source/Utility/DataBufferHeap.cpp lldb/source/Utility/DataBufferLLVM.cpp Index: lldb/source/Utility/DataBufferLLVM.cpp === ---
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
labath added a comment. Yeah, I tried doing this (well, i used `const DataBuffer` to represent read-only data) last time this topic came up, and ran into the same problem. Since there was no pressing need for it, I put the patch away. I think the best way to fix this is to allow the object file classes to control the way the object file is mapped into memory (during creation). Then the ObjectFileELF could map the the file read-write, and the rest would do it read-only (though I think it's possible we need to relocate COFF files as well, only no one ran into the issue yet). If we wanted to be very fancy, we could check whether the object file needs relocations (basically, does it have any `.rel(a).debug_***` sections) and *then* map the file read-write, but I don't think that's really necessary. That doesn't directly help with the code in `ObjectFileELF::ApplyRelocations`, since it would still get an object whose static type prohibits writes. However, if we can ensure that the dynamic type of the object is alright, then we can put an appropriate cast there, and put a comment pointing to the place where the object is created. In llvm the relocations are being applied on-the-fly as the data is being read. That would be another possibility, but it would be a much more intrusive change (and tbh, I can't say I like the llvm model that much). OTOH, it would go a long way towards unifying llvm and lldb object readers 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
[Lldb-commits] [PATCH] D122856: [lldb] Refactor DataBuffer so we can map files as read-only
JDevlieghere created this revision. JDevlieghere added reviewers: labath, jasonmolenda, clayborg. Herald added subscribers: pmatos, asb, atanasyan, jrtc27, kbarton, sbc100, nemanjai, sdardis, emaste. Herald added a project: All. JDevlieghere requested review of this revision. Herald added subscribers: MaskRay, aheejin. Currently, all data buffers are assumed to be writable. This is a problem on macOS where it's not allowed to load unsigned binaries in memory as writable. To be more precise, `MAP_RESILIENT_CODESIGN` and `MAP_RESILIENT_MEDIA` need to be set for mapped (unsigned) binaries on our platform. Binaries are loaded through `FileSystem::CreateDataBuffer` which returns a DataBufferLLVM. The latter is backed by a `llvm::WritableMemoryBuffer` because every DataBuffer is considered to be writable. In order to use a read-only `llvm::MemoryBuffer` I had to split `DataBuffer` into `DataBuffer` (read-only) and `WritableDataBuffer` (read-write). Differentiating between read-only and read-write buffers worked out everywhere except for ObjectFileELF. The latter calls `GetSharedDataBuffer` on the `DataExtractor` and applies relocations in place. I understand the purpose of making this copy on write for just the affected pages, but I'm not sure how to make this work with the new approach: - Copying the whole object file to the heap when we need to apply the relocations is probably too expensive. - We could try to re-map the file but then we'd need to (1) know that it's a DataBufferLLVM and (2) have a way to remap the whole file a writable. I've ifdef'd out the code to show where the issue is. rdar://74890607 https://reviews.llvm.org/D122856 Files: lldb/include/lldb/Core/ValueObject.h lldb/include/lldb/Host/FileSystem.h lldb/include/lldb/Symbol/CompactUnwindInfo.h lldb/include/lldb/Target/ProcessStructReader.h lldb/include/lldb/Target/RegisterCheckpoint.h lldb/include/lldb/Target/RegisterContext.h lldb/include/lldb/Target/RegisterContextUnwind.h lldb/include/lldb/Utility/DataBuffer.h lldb/include/lldb/Utility/DataBufferHeap.h lldb/include/lldb/Utility/DataBufferLLVM.h lldb/include/lldb/lldb-forward.h lldb/source/Commands/CommandObjectMemory.cpp lldb/source/Core/SourceManager.cpp lldb/source/Core/ValueObject.cpp lldb/source/DataFormatters/StringPrinter.cpp lldb/source/Expression/IRExecutionUnit.cpp lldb/source/Host/common/FileSystem.cpp lldb/source/Host/common/Host.cpp lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp lldb/source/Plugins/Language/ObjC/CF.cpp lldb/source/Plugins/Language/ObjC/NSDictionary.cpp lldb/source/Plugins/Language/ObjC/NSSet.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_i386.h lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.cpp lldb/source/Plugins/Process/Utility/RegisterContextDarwin_x86_64.h lldb/source/Plugins/Process/Utility/RegisterContextDummy.cpp lldb/source/Plugins/Process/Utility/RegisterContextDummy.h lldb/source/Plugins/Process/Utility/RegisterContextHistory.cpp lldb/source/Plugins/Process/Utility/RegisterContextHistory.h lldb/source/Plugins/Process/Utility/RegisterContextMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextMemory.h lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h
