[Lldb-commits] [PATCH] D24124: [LLDB][MIPS] Fix register read/write for 32 bit big endian system

[Greg Clayton via lldb-commits]

clayborg added a comment.

A few things about a the RegisterContext class in case it would change and thing that you are submitting here. The entire register context defines a buffer of bytes that can contain all register values. Each RegisterInfo contains the offset for the value of this register in this buffer. This buffer is said to have a specific byte order (big, little, etc). When a register is read, it should place its bytes into the RegisterContext's buffer of bytes and mark itself as being valid in the register context. Some platforms read multiple registers at a time (they don't have a "read one register value", they just have "read all GPR registers") and lets say you are reading one GPR, and this causes all GPR values to be read, then all bytes from all GPR values will be copied into the register context data buffer and all GPRs should be marked as valid. So to get a RegisterValue for a 32 bit register, we normally will just ask the RegisterInfo for the offset of the register, and then extract the bytes from the buffer using a DataExtractor object. If you have a 64 bit register whose value also contains a 32 bit pseudo register (like rax contains eax on x86), then you should have a RegisterInfo defined for "rax" that says its offset is N, and for a big endian system, you would say that the register offset for "eax" is N + 4. Extracting the value simply becomes extracting the bytes from the buffer without the need for any tricks. After reading all of this, do you still believe you have the right fix in here? It doesn't seem like you ever should need to use DataExtractor::CopyByteOrderedData???


================
Comment at: source/Core/DataExtractor.cpp:949-958
@@ +948,12 @@
+  // Validate the source and destination info
+  assert(dst_void_ptr != nullptr || src_void_ptr != nullptr);
+  assert(dst_len > 0 || src_len > 0);
+  assert(src_len <= dst_len);
+  assert(dst_byte_order == eByteOrderBig || dst_byte_order == eByteOrderLittle);
+  assert(src_byte_order == eByteOrderBig || src_byte_order == eByteOrderLittle);
+
+  // Validate that only a word- or register-sized dst is byte swapped
+  assert(dst_byte_order == src_byte_order || dst_len == 1 || dst_len == 2 ||
+         dst_len == 4 || dst_len == 8 || dst_len == 10 || dst_len == 16 ||
+         dst_len == 32);
+
----------------
change all assert calls to lldbassert so they don't crash  running program and add if statements that return if any of the assertion fails. We can't just crash when we are unhappy with function input. I know llvm and clang do this, but it has caused way too many crashes for LLDB.

================
Comment at: source/Core/DataExtractor.cpp:1024-1026
@@ -942,5 +1023,5 @@
 lldb::offset_t
 DataExtractor::CopyByteOrderedData(offset_t src_offset, offset_t src_len,
                                    void *dst_void_ptr, offset_t dst_len,
                                    ByteOrder dst_byte_order) const {
   // Validate the source info
----------------
Should probably have this call the above new function so we don't duplicate functionality.


https://reviews.llvm.org/D24124