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

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:216
+      if (csd_version.find("Linux") != std::string::npos)
+        triple.setOS(llvm::Triple::OSType::Linux);
+      break;
----------------
clayborg wrote:
> 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.
1. any idea where this minidumps originate from?
2. should we raise some kind of signal when we go down this path? print an warning or at least verbose logging?


================
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;
----------------
clayborg wrote:
> 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.
Nice catch/fix in that case!

Just curious: It still seems a bit surprising to me to have the target mutated by the process object - is that how it's normally done in other cases?



https://reviews.llvm.org/D49750