[Lldb-commits] [PATCH] D30622: Remove FileSpec::ReadFileContents
zturner created this revision.
The idea is that `DataBufferLLVM` already covers this use case, so there is no
point duplicating the code. The only tricky thing is that I found one case
where we attempt to modify the bytes we read in (in `ObjectFilePECOFF`). It
seems kind of hackish, but it worked because `ReadFileContents` would only ever
load a copy of the data into memory. But LLVM uses `mmap` whenever possible
and maps as readonly, so this was causing failures when we tried to modify the
bytes.
To address this I've added a flag called `Private` which forces us to not use
`mmap`. In an ideal world, we would still try to use `mmap`, but map the bytes
as private / writable. This is a patch for another day.
Also for another day is to rename `DataBufferLLVM` to `DataBufferFile`.
https://reviews.llvm.org/D30622
Files:
lldb/include/lldb/Host/FileSpec.h
lldb/include/lldb/Utility/DataBufferLLVM.h
lldb/source/API/SBSection.cpp
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Core/SourceManager.cpp
lldb/source/Host/common/FileSpec.cpp
lldb/source/Host/common/Host.cpp
lldb/source/Interpreter/OptionValueFileSpec.cpp
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/source/Symbol/ObjectFile.cpp
lldb/source/Utility/DataBufferLLVM.cpp
lldb/unittests/Process/minidump/MinidumpParserTest.cpp
Index: lldb/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -53,7 +53,7 @@
llvm::SmallString<128> filename = inputs_folder;
llvm::sys::path::append(filename, minidump_filename);
-auto BufferPtr = DataBufferLLVM::CreateFromPath(filename, load_size, 0);
+auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
llvm::Optional optional_parser =
MinidumpParser::Create(BufferPtr);
Index: lldb/source/Utility/DataBufferLLVM.cpp
===
--- lldb/source/Utility/DataBufferLLVM.cpp
+++ lldb/source/Utility/DataBufferLLVM.cpp
@@ -24,13 +24,28 @@
DataBufferLLVM::~DataBufferLLVM() {}
std::shared_ptr
-DataBufferLLVM::CreateFromPath(const llvm::Twine &Path, uint64_t Size,
- uint64_t Offset) {
+DataBufferLLVM::CreateSliceFromPath(const llvm::Twine &Path, uint64_t Size,
+ uint64_t Offset, bool Private) {
// If the file resides non-locally, pass the volatile flag so that we don't
// mmap it.
- bool Volatile = !llvm::sys::fs::is_local(Path);
+ if (!Private)
+Private = !llvm::sys::fs::is_local(Path);
- auto Buffer = llvm::MemoryBuffer::getFileSlice(Path, Size, Offset, Volatile);
+ auto Buffer = llvm::MemoryBuffer::getFileSlice(Path, Size, Offset, Private);
+ if (!Buffer)
+return nullptr;
+ return std::shared_ptr(
+ new DataBufferLLVM(std::move(*Buffer)));
+}
+
+std::shared_ptr
+DataBufferLLVM::CreateFromPath(const llvm::Twine &Path, bool NullTerminate, bool Private) {
+ // If the file resides non-locally, pass the volatile flag so that we don't
+ // mmap it.
+ if (!Private)
+Private = !llvm::sys::fs::is_local(Path);
+
+ auto Buffer = llvm::MemoryBuffer::getFile(Path, -1, NullTerminate, Private);
if (!Buffer)
return nullptr;
return std::shared_ptr(
Index: lldb/source/Symbol/ObjectFile.cpp
===
--- lldb/source/Symbol/ObjectFile.cpp
+++ lldb/source/Symbol/ObjectFile.cpp
@@ -22,6 +22,7 @@
#include "lldb/Target/Target.h"
#include "lldb/Utility/DataBuffer.h"
#include "lldb/Utility/DataBufferHeap.h"
+#include "lldb/Utility/DataBufferLLVM.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/RegularExpression.h"
#include "lldb/lldb-private.h"
@@ -76,8 +77,8 @@
// and object container plug-ins can use these bytes to see if they
// can parse this file.
if (file_size > 0) {
- data_sp = file->ReadFileContents(file_offset,
- std::min(512, file_size));
+ data_sp =
+ DataBufferLLVM::CreateSliceFromPath(file->GetPath(), 512, file_offset);
data_offset = 0;
}
}
@@ -120,7 +121,8 @@
}
// We failed to find any cached object files in the container
// plug-i
[Lldb-commits] [PATCH] D30624: Remove all of LLDB's custom filesystem statting code.
zturner created this revision.
Herald added a subscriber: emaste.
LLVM provides this functionality, so there is no point duplicating it. This is
a pretty large patch because we call functions like
`FileSpec::IsRegularFile()`, or `FileSpec::GetFileType()` all over the place,
and all of these are replaced with calls to llvm now. Also deleted is LLDB's
entire `FileType` enumeration.
I don't think it's a very important optimization, but one nice functional
change is that we now stat only once when previously we would stat many times
on the same file (for example calling `Exists()` followed by `IsDirectory()`
followed by `IsRegularFile()` is 3 stats, whereas in this patch we're down to 1.
In addition to all of this, I think this is an important change because it
removes an assumption previously baked into the `FileSpec` API that this was
always a valid operation. But it is pretty common to have a `FileSpec` that
refers to a file on a remote platform, perhaps even with an incomptible
`PathSyntax`, and in this case it doesn't make sense to be calling these
functions on it.
In any case, it's a necessary pre-requisite to getting `FileSpec` not be
dependent on `Host`.
https://reviews.llvm.org/D30624
Files:
lldb/include/lldb/Host/FileSpec.h
lldb/source/API/SBPlatform.cpp
lldb/source/Commands/CommandCompletions.cpp
lldb/source/Commands/CommandObjectTarget.cpp
lldb/source/Core/Debugger.cpp
lldb/source/Core/FileSpecList.cpp
lldb/source/Core/Module.cpp
lldb/source/Core/ModuleList.cpp
lldb/source/Core/PluginManager.cpp
lldb/source/Host/common/FileSpec.cpp
lldb/source/Host/common/MonitoringProcessLauncher.cpp
lldb/source/Host/common/Symbols.cpp
lldb/source/Host/macosx/Host.mm
lldb/source/Host/macosx/HostInfoMacOSX.mm
lldb/source/Host/macosx/Symbols.cpp
lldb/source/Host/posix/FileSystem.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformAppleTVSimulator.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformAppleWatchSimulator.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h
lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleTV.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleTV.h
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleWatch.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleWatch.h
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteiOS.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteiOS.h
lldb/source/Plugins/Platform/MacOSX/PlatformiOSSimulator.cpp
lldb/source/Plugins/Process/Darwin/NativeProcessDarwin.cpp
lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Target/ModuleCache.cpp
lldb/source/Target/Platform.cpp
lldb/source/Target/TargetList.cpp
llvm/include/llvm/Support/FileSystem.h
llvm/lib/Support/Path.cpp
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -953,6 +953,13 @@
return s.type() != file_type::status_error;
}
+file_type get_file_type(const Twine &Path) {
+ file_status st;
+ if (status(Path, st))
+return file_type::status_error;
+ return st.type();
+}
+
bool is_directory(file_status status) {
return status.type() == file_type::directory_file;
}
Index: llvm/include/llvm/Support/FileSystem.h
===
--- llvm/include/llvm/Support/FileSystem.h
+++ llvm/include/llvm/Support/FileSystem.h
@@ -483,6 +483,12 @@
///
/// @param status A file_status previously returned from status.
/// @returns status.type() == file_type::directory_file.
+file_type get_file_type(const Twine &Path);
+
+/// @brief Does status represent a directory?
+///
+/// @param status A file_status previously returned from status.
+/// @returns status.type() == file_type::directory_file.
bool is_directory(file_status status);
/// @brief Is path a directory?
Index: lldb/source/Target/TargetList.cpp
===
--- lldb/source/Target/TargetList.cpp
+++ lldb/source/Target/TargetList.cpp
@@ -362,7 +362,7 @@
char resolved_bundle_exe_path[PATH_MAX];
resolved_bundle_exe_path[0] = '\0';
if (file) {
-if (file.GetFileType() == FileSpec::eFileTypeDirectory)
+if (llvm::sys::fs::is_directory(file.GetPath()))
user_exe_path_is_bundle = true;
if (file.IsRelative() && !user_exe_path.empty()) {
Index: lldb/source/Target/Platform.cpp
===
--- lldb/sou
