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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Aug 1 02:02:53 PDT 2018

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?


More information about the lldb-commits mailing list