[Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

Zachary Turner via lldb-commits lldb-commits at lists.llvm.org
Mon Sep 26 09:48:23 PDT 2016


zturner added inline comments.

================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp:46-47
@@ +45,4 @@
+  source_data = source_data.drop_front(6 * 8); // p[1-6] home registers
+  const uint32_t *context_flags;
+  consumeObject(source_data, context_flags);
+  source_data = source_data.drop_front(4); // mx_csr
----------------
Can this be a `const MinidumpContext_x86_64_Flags*`?  Then you wouldn't have to do the `uint32_t` conversion each time since you could just `&` the flag you're checking directly against this value.

================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp:48
@@ +47,3 @@
+  consumeObject(source_data, context_flags);
+  source_data = source_data.drop_front(4); // mx_csr
+
----------------
Can we use a `sizeof()` here?

================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h:37
@@ +36,3 @@
+struct MinidumpXmmSaveArea32AMD64 {
+  uint16_t control_word;
+  uint16_t status_word;
----------------
Should these be endian specific types like ulittle16_t?  If someone writes a minidump on a little endian system and runs this code on a big endian system, it would fail to parse the minidump correctly.

================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h:161
@@ +160,3 @@
+
+  static void writeRegister(llvm::ArrayRef<uint8_t> &data, uint8_t *base,
+                            const RegisterInfo *reg_info, size_t size);
----------------
labath wrote:
> This looks like a private utility function. It should not be in the header.
The order of parameters here is confusing.  the output buffer should be adjacent to its length.  So it should be:

`void writeRegister(ArrayRef<>, uint8_t*, size_t, const RegisterInfo*)`

That said, passing around pointers and lengths is a pattern that needs to die.  I would much rather this be:

`void writeRegister(ArrayRef<uint8_t> &reg_src, MutableArrayRef<uint8_t> reg_dest, const RegisterInfo &reg);`

================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:177
@@ +176,3 @@
+void registerEqualToVal(const uint64_t val, uint8_t *reg_val) {
+  ASSERT_EQ(val, *(reinterpret_cast<uint64_t *>(reg_val)));
+}
----------------
I would make the conversion a macro like this:

`#define REG_VAL(x) *(reinterpret_cast<uint64_t *>(x))`

Then write the asserts inline.  The reason for this is that when you do it this way, when a test fails the test runner will report that the failure happened in `registerEqualToVal`, which won't give you any clue about what test it failed in.  if the assertion is kept in the body of the test, the failure will report which test it was in, so you can more easily see.


https://reviews.llvm.org/D24919





More information about the lldb-commits mailing list