[LLDB][MIPS] Fix register read/write for 32 bit big endian system

[nitesh.jain]

Summary

The RegisterValue.SetBytes for register size of 4 byte followed by GetAsUInt64 on 32 bit big endian system will produce incorrect result. This patch fixes RegisterValue.GetAsUint64 function so that it can written correct value based on size.

[labath]

I don't think the patch can go in in this form. Also, you seem to be putting multiple unrelated changes in one patch. It would be much easier to review if you split that up into multiple patches.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2136

This looks like a massive hack. The register value object already takes a byte order as a parameter, so the fact that you are doing some funny endian conversions here means that there is something wrong. Also, this probably will not work for registers whose sizes are not 4 or 8 (%ah, %ax, all SSE registers, etc.).

I think we'll need to find a different way to fix this.

[nitesh.jain]

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2136

The problem is with RegisterValue.SetBytes

RegisterValue (uint8_t *bytes, size_t length, lldb::ByteOrder byte_order)

{
    SetBytes (bytes, length, byte_order);
}

The RegisterValue.SetBytes use memcpy to perform copy . So for register whose size is 4 it will be copy to lower 32bit LSB and hence RegisterValue.GetAsUInt64 will give incorrect result for 32 bit big endian system.

[labath]

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2136

I see that, but I still don't think this is a good idea. I think the fix should be done in a different way although I have to say I don't have an idea how (I am not too familiar with the RegisterValue class).
A couple of thoughts come to mind:

• Why are you calling GetAsUint64 on a 32-bit register in the first place?
• GetAsUint64 (or GetAsUInt32 for that matter) doesn't seem to be very endian-aware. Maybe it should be?
• It could be that what you need to do is simply not possible to do sanely with the current RegisterValue interface. If that's the case, we need to change the interface.

[clayborg]

source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp

659–660 ↗

