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

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Thanks Greg, looks good to me (a couple of inline comments left at your discretion)



================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:15
 // Other libraries and framework includes
+//#include "lldb/Core/Architecture.h"
 #include "lldb/Core/Module.h"
----------------
it this set for removal?


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:195
+// ARM general purpose registers.
+const uint32_t g_gpr_regnums[] = {
+  reg_r0,  reg_r1,  reg_r2,  reg_r3,  reg_r4, reg_r5, reg_r6, reg_r7,
----------------
clayborg wrote:
> lemo wrote:
> > use std::array for these kind of static arrays? (debug bounds checks, easy access to the static size, ...)
> Tried it but it introduces a global constructor. We try to avoid those.
We shouldn't have a dynamic initializer: that's strange, if that's the case we have a compiler bug on our hands. A quick experiment indicates that even with -O0 recent clang/llvm do the right thing: https://godbolt.org/g/NMUFLP

Is the problem only with arrays of RegisterInfo structs? If that's the case the cause is RegisterInfo itself and std::array should not make a difference (ie. we'd see the dynamic initializer even with plain C arrays)


https://reviews.llvm.org/D49750