[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Greg Clayton via Phabricator via lldb-commits]

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