(On Diff #69952)

Please initialize these. Note that "byte_size" will be used with an random value since it isn't initialized in the else clause below.

673 ↗

(On Diff #69952)

set "byte_size" in else clause.

808 ↗

(On Diff #69952)

init nullptr please.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2137

There is coded that can do what you want in:

lldb::offset_t
DataExtractor::CopyByteOrderedData (lldb::offset_t src_offset,
                     lldb::offset_t src_len,
                     void *dst, 
                     lldb::offset_t dst_len,
                     lldb::ByteOrder dst_byte_order) const;

It lets you say "I want to copy 4 big endian bytes over into 8 little endian bytes. It will pad and do the right thing as long as the source data is the smaller or the same size as the destination". We can probably make this function available as a static function where you specify the source "void *", the src_len, and the src_byte_order, and the dst void *, dst_len and dst_byte_order. Then you can call this function.

[nitesh.jain]

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

1940

The GetAsUint64 work well on 32 bit little endian system since 32 bit data is copy at lower address in SetBytes which is valid for little endian but not on 32 bit big endian system. The Goal is to GetAsUint64 work for 32 bit big endian data type. We can do following change inorder to make it work for RegisterValue type eTypeBytes

uint64_t
RegisterValue::GetAsUInt64 (uint64_t fail_value, bool *success_ptr) const
{

if (success_ptr)
    *success_ptr = true;
switch (m_type)
{
    ...
    ...
    ...
    case eTypeBytes:
    {
        switch (buffer.length)
        {
        default:    break;
        case 1:
        case 2:
        case 4:
        case 8:     return *(const uint64_t *)buffer.bytes;
        }
    }
    break;
}
if (success_ptr)
    *success_ptr = false;
return fail_value;

}

We can modify the case for buffer.length = 4/2/1

switch (buffer.length) {

case 1:  return *(const uint8_t *)buffer.bytes;
case 2:  return *(const uint16_t *)buffer.bytes;
case 4:  return *(const uint32_t *)buffer.bytes;

  ...
  ...
  ...
 }

[nitesh.jain]

Will submit separate patch for Floating point register read/write and ptrace changes.

[clayborg]

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???

source/Core/DataExtractor.cpp

949–958

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.

1024–1026

Should probably have this call the above new function so we don't duplicate functionality.

[nitesh.jain]

In D24124#543823, @clayborg wrote:

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???

The issue was in RegisterValue::GetUInt64 function returning incorrect value for register of size 4/2/1 byte on 32 bit big endian system. We have modify it to return value based on register size which will fix the register read/write problem on 32 bit big endian system.

[labath]

It definitely looks cleaner than the original version. I'm fine with this if @clayborg is.

[clayborg]

Much better.

[Eugene.Zelenko]

Looks like patch was not committed. Please rebase with trunk and run Clang-format.

include/lldb/Core/DataExtractor.h

public:
   ///     Returns the number of bytes that were copied, or zero if
   ///     anything goes wrong.
   //------------------------------------------------------------------
   lldb::offset_t CopyByteOrderedData(lldb::offset_t src_offset,
                                      lldb::offset_t src_len, void *dst,
                                      lldb::offset_t dst_len,
                                      lldb::ByteOrder dst_byte_order) const;
 
+  //------------------------------------------------------------------------
+  /// copy a dst_len bytes from a *src_void_ptr and ensure the copied
+  /// data is treated as a value that can be swapped to match the
+  /// specified byte order.
+  ///
+  /// For values that are larger than the supported integer sizes,
+  /// this function can be used to extract data in a specified byte
+  /// order. It can also be used to copy a smaller integer value from
+  /// to a larger value. The extra bytes left over will be padded
+  /// correctly according to src_byte_order and the dst_byte_order.
+  /// This can be very handy when say copying a partial data value
+  /// into a register.
+  /// 
+  /// @param[in] src_void_ptr
+  ///     A pointer to the source buffer from where endian data need to be copy
+  ///
+  /// @param[in] src_len
+  ///     The length of the endian data to copy from source buffer
+  ///
+  /// @param[in] src_byte_order
+  ///     The byte order that the endian value should be in a source buffer
+  ///
+  /// @param[out] dst_void_ptr
+  ///     A pointer to the destination buffer to place endian data
+  ///
+  /// @param[in] dst_len
+  ///     The number of bytes to be copied to the destination buffer
+  ///
+  /// @param[in] dst_byte_order
+  ///     The byte order that the endian value should be in a destination buffer
+  ///
+  /// @return
+  ///     Returns the number of bytes that were copied , or zero if 
+  ///     anything goes wrong
+  //------------------------------------------------------------------------
+  static lldb::offset_t CopyByteOrderedData(void *src_void_ptr,
+                                            lldb::offset_t src_len,
+                                            lldb::ByteOrder src_byte_order,
+                                            void *dst_void_ptr,
+                                            lldb::offset_t dst_len,
+                                            lldb::ByteOrder dst_byte_order);
+
   //------------------------------------------------------------------
   /// Get the data end pointer.
   ///
   /// @return
   ///     Returns a pointer to the next byte contained in this
   ///     object's data, or nullptr of there is no data in this object.
   //------------------------------------------------------------------
   const uint8_t *GetDataEnd() const { return m_end; }

source/Core/DataExtractor.cpp

lldb::offset_t DataExtractor::CopyData(offset_t offset, offset_t length,
   const uint8_t *src = PeekData(offset, length);
   if (src) {
     ::memcpy(dst, src, length);
     return length;
   }
   return 0;
 }
 
+lldb::offset_t DataExtractor::CopyByteOrderedData(void *src_void_ptr,
+                                                  offset_t src_len,
+                                                  ByteOrder src_byte_order,
+                                                  void *dst_void_ptr,
+                                                  offset_t dst_len,
+                                                  ByteOrder dst_byte_order) {
+
+  // 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);
+
+  uint8_t* dst = (uint8_t*)dst_void_ptr;
+  const uint8_t* src = (const uint8_t *)src_void_ptr;
+  if (src) {
+    if (dst_len >= src_len) {
+      // We are copying the entire value from src into dst.
+      // Calculate how many, if any, zeroes we need for the most
+      // significant bytes if "dst_len" is greater than "src_len"...
+      const size_t num_zeroes = dst_len - src_len;
+      if (dst_byte_order == eByteOrderBig) {
+        // Big endian, so we lead with zeroes...
+        if (num_zeroes > 0)
+          ::memset(dst, 0, num_zeroes);
+        // Then either copy or swap the rest
+        if (src_byte_order == eByteOrderBig) {
+          ::memcpy(dst + num_zeroes, src, src_len);
+        } else {
+          for (uint32_t i = 0; i < src_len; ++i)
+            dst[i + num_zeroes] = src[src_len - 1 - i];
+        }
+      } else {
+        // Little endian destination, so we lead the value bytes
+        if (src_byte_order == eByteOrderBig) {
+          for (uint32_t i = 0; i < src_len; ++i)
+            dst[i] = src[src_len - 1 - i];
+        } else {
+          ::memcpy(dst, src, src_len);
+        }
+        // And zero the rest...
+        if (num_zeroes > 0)
+          ::memset(dst + src_len, 0, num_zeroes);
+      }
+      return src_len;
+    } else {
+      // We are only copying some of the value from src into dst..
+
+      if (dst_byte_order == eByteOrderBig) {
+        // Big endian dst
+        if (src_byte_order == eByteOrderBig) {
+          // Big endian dst, with big endian src
+          ::memcpy(dst, src + (src_len - dst_len), dst_len);
+        } else {
+          // Big endian dst, with little endian src
+          for (uint32_t i = 0; i < dst_len; ++i)
+            dst[i] = src[dst_len - 1 - i];
+        }
+      } else {
+        // Little endian dst
+        if (src_byte_order == eByteOrderBig) {
+          // Little endian dst, with big endian src
+          for (uint32_t i = 0; i < dst_len; ++i)
+            dst[i] = src[src_len - 1 - i];
+        } else {
+          // Little endian dst, with big endian src
+          ::memcpy(dst, src, dst_len);
+        }
+      }
+      return dst_len;
+    }
+  }
+  return 0;
+}
+
 // Extract data and swap if needed when doing the copy
 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
   if (!ValidOffsetForDataOfSize(src_offset, src_len))
     assert(ValidOffsetForDataOfSize(src_offset, src_len));
   assert(src_len > 0);
   assert(m_byte_order == eByteOrderBig || m_byte_order == eByteOrderLittle);
 
   // Validate the destination info
   assert(dst_void_ptr != nullptr);

source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp

Error NativeRegisterContextLinux::DoReadRegisterValue(uint32_t offset,
 
   long data;
   Error error = NativeProcessLinux::PtraceWrapper(
       PTRACE_PEEKUSER, m_thread.GetID(), reinterpret_cast<void *>(offset),
       nullptr, 0, &data);
 
   if (error.Success())
     // First cast to an unsigned of the same size to avoid sign extension.
-    value.SetUInt64(static_cast<unsigned long>(data));
+    value.SetUInt(static_cast<unsigned long>(data), size);
 
   if (log)
     log->Printf("NativeRegisterContextLinux::%s() reg %s: 0x%lx", __FUNCTION__,
                 reg_name, data);
 
   return error;
 }
 

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

GDBRemoteCommunicationServerLLGS::Handle_P(StringExtractorGDBRemote &packet) {
   // Hence the reg_size may not be same as reg_info->bytes_size
   if ((reg_size != reg_info->byte_size) &&
       !(reg_info->dynamic_size_dwarf_expr_bytes)) {
     return SendIllFormedResponse(packet, "P packet register size is incorrect");
   }
 
   // Build the reginfos response.
   StreamGDBRemote response;
-
-  RegisterValue reg_value(reg_bytes, reg_size, process_arch.GetByteOrder());
+  uint64_t dst_value;
+  RegisterValue reg_value;
+  lldb::ByteOrder byte_order = lldb::eByteOrderInvalid;
+  // The host will always send data in target byte order
+  // Hence source byte order will be same as destination byte order
+  m_debugged_process_sp->GetByteOrder(byte_order);
+  
+  DataExtractor::CopyByteOrderedData(reg_bytes, reg_size,
+                                     byte_order, &dst_value,
+                                     sizeof(dst_value), byte_order);
+  reg_value.SetUInt(dst_value, reg_size);
   Error error = reg_context_sp->WriteRegister(reg_info, reg_value);
   if (error.Fail()) {
     if (log)
       log->Printf("GDBRemoteCommunicationServerLLGS::%s failed, write of "
                   "requested register %" PRIu32 " (%s) failed: %s",
                   __FUNCTION__, reg_index, reg_info->name, error.AsCString());
     return SendErrorResponse(0x32);
   }
GDBRemoteCommunicationServerLLGS::Handle_memory_read(
 
   // Read the address.  Punting on validation.
   // FIXME replace with Hex U64 read with no default value that fails on failed
   // read.
   const lldb::addr_t read_addr = packet.GetHexMaxU64(false, 0);
 
   // Validate comma.
   if ((packet.GetBytesLeft() < 1) || (packet.GetChar() != ','))
     return SendIllFormedResponse(packet, "Comma sep missing in m packet");
 
   // Get # bytes to read.
   if (packet.GetBytesLeft() < 1)
     return SendIllFormedResponse(packet, "Length missing in m packet");
 
   const uint64_t byte_count = packet.GetHexMaxU64(false, 0);
   if (byte_count == 0) {
     if (log)
       log->Printf("GDBRemoteCommunicationServerLLGS::%s nothing to read: "