clayborg added a comment.
I will make update the patch with many of the suggested inline comments.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:150
+ if (m_arch.IsValid())
+ return m_arch;
const MinidumpSystemInfo *system_info = GetSystemInfo();
----------------
Can do
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:216
+ if (csd_version.find("Linux") != std::string::npos)
+ triple.setOS(llvm::Triple::OSType::Linux);
+ break;
----------------
I have run into some minidump files that don't have linux set corrreclty as the
OS, but they do have "linux" in the description. So this is to handle those
cases. I have told the people that are generating these about the issue and
they will fix it, but we have minidump files out in the wild that don't set
linux as the OS correctly.
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:179
+ auto platform_sp = target.GetPlatform();
+ if (platform_sp && !platform_sp->IsCompatibleArchitecture(arch, false,
nullptr)) {
+ ArchSpec platform_arch;
----------------
lemo wrote:
> shouldn't this be a check resulting in an error? why do we need to make this
> implicit adjustment here?
By default the "host" platform is selected and it was incorrectly being used
along with _any_ triple. So we need to adjust the platform if the host platform
isn't compatible. The platform being set correctly helps with many things
during a debug session like finding symbols and much more.
https://reviews.llvm.org/D49750
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
