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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
