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