[LLDB][Minidump] Make workaround for the Dynamic loader issue #120166
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesIn #119598 my recent TLS feature seems to break crashpad symbols. I have a few ideas on how this is happening, but for now as a mitigation I'm checking if the Minidump was LLDB generated, and if so leveraging the dynamic loader. Patch is 22.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/120166.diff 6 Files Affected:
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index bcac5edbc1a793..a4541f8bddf1a2 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -918,8 +918,8 @@ Status MinidumpFileBuilder::DumpHeader() const {
0u), // not used in most of the writers
header.TimeDateStamp =
static_cast<llvm::support::ulittle32_t>(std::time(nullptr));
- header.Flags =
- static_cast<llvm::support::ulittle64_t>(0u); // minidump normal flag
+ header.Flags = static_cast<llvm::support::ulittle64_t>(
+ llvm::minidump::Header::LLDB_HEADER_FLAG);
Status error;
size_t bytes_written;
diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index afc095ddbb2f91..6a328b3b841ed0 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -20,8 +20,8 @@
#include <algorithm>
#include <map>
#include <optional>
-#include <vector>
#include <utility>
+#include <vector>
using namespace lldb_private;
using namespace minidump;
@@ -45,6 +45,10 @@ llvm::ArrayRef<uint8_t> MinidumpParser::GetData() {
m_data_sp->GetByteSize());
}
+const llvm::minidump::Header *MinidumpParser::GetHeader() const {
+ return reinterpret_cast<llvm::minidump::Header *>(m_file.get());
+}
+
llvm::ArrayRef<uint8_t> MinidumpParser::GetStream(StreamType stream_type) {
return m_file->getRawStream(stream_type).value_or(llvm::ArrayRef<uint8_t>());
}
@@ -70,8 +74,7 @@ UUID MinidumpParser::GetModuleUUID(const minidump::Module *module) {
if (GetArchitecture().GetTriple().isOSBinFormatELF()) {
if (pdb70_uuid->Age != 0)
return UUID(pdb70_uuid, sizeof(*pdb70_uuid));
- return UUID(&pdb70_uuid->Uuid,
- sizeof(pdb70_uuid->Uuid));
+ return UUID(&pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid));
}
return UUID(*pdb70_uuid);
} else if (cv_signature == CvSignature::ElfBuildId)
@@ -453,10 +456,12 @@ MinidumpParser::FindMemoryRange(lldb::addr_t addr) {
if (!GetStream(StreamType::Memory64List).empty()) {
llvm::Error err = llvm::Error::success();
- for (const auto &memory_desc : GetMinidumpFile().getMemory64List(err)) {
- if (memory_desc.first.StartOfMemoryRange <= addr
- && addr < memory_desc.first.StartOfMemoryRange + memory_desc.first.DataSize) {
- return minidump::Range(memory_desc.first.StartOfMemoryRange, memory_desc.second);
+ for (const auto &memory_desc : GetMinidumpFile().getMemory64List(err)) {
+ if (memory_desc.first.StartOfMemoryRange <= addr &&
+ addr < memory_desc.first.StartOfMemoryRange +
+ memory_desc.first.DataSize) {
+ return minidump::Range(memory_desc.first.StartOfMemoryRange,
+ memory_desc.second);
}
}
@@ -490,7 +495,8 @@ llvm::ArrayRef<uint8_t> MinidumpParser::GetMemory(lldb::addr_t addr,
return range->range_ref.slice(offset, overlap);
}
-llvm::iterator_range<FallibleMemory64Iterator> MinidumpParser::GetMemory64Iterator(llvm::Error &err) {
+llvm::iterator_range<FallibleMemory64Iterator>
+MinidumpParser::GetMemory64Iterator(llvm::Error &err) {
llvm::ErrorAsOutParameter ErrAsOutParam(&err);
return m_file->getMemory64List(err);
}
@@ -602,8 +608,7 @@ std::pair<MemoryRegionInfos, bool> MinidumpParser::BuildMemoryRegions() {
case StreamType::ST: \
return #ST
-llvm::StringRef
-MinidumpParser::GetStreamTypeAsString(StreamType stream_type) {
+llvm::StringRef MinidumpParser::GetStreamTypeAsString(StreamType stream_type) {
switch (stream_type) {
ENUM_TO_CSTR(Unused);
ENUM_TO_CSTR(ThreadList);
diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.h b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
index f0b6e6027c52f0..e13065264668c2 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.h
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
@@ -47,7 +47,8 @@ struct Range {
}
};
-using FallibleMemory64Iterator = llvm::object::MinidumpFile::FallibleMemory64Iterator;
+using FallibleMemory64Iterator =
+ llvm::object::MinidumpFile::FallibleMemory64Iterator;
using ExceptionStreamsIterator =
llvm::object::MinidumpFile::ExceptionStreamsIterator;
@@ -56,6 +57,8 @@ class MinidumpParser {
static llvm::Expected<MinidumpParser>
Create(const lldb::DataBufferSP &data_buf_sp);
+ const llvm::minidump::Header *GetHeader() const;
+
llvm::ArrayRef<uint8_t> GetData();
llvm::ArrayRef<uint8_t> GetStream(StreamType stream_type);
@@ -96,7 +99,8 @@ class MinidumpParser {
/// complete (includes all regions mapped into the process memory).
std::pair<MemoryRegionInfos, bool> BuildMemoryRegions();
- llvm::iterator_range<FallibleMemory64Iterator> GetMemory64Iterator(llvm::Error &err);
+ llvm::iterator_range<FallibleMemory64Iterator>
+ GetMemory64Iterator(llvm::Error &err);
static llvm::StringRef GetStreamTypeAsString(StreamType stream_type);
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
index 5b0df72130c161..e03e91887781d8 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -138,7 +138,8 @@ lldb::ProcessSP ProcessMinidump::CreateInstance(lldb::TargetSP target_sp,
return nullptr;
lldbassert(DataPtr->GetByteSize() == header_size);
- if (identify_magic(toStringRef(DataPtr->GetData())) != llvm::file_magic::minidump)
+ if (identify_magic(toStringRef(DataPtr->GetData())) !=
+ llvm::file_magic::minidump)
return nullptr;
auto AllData =
@@ -344,6 +345,23 @@ ArchSpec ProcessMinidump::GetArchitecture() {
return ArchSpec(triple);
}
+bool ProcessMinidump::IsLLDBMinidump() const {
+ const llvm::minidump::Header *header = m_minidump_parser->GetHeader();
+ if (!header)
+ return false;
+ return (header->Flags & llvm::minidump::Header::LLDB_HEADER_FLAG) == 0;
+}
+
+DynamicLoader *ProcessMinidump::GetDynamicLoader() {
+ // This is a workaround for the dynamic loader not playing nice in issue
+ // #119598. The specific reason we use the dynamic loader is to get the TLS
+ // info sections, which we can assume are not being written to the minidump
+ // unless it's an LLDB generate minidump.
+ if (IsLLDBMinidump())
+ return PostMortemProcess::GetDynamicLoader();
+ return nullptr;
+}
+
DataExtractor ProcessMinidump::GetAuxvData() {
std::optional<llvm::ArrayRef<uint8_t>> auxv =
m_minidump_parser->GetStream(StreamType::LinuxAuxv);
@@ -487,8 +505,8 @@ void ProcessMinidump::ReadModuleList() {
Log *log = GetLog(LLDBLog::DynamicLoader);
for (auto module : filtered_modules) {
- std::string name = cantFail(m_minidump_parser->GetMinidumpFile().getString(
- module->ModuleNameRVA));
+ std::string name = cantFail(
+ m_minidump_parser->GetMinidumpFile().getString(module->ModuleNameRVA));
const uint64_t load_addr = module->BaseOfImage;
const uint64_t load_size = module->SizeOfImage;
LLDB_LOG(log, "found module: name: {0} {1:x10}-{2:x10} size: {3}", name,
@@ -507,8 +525,8 @@ void ProcessMinidump::ReadModuleList() {
Status error;
// Try and find a module with a full UUID that matches. This function will
// add the module to the target if it finds one.
- lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec,
- true /* notify */, &error);
+ lldb::ModuleSP module_sp =
+ GetTarget().GetOrCreateModule(module_spec, true /* notify */, &error);
if (module_sp) {
LLDB_LOG(log, "Full uuid match for {0}.", name);
} else {
@@ -532,9 +550,8 @@ void ProcessMinidump::ReadModuleList() {
// we don't then we will end up setting the load address of a different
// ObjectFilePlaceholder and an assertion will fire.
auto *objfile = module_sp->GetObjectFile();
- if (objfile &&
- objfile->GetPluginName() ==
- ObjectFilePlaceholder::GetPluginNameStatic()) {
+ if (objfile && objfile->GetPluginName() ==
+ ObjectFilePlaceholder::GetPluginNameStatic()) {
if (((ObjectFilePlaceholder *)objfile)->GetBaseImageAddress() !=
load_addr)
module_sp.reset();
@@ -564,8 +581,7 @@ void ProcessMinidump::ReadModuleList() {
}
bool load_addr_changed = false;
- module_sp->SetLoadAddress(GetTarget(), load_addr, false,
- load_addr_changed);
+ module_sp->SetLoadAddress(GetTarget(), load_addr, false, load_addr_changed);
}
}
@@ -593,10 +609,10 @@ JITLoaderList &ProcessMinidump::GetJITLoaders() {
return *m_jit_loaders_up;
}
-#define INIT_BOOL(VAR, LONG, SHORT, DESC) \
- VAR(LLDB_OPT_SET_1, false, LONG, SHORT, DESC, false, true)
-#define APPEND_OPT(VAR) \
- m_option_group.Append(&VAR, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1)
+#define INIT_BOOL(VAR, LONG, SHORT, DESC) \
+ VAR(LLDB_OPT_SET_1, false, LONG, SHORT, DESC, false, true)
+#define APPEND_OPT(VAR) \
+ m_option_group.Append(&VAR, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1)
class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
private:
@@ -657,55 +673,50 @@ class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
// If no options were set, then dump everything
m_dump_all.GetOptionValue().SetCurrentValue(true);
}
- bool DumpAll() const {
- return m_dump_all.GetOptionValue().GetCurrentValue();
- }
+ bool DumpAll() const { return m_dump_all.GetOptionValue().GetCurrentValue(); }
bool DumpDirectory() const {
- return DumpAll() ||
- m_dump_directory.GetOptionValue().GetCurrentValue();
+ return DumpAll() || m_dump_directory.GetOptionValue().GetCurrentValue();
}
bool DumpLinux() const {
return DumpAll() || m_dump_linux_all.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxCPUInfo() const {
return DumpLinux() ||
- m_dump_linux_cpuinfo.GetOptionValue().GetCurrentValue();
+ m_dump_linux_cpuinfo.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxProcStatus() const {
return DumpLinux() ||
- m_dump_linux_proc_status.GetOptionValue().GetCurrentValue();
+ m_dump_linux_proc_status.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxProcStat() const {
return DumpLinux() ||
- m_dump_linux_proc_stat.GetOptionValue().GetCurrentValue();
+ m_dump_linux_proc_stat.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxLSBRelease() const {
return DumpLinux() ||
- m_dump_linux_lsb_release.GetOptionValue().GetCurrentValue();
+ m_dump_linux_lsb_release.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxCMDLine() const {
return DumpLinux() ||
- m_dump_linux_cmdline.GetOptionValue().GetCurrentValue();
+ m_dump_linux_cmdline.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxEnviron() const {
return DumpLinux() ||
- m_dump_linux_environ.GetOptionValue().GetCurrentValue();
+ m_dump_linux_environ.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxAuxv() const {
- return DumpLinux() ||
- m_dump_linux_auxv.GetOptionValue().GetCurrentValue();
+ return DumpLinux() || m_dump_linux_auxv.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxMaps() const {
- return DumpLinux() ||
- m_dump_linux_maps.GetOptionValue().GetCurrentValue();
+ return DumpLinux() || m_dump_linux_maps.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxProcUptime() const {
return DumpLinux() ||
- m_dump_linux_proc_uptime.GetOptionValue().GetCurrentValue();
+ m_dump_linux_proc_uptime.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxProcFD() const {
return DumpLinux() ||
- m_dump_linux_proc_fd.GetOptionValue().GetCurrentValue();
+ m_dump_linux_proc_fd.GetOptionValue().GetCurrentValue();
}
bool DumpFacebook() const {
return DumpAll() || m_fb_all.GetOptionValue().GetCurrentValue();
@@ -743,60 +754,59 @@ class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
bool DumpFacebookLogcat() const {
return DumpFacebook() || m_fb_logcat.GetOptionValue().GetCurrentValue();
}
+
public:
CommandObjectProcessMinidumpDump(CommandInterpreter &interpreter)
- : CommandObjectParsed(interpreter, "process plugin dump",
- "Dump information from the minidump file.", nullptr),
- m_option_group(),
- INIT_BOOL(m_dump_all, "all", 'a',
- "Dump the everything in the minidump."),
- INIT_BOOL(m_dump_directory, "directory", 'd',
- "Dump the minidump directory map."),
- INIT_BOOL(m_dump_linux_cpuinfo, "cpuinfo", 'C',
- "Dump linux /proc/cpuinfo."),
- INIT_BOOL(m_dump_linux_proc_status, "status", 's',
- "Dump linux /proc/<pid>/status."),
- INIT_BOOL(m_dump_linux_lsb_release, "lsb-release", 'r',
- "Dump linux /etc/lsb-release."),
- INIT_BOOL(m_dump_linux_cmdline, "cmdline", 'c',
- "Dump linux /proc/<pid>/cmdline."),
- INIT_BOOL(m_dump_linux_environ, "environ", 'e',
- "Dump linux /proc/<pid>/environ."),
- INIT_BOOL(m_dump_linux_auxv, "auxv", 'x',
- "Dump linux /proc/<pid>/auxv."),
- INIT_BOOL(m_dump_linux_maps, "maps", 'm',
- "Dump linux /proc/<pid>/maps."),
- INIT_BOOL(m_dump_linux_proc_stat, "stat", 'S',
- "Dump linux /proc/<pid>/stat."),
- INIT_BOOL(m_dump_linux_proc_uptime, "uptime", 'u',
- "Dump linux process uptime."),
- INIT_BOOL(m_dump_linux_proc_fd, "fd", 'f',
- "Dump linux /proc/<pid>/fd."),
- INIT_BOOL(m_dump_linux_all, "linux", 'l',
- "Dump all linux streams."),
- INIT_BOOL(m_fb_app_data, "fb-app-data", 1,
- "Dump Facebook application custom data."),
- INIT_BOOL(m_fb_build_id, "fb-build-id", 2,
- "Dump the Facebook build ID."),
- INIT_BOOL(m_fb_version, "fb-version", 3,
- "Dump Facebook application version string."),
- INIT_BOOL(m_fb_java_stack, "fb-java-stack", 4,
- "Dump Facebook java stack."),
- INIT_BOOL(m_fb_dalvik, "fb-dalvik-info", 5,
- "Dump Facebook Dalvik info."),
- INIT_BOOL(m_fb_unwind, "fb-unwind-symbols", 6,
- "Dump Facebook unwind symbols."),
- INIT_BOOL(m_fb_error_log, "fb-error-log", 7,
- "Dump Facebook error log."),
- INIT_BOOL(m_fb_app_state, "fb-app-state-log", 8,
- "Dump Facebook java stack."),
- INIT_BOOL(m_fb_abort, "fb-abort-reason", 9,
- "Dump Facebook abort reason."),
- INIT_BOOL(m_fb_thread, "fb-thread-name", 10,
- "Dump Facebook thread name."),
- INIT_BOOL(m_fb_logcat, "fb-logcat", 11,
- "Dump Facebook logcat."),
- INIT_BOOL(m_fb_all, "facebook", 12, "Dump all Facebook streams.") {
+ : CommandObjectParsed(interpreter, "process plugin dump",
+ "Dump information from the minidump file.",
+ nullptr),
+ m_option_group(), INIT_BOOL(m_dump_all, "all", 'a',
+ "Dump the everything in the minidump."),
+ INIT_BOOL(m_dump_directory, "directory", 'd',
+ "Dump the minidump directory map."),
+ INIT_BOOL(m_dump_linux_cpuinfo, "cpuinfo", 'C',
+ "Dump linux /proc/cpuinfo."),
+ INIT_BOOL(m_dump_linux_proc_status, "status", 's',
+ "Dump linux /proc/<pid>/status."),
+ INIT_BOOL(m_dump_linux_lsb_release, "lsb-release", 'r',
+ "Dump linux /etc/lsb-release."),
+ INIT_BOOL(m_dump_linux_cmdline, "cmdline", 'c',
+ "Dump linux /proc/<pid>/cmdline."),
+ INIT_BOOL(m_dump_linux_environ, "environ", 'e',
+ "Dump linux /proc/<pid>/environ."),
+ INIT_BOOL(m_dump_linux_auxv, "auxv", 'x',
+ "Dump linux /proc/<pid>/auxv."),
+ INIT_BOOL(m_dump_linux_maps, "maps", 'm',
+ "Dump linux /proc/<pid>/maps."),
+ INIT_BOOL(m_dump_linux_proc_stat, "stat", 'S',
+ "Dump linux /proc/<pid>/stat."),
+ INIT_BOOL(m_dump_linux_proc_uptime, "uptime", 'u',
+ "Dump linux process uptime."),
+ INIT_BOOL(m_dump_linux_proc_fd, "fd", 'f',
+ "Dump linux /proc/<pid>/fd."),
+ INIT_BOOL(m_dump_linux_all, "linux", 'l', "Dump all linux streams."),
+ INIT_BOOL(m_fb_app_data, "fb-app-data", 1,
+ "Dump Facebook application custom data."),
+ INIT_BOOL(m_fb_build_id, "fb-build-id", 2,
+ "Dump the Facebook build ID."),
+ INIT_BOOL(m_fb_version, "fb-version", 3,
+ "Dump Facebook application version string."),
+ INIT_BOOL(m_fb_java_stack, "fb-java-stack", 4,
+ "Dump Facebook java stack."),
+ INIT_BOOL(m_fb_dalvik, "fb-dalvik-info", 5,
+ "Dump Facebook Dalvik info."),
+ INIT_BOOL(m_fb_unwind, "fb-unwind-symbols", 6,
+ "Dump Facebook unwind symbols."),
+ INIT_BOOL(m_fb_error_log, "fb-error-log", 7,
+ "Dump Facebook error log."),
+ INIT_BOOL(m_fb_app_state, "fb-app-state-log", 8,
+ "Dump Facebook java stack."),
+ INIT_BOOL(m_fb_abort, "fb-abort-reason", 9,
+ "Dump Facebook abort reason."),
+ INIT_BOOL(m_fb_thread, "fb-thread-name", 10,
+ "Dump Facebook thread name."),
+ INIT_BOOL(m_fb_logcat, "fb-logcat", 11, "Dump Facebook logcat."),
+ INIT_BOOL(m_fb_all, "facebook", 12, "Dump all Facebook streams.") {
APPEND_OPT(m_dump_all);
APPEND_OPT(m_dump_directory);
APPEND_OPT(m_dump_linux_cpuinfo);
@@ -899,8 +909,7 @@ class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
if (DumpLinuxProcFD())
DumpTextStream(StreamType::LinuxProcFD, "/proc/PID/fd");
if (DumpFacebookAppData())
- DumpTextStream(StreamType::FacebookAppCustomData,
- "Facebook App Data");
+ DumpTextStream(StreamType::FacebookAppCustomData, "Facebook App Data");
if (DumpFacebookBuildID()) {
auto bytes = minidump.GetStream(StreamType::FacebookBuildID);
if (bytes.size() >= 4) {
@@ -917,26 +926,21 @@ class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
DumpTextStream(StreamType::FacebookAppVersionName,
"Facebook Version String");
if (DumpFacebookJavaStack())
- DumpTextStream(StreamType::FacebookJavaStack,
- "Facebook Java Stack");
+ DumpTextStream(StreamType::FacebookJavaStack, "Facebook Java Stack");
if (DumpFacebookDalvikInfo())
- DumpTextStream(StreamType::FacebookDalvikInfo,
- "Facebook Dalvik Info");
+ DumpTextStream(StreamType::FacebookDalvikInfo, "Facebook Dalvik Info");
if (DumpFacebookUnwindSymbols())
DumpBinaryStream(StreamType::FacebookUnwindSymbols,
"Facebook Unwind Symbols Bytes");
if (DumpFacebookErrorLog())
- DumpTextStream(StreamType::FacebookDumpErrorLog,
- "Facebook Error Log");
+ DumpTextStream(StreamType::FacebookDumpErrorLog, "Facebook Error Log");
if (DumpFacebookAppStateLog())
DumpTextStream(StreamType::FacebookAppStateLog,
"Faceook Application State Log");
if (DumpFacebookAbortReason())
- DumpTextStream(StreamType::FacebookAbortReason,
- "Facebook Abort Reason");
+ DumpTextStream(StreamType::FacebookAbortReason, "Facebook Abort Reason");
if (DumpFacebookThreadName())
- DumpTextStream(StreamType::FacebookThreadName,
- "Facebook Thread Name");
+ DumpTextStream(StreamType::FacebookThreadName, "Facebook Thread Name");
if (DumpFacebookLogcat()...
[truncated]
|
|
@llvm/pr-subscribers-llvm-binary-utilities Author: Jacob Lalonde (Jlalond) ChangesIn #119598 my recent TLS feature seems to break crashpad symbols. I have a few ideas on how this is happening, but for now as a mitigation I'm checking if the Minidump was LLDB generated, and if so leveraging the dynamic loader. Patch is 22.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/120166.diff 6 Files Affected:
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index bcac5edbc1a793..a4541f8bddf1a2 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -918,8 +918,8 @@ Status MinidumpFileBuilder::DumpHeader() const {
0u), // not used in most of the writers
header.TimeDateStamp =
static_cast<llvm::support::ulittle32_t>(std::time(nullptr));
- header.Flags =
- static_cast<llvm::support::ulittle64_t>(0u); // minidump normal flag
+ header.Flags = static_cast<llvm::support::ulittle64_t>(
+ llvm::minidump::Header::LLDB_HEADER_FLAG);
Status error;
size_t bytes_written;
diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index afc095ddbb2f91..6a328b3b841ed0 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -20,8 +20,8 @@
#include <algorithm>
#include <map>
#include <optional>
-#include <vector>
#include <utility>
+#include <vector>
using namespace lldb_private;
using namespace minidump;
@@ -45,6 +45,10 @@ llvm::ArrayRef<uint8_t> MinidumpParser::GetData() {
m_data_sp->GetByteSize());
}
+const llvm::minidump::Header *MinidumpParser::GetHeader() const {
+ return reinterpret_cast<llvm::minidump::Header *>(m_file.get());
+}
+
llvm::ArrayRef<uint8_t> MinidumpParser::GetStream(StreamType stream_type) {
return m_file->getRawStream(stream_type).value_or(llvm::ArrayRef<uint8_t>());
}
@@ -70,8 +74,7 @@ UUID MinidumpParser::GetModuleUUID(const minidump::Module *module) {
if (GetArchitecture().GetTriple().isOSBinFormatELF()) {
if (pdb70_uuid->Age != 0)
return UUID(pdb70_uuid, sizeof(*pdb70_uuid));
- return UUID(&pdb70_uuid->Uuid,
- sizeof(pdb70_uuid->Uuid));
+ return UUID(&pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid));
}
return UUID(*pdb70_uuid);
} else if (cv_signature == CvSignature::ElfBuildId)
@@ -453,10 +456,12 @@ MinidumpParser::FindMemoryRange(lldb::addr_t addr) {
if (!GetStream(StreamType::Memory64List).empty()) {
llvm::Error err = llvm::Error::success();
- for (const auto &memory_desc : GetMinidumpFile().getMemory64List(err)) {
- if (memory_desc.first.StartOfMemoryRange <= addr
- && addr < memory_desc.first.StartOfMemoryRange + memory_desc.first.DataSize) {
- return minidump::Range(memory_desc.first.StartOfMemoryRange, memory_desc.second);
+ for (const auto &memory_desc : GetMinidumpFile().getMemory64List(err)) {
+ if (memory_desc.first.StartOfMemoryRange <= addr &&
+ addr < memory_desc.first.StartOfMemoryRange +
+ memory_desc.first.DataSize) {
+ return minidump::Range(memory_desc.first.StartOfMemoryRange,
+ memory_desc.second);
}
}
@@ -490,7 +495,8 @@ llvm::ArrayRef<uint8_t> MinidumpParser::GetMemory(lldb::addr_t addr,
return range->range_ref.slice(offset, overlap);
}
-llvm::iterator_range<FallibleMemory64Iterator> MinidumpParser::GetMemory64Iterator(llvm::Error &err) {
+llvm::iterator_range<FallibleMemory64Iterator>
+MinidumpParser::GetMemory64Iterator(llvm::Error &err) {
llvm::ErrorAsOutParameter ErrAsOutParam(&err);
return m_file->getMemory64List(err);
}
@@ -602,8 +608,7 @@ std::pair<MemoryRegionInfos, bool> MinidumpParser::BuildMemoryRegions() {
case StreamType::ST: \
return #ST
-llvm::StringRef
-MinidumpParser::GetStreamTypeAsString(StreamType stream_type) {
+llvm::StringRef MinidumpParser::GetStreamTypeAsString(StreamType stream_type) {
switch (stream_type) {
ENUM_TO_CSTR(Unused);
ENUM_TO_CSTR(ThreadList);
diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.h b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
index f0b6e6027c52f0..e13065264668c2 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.h
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
@@ -47,7 +47,8 @@ struct Range {
}
};
-using FallibleMemory64Iterator = llvm::object::MinidumpFile::FallibleMemory64Iterator;
+using FallibleMemory64Iterator =
+ llvm::object::MinidumpFile::FallibleMemory64Iterator;
using ExceptionStreamsIterator =
llvm::object::MinidumpFile::ExceptionStreamsIterator;
@@ -56,6 +57,8 @@ class MinidumpParser {
static llvm::Expected<MinidumpParser>
Create(const lldb::DataBufferSP &data_buf_sp);
+ const llvm::minidump::Header *GetHeader() const;
+
llvm::ArrayRef<uint8_t> GetData();
llvm::ArrayRef<uint8_t> GetStream(StreamType stream_type);
@@ -96,7 +99,8 @@ class MinidumpParser {
/// complete (includes all regions mapped into the process memory).
std::pair<MemoryRegionInfos, bool> BuildMemoryRegions();
- llvm::iterator_range<FallibleMemory64Iterator> GetMemory64Iterator(llvm::Error &err);
+ llvm::iterator_range<FallibleMemory64Iterator>
+ GetMemory64Iterator(llvm::Error &err);
static llvm::StringRef GetStreamTypeAsString(StreamType stream_type);
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
index 5b0df72130c161..e03e91887781d8 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -138,7 +138,8 @@ lldb::ProcessSP ProcessMinidump::CreateInstance(lldb::TargetSP target_sp,
return nullptr;
lldbassert(DataPtr->GetByteSize() == header_size);
- if (identify_magic(toStringRef(DataPtr->GetData())) != llvm::file_magic::minidump)
+ if (identify_magic(toStringRef(DataPtr->GetData())) !=
+ llvm::file_magic::minidump)
return nullptr;
auto AllData =
@@ -344,6 +345,23 @@ ArchSpec ProcessMinidump::GetArchitecture() {
return ArchSpec(triple);
}
+bool ProcessMinidump::IsLLDBMinidump() const {
+ const llvm::minidump::Header *header = m_minidump_parser->GetHeader();
+ if (!header)
+ return false;
+ return (header->Flags & llvm::minidump::Header::LLDB_HEADER_FLAG) == 0;
+}
+
+DynamicLoader *ProcessMinidump::GetDynamicLoader() {
+ // This is a workaround for the dynamic loader not playing nice in issue
+ // #119598. The specific reason we use the dynamic loader is to get the TLS
+ // info sections, which we can assume are not being written to the minidump
+ // unless it's an LLDB generate minidump.
+ if (IsLLDBMinidump())
+ return PostMortemProcess::GetDynamicLoader();
+ return nullptr;
+}
+
DataExtractor ProcessMinidump::GetAuxvData() {
std::optional<llvm::ArrayRef<uint8_t>> auxv =
m_minidump_parser->GetStream(StreamType::LinuxAuxv);
@@ -487,8 +505,8 @@ void ProcessMinidump::ReadModuleList() {
Log *log = GetLog(LLDBLog::DynamicLoader);
for (auto module : filtered_modules) {
- std::string name = cantFail(m_minidump_parser->GetMinidumpFile().getString(
- module->ModuleNameRVA));
+ std::string name = cantFail(
+ m_minidump_parser->GetMinidumpFile().getString(module->ModuleNameRVA));
const uint64_t load_addr = module->BaseOfImage;
const uint64_t load_size = module->SizeOfImage;
LLDB_LOG(log, "found module: name: {0} {1:x10}-{2:x10} size: {3}", name,
@@ -507,8 +525,8 @@ void ProcessMinidump::ReadModuleList() {
Status error;
// Try and find a module with a full UUID that matches. This function will
// add the module to the target if it finds one.
- lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec,
- true /* notify */, &error);
+ lldb::ModuleSP module_sp =
+ GetTarget().GetOrCreateModule(module_spec, true /* notify */, &error);
if (module_sp) {
LLDB_LOG(log, "Full uuid match for {0}.", name);
} else {
@@ -532,9 +550,8 @@ void ProcessMinidump::ReadModuleList() {
// we don't then we will end up setting the load address of a different
// ObjectFilePlaceholder and an assertion will fire.
auto *objfile = module_sp->GetObjectFile();
- if (objfile &&
- objfile->GetPluginName() ==
- ObjectFilePlaceholder::GetPluginNameStatic()) {
+ if (objfile && objfile->GetPluginName() ==
+ ObjectFilePlaceholder::GetPluginNameStatic()) {
if (((ObjectFilePlaceholder *)objfile)->GetBaseImageAddress() !=
load_addr)
module_sp.reset();
@@ -564,8 +581,7 @@ void ProcessMinidump::ReadModuleList() {
}
bool load_addr_changed = false;
- module_sp->SetLoadAddress(GetTarget(), load_addr, false,
- load_addr_changed);
+ module_sp->SetLoadAddress(GetTarget(), load_addr, false, load_addr_changed);
}
}
@@ -593,10 +609,10 @@ JITLoaderList &ProcessMinidump::GetJITLoaders() {
return *m_jit_loaders_up;
}
-#define INIT_BOOL(VAR, LONG, SHORT, DESC) \
- VAR(LLDB_OPT_SET_1, false, LONG, SHORT, DESC, false, true)
-#define APPEND_OPT(VAR) \
- m_option_group.Append(&VAR, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1)
+#define INIT_BOOL(VAR, LONG, SHORT, DESC) \
+ VAR(LLDB_OPT_SET_1, false, LONG, SHORT, DESC, false, true)
+#define APPEND_OPT(VAR) \
+ m_option_group.Append(&VAR, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1)
class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
private:
@@ -657,55 +673,50 @@ class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
// If no options were set, then dump everything
m_dump_all.GetOptionValue().SetCurrentValue(true);
}
- bool DumpAll() const {
- return m_dump_all.GetOptionValue().GetCurrentValue();
- }
+ bool DumpAll() const { return m_dump_all.GetOptionValue().GetCurrentValue(); }
bool DumpDirectory() const {
- return DumpAll() ||
- m_dump_directory.GetOptionValue().GetCurrentValue();
+ return DumpAll() || m_dump_directory.GetOptionValue().GetCurrentValue();
}
bool DumpLinux() const {
return DumpAll() || m_dump_linux_all.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxCPUInfo() const {
return DumpLinux() ||
- m_dump_linux_cpuinfo.GetOptionValue().GetCurrentValue();
+ m_dump_linux_cpuinfo.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxProcStatus() const {
return DumpLinux() ||
- m_dump_linux_proc_status.GetOptionValue().GetCurrentValue();
+ m_dump_linux_proc_status.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxProcStat() const {
return DumpLinux() ||
- m_dump_linux_proc_stat.GetOptionValue().GetCurrentValue();
+ m_dump_linux_proc_stat.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxLSBRelease() const {
return DumpLinux() ||
- m_dump_linux_lsb_release.GetOptionValue().GetCurrentValue();
+ m_dump_linux_lsb_release.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxCMDLine() const {
return DumpLinux() ||
- m_dump_linux_cmdline.GetOptionValue().GetCurrentValue();
+ m_dump_linux_cmdline.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxEnviron() const {
return DumpLinux() ||
- m_dump_linux_environ.GetOptionValue().GetCurrentValue();
+ m_dump_linux_environ.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxAuxv() const {
- return DumpLinux() ||
- m_dump_linux_auxv.GetOptionValue().GetCurrentValue();
+ return DumpLinux() || m_dump_linux_auxv.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxMaps() const {
- return DumpLinux() ||
- m_dump_linux_maps.GetOptionValue().GetCurrentValue();
+ return DumpLinux() || m_dump_linux_maps.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxProcUptime() const {
return DumpLinux() ||
- m_dump_linux_proc_uptime.GetOptionValue().GetCurrentValue();
+ m_dump_linux_proc_uptime.GetOptionValue().GetCurrentValue();
}
bool DumpLinuxProcFD() const {
return DumpLinux() ||
- m_dump_linux_proc_fd.GetOptionValue().GetCurrentValue();
+ m_dump_linux_proc_fd.GetOptionValue().GetCurrentValue();
}
bool DumpFacebook() const {
return DumpAll() || m_fb_all.GetOptionValue().GetCurrentValue();
@@ -743,60 +754,59 @@ class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
bool DumpFacebookLogcat() const {
return DumpFacebook() || m_fb_logcat.GetOptionValue().GetCurrentValue();
}
+
public:
CommandObjectProcessMinidumpDump(CommandInterpreter &interpreter)
- : CommandObjectParsed(interpreter, "process plugin dump",
- "Dump information from the minidump file.", nullptr),
- m_option_group(),
- INIT_BOOL(m_dump_all, "all", 'a',
- "Dump the everything in the minidump."),
- INIT_BOOL(m_dump_directory, "directory", 'd',
- "Dump the minidump directory map."),
- INIT_BOOL(m_dump_linux_cpuinfo, "cpuinfo", 'C',
- "Dump linux /proc/cpuinfo."),
- INIT_BOOL(m_dump_linux_proc_status, "status", 's',
- "Dump linux /proc/<pid>/status."),
- INIT_BOOL(m_dump_linux_lsb_release, "lsb-release", 'r',
- "Dump linux /etc/lsb-release."),
- INIT_BOOL(m_dump_linux_cmdline, "cmdline", 'c',
- "Dump linux /proc/<pid>/cmdline."),
- INIT_BOOL(m_dump_linux_environ, "environ", 'e',
- "Dump linux /proc/<pid>/environ."),
- INIT_BOOL(m_dump_linux_auxv, "auxv", 'x',
- "Dump linux /proc/<pid>/auxv."),
- INIT_BOOL(m_dump_linux_maps, "maps", 'm',
- "Dump linux /proc/<pid>/maps."),
- INIT_BOOL(m_dump_linux_proc_stat, "stat", 'S',
- "Dump linux /proc/<pid>/stat."),
- INIT_BOOL(m_dump_linux_proc_uptime, "uptime", 'u',
- "Dump linux process uptime."),
- INIT_BOOL(m_dump_linux_proc_fd, "fd", 'f',
- "Dump linux /proc/<pid>/fd."),
- INIT_BOOL(m_dump_linux_all, "linux", 'l',
- "Dump all linux streams."),
- INIT_BOOL(m_fb_app_data, "fb-app-data", 1,
- "Dump Facebook application custom data."),
- INIT_BOOL(m_fb_build_id, "fb-build-id", 2,
- "Dump the Facebook build ID."),
- INIT_BOOL(m_fb_version, "fb-version", 3,
- "Dump Facebook application version string."),
- INIT_BOOL(m_fb_java_stack, "fb-java-stack", 4,
- "Dump Facebook java stack."),
- INIT_BOOL(m_fb_dalvik, "fb-dalvik-info", 5,
- "Dump Facebook Dalvik info."),
- INIT_BOOL(m_fb_unwind, "fb-unwind-symbols", 6,
- "Dump Facebook unwind symbols."),
- INIT_BOOL(m_fb_error_log, "fb-error-log", 7,
- "Dump Facebook error log."),
- INIT_BOOL(m_fb_app_state, "fb-app-state-log", 8,
- "Dump Facebook java stack."),
- INIT_BOOL(m_fb_abort, "fb-abort-reason", 9,
- "Dump Facebook abort reason."),
- INIT_BOOL(m_fb_thread, "fb-thread-name", 10,
- "Dump Facebook thread name."),
- INIT_BOOL(m_fb_logcat, "fb-logcat", 11,
- "Dump Facebook logcat."),
- INIT_BOOL(m_fb_all, "facebook", 12, "Dump all Facebook streams.") {
+ : CommandObjectParsed(interpreter, "process plugin dump",
+ "Dump information from the minidump file.",
+ nullptr),
+ m_option_group(), INIT_BOOL(m_dump_all, "all", 'a',
+ "Dump the everything in the minidump."),
+ INIT_BOOL(m_dump_directory, "directory", 'd',
+ "Dump the minidump directory map."),
+ INIT_BOOL(m_dump_linux_cpuinfo, "cpuinfo", 'C',
+ "Dump linux /proc/cpuinfo."),
+ INIT_BOOL(m_dump_linux_proc_status, "status", 's',
+ "Dump linux /proc/<pid>/status."),
+ INIT_BOOL(m_dump_linux_lsb_release, "lsb-release", 'r',
+ "Dump linux /etc/lsb-release."),
+ INIT_BOOL(m_dump_linux_cmdline, "cmdline", 'c',
+ "Dump linux /proc/<pid>/cmdline."),
+ INIT_BOOL(m_dump_linux_environ, "environ", 'e',
+ "Dump linux /proc/<pid>/environ."),
+ INIT_BOOL(m_dump_linux_auxv, "auxv", 'x',
+ "Dump linux /proc/<pid>/auxv."),
+ INIT_BOOL(m_dump_linux_maps, "maps", 'm',
+ "Dump linux /proc/<pid>/maps."),
+ INIT_BOOL(m_dump_linux_proc_stat, "stat", 'S',
+ "Dump linux /proc/<pid>/stat."),
+ INIT_BOOL(m_dump_linux_proc_uptime, "uptime", 'u',
+ "Dump linux process uptime."),
+ INIT_BOOL(m_dump_linux_proc_fd, "fd", 'f',
+ "Dump linux /proc/<pid>/fd."),
+ INIT_BOOL(m_dump_linux_all, "linux", 'l', "Dump all linux streams."),
+ INIT_BOOL(m_fb_app_data, "fb-app-data", 1,
+ "Dump Facebook application custom data."),
+ INIT_BOOL(m_fb_build_id, "fb-build-id", 2,
+ "Dump the Facebook build ID."),
+ INIT_BOOL(m_fb_version, "fb-version", 3,
+ "Dump Facebook application version string."),
+ INIT_BOOL(m_fb_java_stack, "fb-java-stack", 4,
+ "Dump Facebook java stack."),
+ INIT_BOOL(m_fb_dalvik, "fb-dalvik-info", 5,
+ "Dump Facebook Dalvik info."),
+ INIT_BOOL(m_fb_unwind, "fb-unwind-symbols", 6,
+ "Dump Facebook unwind symbols."),
+ INIT_BOOL(m_fb_error_log, "fb-error-log", 7,
+ "Dump Facebook error log."),
+ INIT_BOOL(m_fb_app_state, "fb-app-state-log", 8,
+ "Dump Facebook java stack."),
+ INIT_BOOL(m_fb_abort, "fb-abort-reason", 9,
+ "Dump Facebook abort reason."),
+ INIT_BOOL(m_fb_thread, "fb-thread-name", 10,
+ "Dump Facebook thread name."),
+ INIT_BOOL(m_fb_logcat, "fb-logcat", 11, "Dump Facebook logcat."),
+ INIT_BOOL(m_fb_all, "facebook", 12, "Dump all Facebook streams.") {
APPEND_OPT(m_dump_all);
APPEND_OPT(m_dump_directory);
APPEND_OPT(m_dump_linux_cpuinfo);
@@ -899,8 +909,7 @@ class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
if (DumpLinuxProcFD())
DumpTextStream(StreamType::LinuxProcFD, "/proc/PID/fd");
if (DumpFacebookAppData())
- DumpTextStream(StreamType::FacebookAppCustomData,
- "Facebook App Data");
+ DumpTextStream(StreamType::FacebookAppCustomData, "Facebook App Data");
if (DumpFacebookBuildID()) {
auto bytes = minidump.GetStream(StreamType::FacebookBuildID);
if (bytes.size() >= 4) {
@@ -917,26 +926,21 @@ class CommandObjectProcessMinidumpDump : public CommandObjectParsed {
DumpTextStream(StreamType::FacebookAppVersionName,
"Facebook Version String");
if (DumpFacebookJavaStack())
- DumpTextStream(StreamType::FacebookJavaStack,
- "Facebook Java Stack");
+ DumpTextStream(StreamType::FacebookJavaStack, "Facebook Java Stack");
if (DumpFacebookDalvikInfo())
- DumpTextStream(StreamType::FacebookDalvikInfo,
- "Facebook Dalvik Info");
+ DumpTextStream(StreamType::FacebookDalvikInfo, "Facebook Dalvik Info");
if (DumpFacebookUnwindSymbols())
DumpBinaryStream(StreamType::FacebookUnwindSymbols,
"Facebook Unwind Symbols Bytes");
if (DumpFacebookErrorLog())
- DumpTextStream(StreamType::FacebookDumpErrorLog,
- "Facebook Error Log");
+ DumpTextStream(StreamType::FacebookDumpErrorLog, "Facebook Error Log");
if (DumpFacebookAppStateLog())
DumpTextStream(StreamType::FacebookAppStateLog,
"Faceook Application State Log");
if (DumpFacebookAbortReason())
- DumpTextStream(StreamType::FacebookAbortReason,
- "Facebook Abort Reason");
+ DumpTextStream(StreamType::FacebookAbortReason, "Facebook Abort Reason");
if (DumpFacebookThreadName())
- DumpTextStream(StreamType::FacebookThreadName,
- "Facebook Thread Name");
+ DumpTextStream(StreamType::FacebookThreadName, "Facebook Thread Name");
if (DumpFacebookLogcat()...
[truncated]
|
labath
reviewed
Dec 17, 2024
There was a problem hiding this comment.
Marking a file as being generated by lldb makes sense, but given that there's no way (is there?) to reserve a flag bit for lldb, and the flag bits are a relatively scarce commodity (there's 64 of them total, how many are free right now?).
What would you say to creating a new "CreatedByLLDB" stream type and using that as the marker? There are 4 billion stream types, so the danger of collision is much lower, and we also have ample precedent for creating custom stream types. The thing I like about that is the we can then also insert additional metadata about the minidump into the stream, for example, whether it includes a linker rendezvous structures, which I think is the important part here (I'm not saying you have to do that now -- we can start out with a completely empty stream -- but I like how this gives us an option to do that in the future).
| @@ -45,6 +45,10 @@ llvm::ArrayRef<uint8_t> MinidumpParser::GetData() { | |||
| m_data_sp->GetByteSize()); | |||
| } | |||
|
|
|||
| const llvm::minidump::Header *MinidumpParser::GetHeader() const { | |||
| return reinterpret_cast<llvm::minidump::Header *>(m_file.get()); | |||
There was a problem hiding this comment.
This looks wrong. I think you want m_file->header()
| if (memory_desc.first.StartOfMemoryRange <= addr | ||
| && addr < memory_desc.first.StartOfMemoryRange + memory_desc.first.DataSize) { | ||
| return minidump::Range(memory_desc.first.StartOfMemoryRange, memory_desc.second); | ||
| for (const auto &memory_desc : GetMinidumpFile().getMemory64List(err)) { |
There was a problem hiding this comment.
Please revert all the formatting changes to code you didn't modify. You can submit those as a separate PR if you want.
| // We set all the high bits flag to indicate this is from LLDB. | ||
| // We don't want to conflict with anything Microsoft is using the flags for | ||
| // and the highest bits are not currently being used. | ||
| static const uint32_t LLDB_HEADER_FLAG = 0xF0000000; |
There was a problem hiding this comment.
uint64_t to match the width of the field, and change the name to fit the convention of the constants above.
Jlalond
force-pushed
the
tls-crashpad-fix
branch
from
697cf02 to
49bc821
Compare
|
@labath I went with your suggestion as the better idea. I ended up just writing 'LLDB' to the stream, because the current API behavior for MinidumpParser is to return an empty ArrayRef. So there was no direct way to detect an existing but empty stream. For now I just set the check if it's |
Jlalond
force-pushed
the
tls-crashpad-fix
branch
from
82b9c28 to
66de146
Compare
labath
approved these changes
Dec 23, 2024
There was a problem hiding this comment.
Thank you. If you really want to keep the magic header, then I guess it's fine, but I think it adds a lot of unnecessary code and is not particularly consistent with the other stream types (I don't remember seeing empty streams, but I also don't remember seeing streams that begin with a magic value -- it's kinda pointless since the stream type already serves as a sort of a magic value).
I don't find the "MinidumpParser does not distinguish empty/missing streams" argument particularly convincing, since the underlying llvm class (MinidumpFile) does distinguish them and a MinidumpParser::ContainsStream() function would be a lot simpler than this magic handling code.
| @@ -52,6 +52,7 @@ enum class StreamType : uint32_t { | |||
| #include "llvm/BinaryFormat/MinidumpConstants.def" | |||
| Unused = 0, | |||
| LastReserved = 0x0000ffff, | |||
| LLDBGenerated = 0x4C4C4442, // ASCII for 'LLDB' | |||
There was a problem hiding this comment.
Please put this in llvm/BinaryFormat/MinidumpConstants.def. We already have breakpad and facebook types defined there, so I don't see a reason to do something different here.
| if (m_is_lldb_generated.has_value()) | ||
| return *m_is_lldb_generated; | ||
|
|
||
| // If the minidump doesn't have a LLDBGeneratedStream, it's not an LLDB |
There was a problem hiding this comment.
There seems to be something missing in this sentence.
| // impacting older generated Minidumps. | ||
| llvm::ArrayRef<uint8_t> lldbStream = | ||
| m_minidump_parser->GetStream(StreamType::LLDBGenerated); | ||
| if (lldbStream.empty() || lldbStream.size() <= sizeof(StreamType)) { |
There was a problem hiding this comment.
Suggested change
| if (lldbStream.empty() || lldbStream.size() <= sizeof(StreamType)) { | |
| if (lldbStream.size() < sizeof(StreamType)) { |
empty is subsumed by the size check, and I think you need a strict comparison here.
Comment on lines
373
to
374
| const uint32_t *lldbStreamType = | ||
| reinterpret_cast<const uint32_t *>(lldbStream.data()); |
There was a problem hiding this comment.
Suggested change
| const uint32_t *lldbStreamType = | |
| reinterpret_cast<const uint32_t *>(lldbStream.data()); | |
| uint32_t lldbStreamType = llvm::support::endian::read<uint32_t, llvm::endianness::little>(lldbStream.data()); |
We only support little-endian minidumps right now, but it'd be nice if they could be read on a system of any endianness. I'm also not sure if this is guaranteed to be aligned.
After looking at your review, I agree. I'm not sure why I didn't want to add more features to Minidump parser, but this is much much cleaner now. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
labath
approved these changes
Dec 26, 2024
Comment on lines
92
to
94
| return error; | ||
| if (error.Fail()) | ||
| return error; |
There was a problem hiding this comment.
Remove the extra return?
(It would be nice to at least have a test that verifies that the lldb stream is actually written)
There was a problem hiding this comment.
We do have a test that depends on the dynamic loader, but it was passing in CI, even with this incorrect return which is unexpected. I tested locally by capturing a minidump with TLS variables on an older version of lldb, and verified there is no dyld or TLS variables when loaded with LLDB built on this patch. Capturing and loading on this patch ensured it worked. This does mean we're making a breaking change for behavior, where all older TLS minidumps will no longer work.
There was a problem hiding this comment.
Ultimately I think this patch is a workaround for the fact that the dynamic loader (posix) and process (minidump) classes can't agree on how to load the modules. The dynamic loader expect that it will be completely in charge of module loading, and this works fine in case of a live process, as it always contains enough information to determine the set of loaded modules. A minidump does not (always) contains this information, but (maybe for that same reason) it contains a explicit list of loaded modules (which is handled by the process class). When we start to use both sources, things get messy. I'm not sure what's the best way to handle this situation, but I think this patch approximates the desired behavior relatively well, and I think it is useful to be able to tell who generated a minidump.
There was a problem hiding this comment.
@labath I agree, I think it would be ideal to agree with Breakpad/Crashpad on a section about what program generated it, including version of that software. From Memory I know there is some brakepad relevant data, but a unified section would be great.
There was a problem hiding this comment.
We should change all dynamic loaders to always check with the process via:
lldb_private::StructuredData::ObjectSP Process::GetLoadedDynamicLibrariesInfos()
This allows processes to tell the dynamic loaders where things are loaded in case they know, like in the case of minidumps. So we can probably make the POSIX dynamic loader a bit smarter when it comes to dealing with core files.
There was a problem hiding this comment.
If nothing comes back from this call, then the dynamic loader will use its normal means to locate things, else, it should use the list that the process provides.
There was a problem hiding this comment.
@clayborg Just so I follow, you're recommending we embed a structured data (really just Json) into a section? If so, I really like the proposal, because it allows us to have a fairly loose schema across Brakepad, Crashpad and LLDB.
There was a problem hiding this comment.
No, we should implement the lldb_private::StructuredData::ObjectSP Process::GetLoadedDynamicLibrariesInfos() virtual method in ProcessMinidump and return a structured data that describes where everything is loaded. Then have the POSIX dynamic loader call this method on the process object and see if it returns anything. If it does, then we use this as the truth, else we discover the shared libraries using the existing code in the POSIX dynamic loader.
There was a problem hiding this comment.
(since ProcessMinidump has the shared library load list embedded in a directory).
There was a problem hiding this comment.
@clayborg Got it, from the context of the conversation I thought you wanted that structure data embedded in the Corefile.
I think that makes sense instead of using this Flag stream.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/8315 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/136/builds/2193 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #119598 my recent TLS feature seems to break crashpad symbols. I have a few ideas on how this is happening, but for now as a mitigation I'm checking if the Minidump was LLDB generated, and if so leveraging the dynamic loader.