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

[Dimitar Vlahovski via lldb-commits]

dvlahovski marked 5 inline comments as done.

================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp:49
@@ +48,3 @@
+    writeRegister(source_data, result_base, &reg_info[lldb_cs_x86_64], 2);
+  }
+
----------------
zturner wrote:
> dvlahovski wrote:
> > sizeof(uint16_t), sizeof(uint32_t), etc ?
> I think my comments got out of line and this is no longer at the position I suggested it.  that said, I actually didn't notice all these constants being passed into `writeRegister`.  I would propose changing this.  Find the `RegisterInfo` class in `lldb-private-types.h` and add the following methods:
> 
> ```
> llvm::ArrayRef<uint8_t> data(const uint8_t *context_base) const { 
>   return llvm::ArrayRef<uint8_t>(context_base + byte_offset, byte_size);
> }
> 
> llvm::MutableArrayRef<uint8_t> mutable_data(uint8_t *context_base) const {
>   return llvm::MutableArrayRef<uint8_t>(context_base + byte_offset, byte_size);
> }
> ```
> 
> Then you can write:
> 
> ```writeRegister(source_data, reg_info[lldb_cs_x86_64].mutable_data(result_base));```
This will make the code very clean indeed, but there is a problem.

>From the `RegisterInfo` I only get the `byte_offset` and I don't use the `byte_size` because some of the registers in the Minidump context are smaller (e.g. cs is 16 bit). All of the registers in the Linux context are 64 bits in size. So that's why I basically hard-coded the size in each of the funciton calls. Now with the next revision I tried to make the code clearer.

================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h:38
@@ +37,3 @@
+// - uint64_t p4_home
+// - uint64_t p5_home
+// - uint64_t p6_home
----------------
So I had the actual struct for the register context in the previous revision, but now I've removed it, because I actually don't use it. So added a comment describing it's layout.

For the endianness part -  because this code only permutes the bytes in the layout the Linux register context expects them, I don't think that here is the place to handle that. I was hoping that the Linux register context will handle it, but I don't think that's the case when I look at the code in `RegisterContextCorePOSIX_x86_64::ReadRegister`.

Am I right - do the Linux context assume little endian? If so maybe I can also fix that.

================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:177
@@ +176,3 @@
+static void registerEqualToVal(const uint64_t val, uint8_t *reg_val) {
+  ASSERT_EQ(val, *(reinterpret_cast<uint64_t *>(reg_val)));
+}
----------------
amccarth wrote:
> +1 to Zach's idea.
> 
> Also, consider whether `EXPECT_EQ` is more appropriate than `ASSERT_EQ` for these.
Probably `EXPECT_EQ` is better because if there is a mismatch in the reg values, one can see all of the values.

I still don't have the sense when to use `EXPECT_EQ` and when `ASSERT_EQ`.


https://reviews.llvm.org/D24919