[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

1844

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

1844

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

1844

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

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

set "byte_size" in else clause.

808

init nullptr please.

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

1845

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

1530

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 ↗

(On Diff #71360)

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 ↗

(On Diff #71360)

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.

source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp

NativeRegisterContextLinux::DoReadRegisterValue(uint32_t offset,
     Log *log (ProcessPOSIXLog::GetLogIfAllCategoriesSet (POSIX_LOG_REGISTERS));
 
     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;
 }
 
 Error

source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h

public:
         uint32_t
         NumSupportedHardwareWatchpoints () override;
 
         static bool
         IsMSAAvailable();
 
     protected:
         Error
-        DoReadRegisterValue(uint32_t offset,
+        DoReadRegister_SR_Config (uint32_t offset,
                                   const char* reg_name,
                                   uint32_t size,
-                            RegisterValue &value) override;
+                                  RegisterValue &value);
+
+        uint32_t
+        GetPtraceRegisterOffset (uint32_t reg_index,
+                                 const RegisterInfo *const reg_info);
+
+        Error
+        ReadRegisterRaw (uint32_t reg_index,
 
         Error
-        DoWriteRegisterValue(uint32_t offset,
-                             const char* reg_name,
-                             const RegisterValue &value) override;
+        WriteRegisterRaw (uint32_t reg_index,
 
         Error
         DoReadWatchPointRegisterValue(lldb::tid_t tid, void* watch_readback);
 
         Error
         DoWriteWatchPointRegisterValue(lldb::tid_t tid, void* watch_readback);
 
         bool

source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp

 #include "lldb/Host/Host.h"
 #include "lldb/Core/EmulateInstruction.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "Plugins/Process/Linux/NativeProcessLinux.h"
 #include "Plugins/Process/Linux/Procfs.h"
 #include "Plugins/Process/Utility/RegisterContextLinux_mips64.h"
 #include "Plugins/Process/Utility/RegisterContextLinux_mips.h"
+
 #define NT_MIPS_MSA 0x600
 #define CONFIG5_FRE (1 << 8)
 #define SR_FR (1 << 26)
+#define FPR_BASE        32
+#define PC              64
+#define CAUSE           65
+#define BADVADDR        66
+#define MMHI            67
+#define MMLO            68
 #define NUM_REGISTERS 32
 
 #include <sys/ptrace.h>
 #include <asm/ptrace.h>
 
 #ifndef PTRACE_GET_WATCH_REGS
 enum pt_watch_style
 {
NativeRegisterContextLinux_mips64::WriteRegister (const RegisterInfo *reg_info, const RegisterValue &reg_value)
     if (IsMSA(reg_index) && !IsMSAAvailable())
     {
         error.SetErrorString ("MSA not available on this processor");
         return error;
     }
 
     if (IsFPR(reg_index) || IsMSA(reg_index))
     {
-        uint8_t *dst;
+        uint8_t *dst, byte_size;
         uint64_t *src;
 
         // Initialise the FP and MSA buffers by reading all co-processor 1 registers
         ReadCP1();
 
         if (IsFPR(reg_index))
         {
             assert (reg_info->byte_offset < sizeof(UserArea));
             dst = (uint8_t *)&m_fpr + reg_info->byte_offset - (sizeof(m_gpr));
+            byte_size = IsFR0 () ? 4 : 8;
         }
         else
         {
             assert (reg_info->byte_offset < sizeof(UserArea));
             dst = (uint8_t *)&m_msa + reg_info->byte_offset - (sizeof(m_gpr) + sizeof(m_fpr));
         }
-        switch (reg_info->byte_size)
+        switch (byte_size)
         {
             case 4:
                 *(uint32_t *)dst = reg_value.GetAsUInt32();
                 break;
             case 8:
                 *(uint64_t *)dst = reg_value.GetAsUInt64();
                 break;
             case 16:
NativeRegisterContextLinux_mips64::WriteAllRegisterValues (const lldb::DataBufferSP &data_sp)
     return error;
 }
 
 Error
 NativeRegisterContextLinux_mips64::ReadCP1()
 {
     Error error;
 
     uint8_t *src, *dst;
 
     lldb::ByteOrder byte_order = GetByteOrder();
 
-    uint32_t IsBigEndian = (byte_order == lldb::eByteOrderBig);
+    bool IsBigEndian = (byte_order == lldb::eByteOrderBig);
 
     if (IsMSAAvailable())
     {
         error = NativeRegisterContextLinux::ReadRegisterSet(&m_iovec, sizeof(MSA_linux_mips), NT_MIPS_MSA);
         src = (uint8_t *)&m_msa + (IsBigEndian * 8);
         dst = (uint8_t *)&m_fpr;
         for ( int i = 0; i < NUM_REGISTERS; i++)
         {
NativeRegisterContextLinux_mips64::ReadCP1()
     else
     {
         error = NativeRegisterContextLinux::ReadFPR();
     }
 
     // TODO: Add support for FRE
     if (IsFR0())
     {
-         src = (uint8_t *)&m_fpr + 4 + (IsBigEndian * 4);
-         dst = (uint8_t *)&m_fpr + 8 + (IsBigEndian * 4);
+         src = (uint8_t *)&m_fpr + (!IsBigEndian) * 4;
+         dst = (uint8_t *)&m_fpr + 8;
          for (int i = 0; i < (NUM_REGISTERS / 2); i++)
          {
               // copy odd single from top of neighbouring even double
-              *(uint32_t *) dst = *(uint32_t *) src;
+              // In case of little endian, 32 bit LSB contain even floating point register
+              // and 32 bit MSB contain odd floating point register
+              // vice-versa for big endian 
+              *(uint32_t *) dst = *(uint32_t *) src; 
+
+              if (IsBigEndian)
+                   // Copy 32 bit MSB to 32 bit LSB 
+                  *(uint32_t *) src = *(uint32_t *)(src + 4);
+
               src = src + 16;
               dst = dst + 16;
          }
     }
 
     return error;
 }
 
 Error
 NativeRegisterContextLinux_mips64::WriteCP1()
 {
     Error error;
 
     uint8_t *src, *dst;
 
     lldb::ByteOrder byte_order = GetByteOrder();
 
-    uint32_t IsBigEndian = (byte_order == lldb::eByteOrderBig);
+    bool IsBigEndian = (byte_order == lldb::eByteOrderBig);
 
     // TODO: Add support for FRE
     if (IsFR0())
     {
-        src = (uint8_t *)&m_fpr + 8 + (IsBigEndian * 4);
-        dst = (uint8_t *)&m_fpr + 4 + (IsBigEndian * 4);
+        dst = (uint8_t *)&m_fpr + (!IsBigEndian) * 4;
+        src = dst + 8 - (!IsBigEndian) * 4;
         for (int i = 0; i < (NUM_REGISTERS / 2); i++)
         {
              // copy odd single to top of neighbouring even double
-             *(uint32_t *) dst = *(uint32_t *) src;
+             if (IsBigEndian)
+                  // Copy 32 bit LSB to 32 bit MSB
+                 *(uint32_t *) (dst + 4) = *(uint32_t *) dst;
+
+             *(uint32_t *)dst = *(uint32_t *)src;
+             
              src = src + 16;
-             dst = dst + 16;
+             dst = src + 16;
         }
     }
 
     if (IsMSAAvailable())
     {
         dst = (uint8_t *)&m_msa + (IsBigEndian * 8);
         src = (uint8_t *)&m_fpr;
         for (int i = 0; i < NUM_REGISTERS; i++)
if (!num_valid)
             default:
                 if(log)
                     log->Printf("NativeRegisterContextLinux_mips64::%s Error: Unrecognized watch register style", __FUNCTION__);
         }
         return 0;
     }
     return num_valid;
 }
+
+uint32_t
+NativeRegisterContextLinux_mips64::GetPtraceRegisterOffset (uint32_t reg_index, 
+                                                            const RegisterInfo *const reg_info)
+{
+    // In case of MIPS, the PTRACE_PEEKUSER/PTRACE_POKEUSER 
+    // take register number has an offset
+    // Apart from GPR registers , the offset for other registers are
+    // FPR_BASE        32
+    // PC              64
+    // CAUSE           65
+    // BADVADDR        66
+    // MMHI            67
+    // MMLO            68
+
+    static_assert (dwarf_zero_mips == dwarf_zero_mips64 &&
+                   dwarf_pc_mips == dwarf_pc_mips64,
+                   "MIPS and MIPS64 dwarf no. mismatch");
+
+    if (reg_index < dwarf_sr_mips)
+        return reg_index;
+
+    switch (reg_info->kinds[lldb::eRegisterKindDWARF])
+    {
+        case dwarf_pc_mips:
+            return PC;
+        case dwarf_sr_mips:
+        case dwarf_config5_mips:
+        case dwarf_config5_mips64:
+            return reg_info->byte_offset;
+        case dwarf_cause_mips:
+            return CAUSE;
+        case dwarf_bad_mips:
+            return BADVADDR;
+        case dwarf_hi_mips:
+            return MMHI;
+        case dwarf_lo_mips:
+            return MMLO;
+        default:
+            assert (false && "Invalid Register Kinds");
+    }
+    return 0;
+}
+
 Error
-NativeRegisterContextLinux_mips64::DoReadRegisterValue(uint32_t offset,
-                                                       const char* reg_name,
-                                                       uint32_t size,
-                                                       RegisterValue &value)
+NativeRegisterContextLinux_mips64::ReadRegisterRaw (uint32_t reg_index,
 {
-    GPR_linux_mips regs;
-    ::memset(&regs, 0, sizeof(GPR_linux_mips));
+    const RegisterInfo *const reg_info = GetRegisterInfoAtIndex (reg_index);
 
-    // Clear all bits in RegisterValue before writing actual value read from ptrace to avoid garbage value in 32-bit MSB 
-    value.SetBytes((void *)(((unsigned char *)&regs) + offset), 8, GetByteOrder());
-    Error error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGS, m_thread.GetID(), NULL, &regs, sizeof regs);
-    if (error.Success())
-    {
-        lldb_private::ArchSpec arch;
-        if (m_thread.GetProcess()->GetArchitecture(arch))
-        {
-            void* target_address = ((uint8_t*)&regs) + offset + 4 * (arch.GetMachine() == llvm::Triple::mips);
-            uint32_t target_size;
-            if ((::strcmp(reg_name, "sr") == 0) || (::strcmp(reg_name, "cause") == 0) || (::strcmp(reg_name, "config5") == 0))
-                target_size = 4;
-            else
-                target_size = arch.GetFlags() & lldb_private::ArchSpec::eMIPSABI_O32 ? 4 : 8;
-            value.SetBytes(target_address, target_size, arch.GetByteOrder());
-        }
-        else
-            error.SetErrorString("failed to get architecture");
-    }
-    return error;
+    if (!reg_info)
+        return Error ("register %" PRIu32 " not found", reg_index);
+
+    uint32_t offset = GetPtraceRegisterOffset (reg_index, reg_info);
+    
+    if ((reg_index == dwarf_sr_mips) || (strcmp(reg_info->name, "config5") == 0))
+        return DoReadRegister_SR_Config (offset, reg_info->name,
+                                         reg_info->byte_size, value);
+
+    return DoReadRegisterValue (offset, reg_info->name,
+                                reg_info->byte_size, value);
+
+Error
+NativeRegisterContextLinux_mips64::WriteRegisterRaw (uint32_t reg_index,
+                                                     const RegisterValue &value)
+{
+    const RegisterInfo *const reg_info = GetRegisterInfoAtIndex (reg_index);
+
+    if (!reg_info)
+        return Error ("register %" PRIu32 " not found", reg_index);
+
+    if (reg_info->invalidate_regs)
+        assert (false && "In MIPS, reg_info->invalidate_regs is unhandled");
+
+    uint32_t offset = GetPtraceRegisterOffset (reg_index, reg_info);
+    return DoWriteRegisterValue (offset, reg_info->name, value);
 }
 
 Error
-NativeRegisterContextLinux_mips64::DoWriteRegisterValue(uint32_t offset,
+NativeRegisterContextLinux_mips64::DoReadRegister_SR_Config (uint32_t offset,
                                                              const char* reg_name,
-                                                        const RegisterValue &value)
+                                                             uint32_t size,
+                                                             RegisterValue &value)
 {
     GPR_linux_mips regs;
-    Error error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGS, m_thread.GetID(), NULL, &regs, sizeof regs);
+    ::memset(&regs, 0, sizeof(GPR_linux_mips));
+
+    Error error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGS,
+                                                    m_thread.GetID(), NULL,
+                                                    &regs, sizeof regs);
     if (error.Success())
     {
         lldb_private::ArchSpec arch;
         if (m_thread.GetProcess()->GetArchitecture(arch))
         {
-            ::memcpy((void *)(((unsigned char *)(&regs)) + offset), value.GetBytes(), arch.GetFlags() & lldb_private::ArchSpec::eMIPSABI_O32 ? 4 : 8);
-            error = NativeProcessLinux::PtraceWrapper(PTRACE_SETREGS, m_thread.GetID(), NULL, &regs, sizeof regs);
+            void* target_address = ((uint8_t*)&regs) + offset +
+                                    4 * (arch.GetMachine() == llvm::Triple::mips);
+            value.SetUInt (*(uint32_t *)target_address, size);
         }
         else
             error.SetErrorString("failed to get architecture");
     }
     return error;
 }
 
 Error

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

GDBRemoteCommunicationServerLLGS::Handle_qRegisterInfo (StringExtractorGDBRemote &packet)
     // Return the end of registers response if we've iterated one past the end of the register set.
     if (reg_index >= reg_context_sp->GetUserRegisterCount ())
         return SendErrorResponse (69);
 
     const RegisterInfo *reg_info = reg_context_sp->GetRegisterInfoAtIndex(reg_index);
     if (!reg_info)
         return SendErrorResponse (69);
 
     // Build the reginfos response.
     StreamGDBRemote response;
 
     response.PutCString ("name:");
     response.PutCString (reg_info->name);
     response.PutChar (';');
 
     if (reg_info->alt_name && reg_info->alt_name[0])
     {
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 value;
+    value = reg_size == 4 ? *(uint32_t *)reg_bytes : *(uint64_t *)reg_bytes;
+
+    RegisterValue reg_value;
+    reg_value.SetUInt (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);
     }