labath accepted this revision. labath added a comment. This revision is now accepted and ready to land.
Looks great. I only noticed some typos when looking this over again. We can continue the register shuffling discussion offline. ================ Comment at: include/lldb/Target/Target.h:929-939 + /// @param[in] adjust_platform + /// If \b true, then the platform will be adjusted if the currently + /// selected platform is not compatible with the archicture being set. + /// If \b false, then just the architecture will be set even if the + /// currently selected platform isn't compatible (in case it might be + /// manually set following this function call). + /// ---------------- mismatch in the parameter name in the comment and signature ================ Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:497 + if (!system_info) + return error; + ---------------- should `error` be initialized here? ================ Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:56 + k_num_regs +}; + ---------------- clayborg wrote: > I would rather define a new context and avoid mutating one register context > into another. I didn't really like the other register contexts for minidumps. > I like to show the actual data that is available, not a translation of one > set of data to another. Since, you're the one doing the work, I am fine leaving this up to you, but I'd like to understand what is it that you didn't like about the x86 register contexts. In what way do they misrepresent the data available? As far as I can tell, the difference here is just in the internal representation of the data. It should not change the actual values of the registers displayed to the user (if it does, I'd like to fix it). In other words, I should be able to rewrite the implementation here to reshuffle the register order in the same way as x86, and the tests should still pass. If this is true, then this is only a question of optimizing the internal implementation for some criteria. In this case, I would choose to optimize for code size/readability, and try to avoid defining 200 lines of constants, which largely duplicate what is already given in the other register contexts. ================ Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:221 + m_regs.context_flags = data.GetU64(&offset); + for (unsigned i=0; i<32; ++i) + m_regs.x[i] = data.GetU64(&offset); ---------------- llvm style would put spaces between the operators here. Could you run clang-format over the diff? https://reviews.llvm.org/D49750 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
