[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.

Looks really good, a few comments inline.

This may not big a big deal, but the part about FP (and Apple vs. non-Apple) is confusing: the FP is a pretty weak convention, and in some ABIs is not actually "fixed" (ex. FP can be either https://reviews.llvm.org/source/openmp/ or https://reviews.llvm.org/source/libunwind/, which in turn can be used as GPRs). If Apple sticks to a strict usage it makes sense to name it but then maybe we should just not name any FP for non-Apple?



================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:149-150
 ArchSpec MinidumpParser::GetArchitecture() {
-  ArchSpec arch_spec;
+  if (m_arch.IsValid())
+    return m_arch;
   const MinidumpSystemInfo *system_info = GetSystemInfo();
----------------
instead of useing m_arch as a cache I'd explicitly initialize it in MinidumpParser::Initialize() 


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:212-216
+      std::string csd_version;
+      if (auto s = GetMinidumpString(system_info->csd_version_rva))
+        csd_version = *s;
+      if (csd_version.find("Linux") != std::string::npos)
+        triple.setOS(llvm::Triple::OSType::Linux);
----------------
why is this needed when we have MinidumpOSPlatform::Linux ?


================
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;
----------------
shouldn't this be a check resulting in an error? why do we need to make this implicit adjustment here?


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:192
+
+static size_t k_num_reg_infos = llvm::array_lengthof(g_reg_infos);
+
----------------
constexpr?


================
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,
----------------
use std::array for these kind of static arrays? (debug bounds checks, easy access to the static size, ...)


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:225
+  m_regs.context_flags = data.GetU32(&offset);
+  for (unsigned i=0; i<16; ++i)
+    m_regs.r[i] = data.GetU32(&offset);
----------------
symbolic constants or use the array size?


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:231
+    m_regs.d[i] = data.GetU64(&offset);
+  assert(k_num_regs == k_num_reg_infos_apple);
+  assert(k_num_regs == k_num_reg_infos);
----------------
lldb_assert ?


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:242
+}
+size_t RegisterContextMinidump_ARM::GetRegisterCount() {
+  return k_num_regs;
----------------
not a big deal, but this kind of accessors for constants can be constexpr themselves


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:248
+RegisterContextMinidump_ARM::GetRegisterInfoAtIndex(size_t reg) {
+  if (reg < k_num_reg_infos)
+    return &m_reg_infos[reg];
----------------
failfast if out of bounds? who'd pass an invalid index and expect a meaninful result?
(btw, std::array would provide the debug checks if that's all that we want)


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:281
+bool RegisterContextMinidump_ARM::WriteRegister(
+    const RegisterInfo *reg_info, const RegisterValue &reg_value) {
+  return false;
----------------
remove unused parameter name or [[maybe_unused]]


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h:82
+    Context m_regs;
+    RegisterInfo *m_reg_infos;
+    size_t m_num_reg_infos;
----------------
 = nullptr;


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h:83
+    RegisterInfo *m_reg_infos;
+    size_t m_num_reg_infos;
+};
----------------
= 0;


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h:37
+  
+  ~RegisterContextMinidump_ARM64() override {}
+  
----------------
=default instead of {} ?


https://reviews.llvm.org/D49750