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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
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:
> 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?
> 
I agree that this looks out of place in the minidump plugin. I don't see any other process/core file plugin doing that, but there should be nothing special about minidump files as far as platform selection goes, right? (In fact I wouldn't be surprised if this is causing problems for us already, as some of our elf core tests are disabled on windows).

Maybe this could be moved at least to a binary-format-agnostic place, so that all core file plugins benefit from this?


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:43-56
+enum {
+  // General Purpose Registers
+  reg_x0 = 0, reg_x1, reg_x2, reg_x3, reg_x4, reg_x5, reg_x6, reg_x7, reg_x8,
+  reg_x9, reg_x10, reg_x11, reg_x12, reg_x13, reg_x14, reg_x15, reg_x16,
+  reg_x17, reg_x18, reg_x19, reg_x20, reg_x21, reg_x22, reg_x23, reg_x24,
+  reg_x25, reg_x26, reg_x27, reg_x28, reg_fp, reg_lr, reg_sp, reg_pc, reg_cpsr,
+  // Floating Point Registers
----------------
It looks like these don't define any of the sub-registers (W, H, S and D sets)?

Btw, for x86, the way we avoided the need to define all of these registers all over again is that we took the register descriptions from the minidump file, and rearranged them in memory to match what the existing register contexts (for elf core files) expect. Maybe you could do that here too? Since you're already storing a copy of the register data in the `m_regs` field, it shouldn't even impact the memory footprint (but it will make all of this goo go away).


https://reviews.llvm.org/D49750