[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 ↗

(On Diff #69952)

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 ↗

(On Diff #69952)

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 ↗

(On Diff #69952)

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

1845 ↗

(On Diff #69952)

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 ↗

(On Diff #71360)

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.

lldb/trunk/source/Core/RegisterValue.cpp

uint64_t RegisterValue::GetAsUInt64(uint64_t fail_value,
   case eTypeDouble:
   case eTypeLongDouble:
     return m_scalar.ULongLong(fail_value);
   case eTypeBytes: {
     switch (buffer.length) {
     default:
       break;
     case 1:
+      return *(const uint8_t *)buffer.bytes;
     case 2:
+      return *(const uint16_t *)buffer.bytes;
     case 4:
+      return *(const uint32_t *)buffer.bytes;
     case 8:
       return *(const uint64_t *)buffer.bytes;
     }
   } break;
   }
   if (success_ptr)
     *success_ptr = false;
   return fail_value;

lldb/trunk/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;
 }