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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to