This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by nitesh.jain on Sep 1 2016, 3:46 AM.

Details

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.

Diff Detail

Event Timeline

nitesh.jain updated this revision to Diff 69952.Sep 1 2016, 3:46 AM
nitesh.jain retitled this revision from to [LLDB][MIPS] Fix register read/write for big endian.
nitesh.jain updated this object.
nitesh.jain added reviewers: clayborg, labath.
nitesh.jain updated this object.Sep 1 2016, 3:46 AM
nitesh.jain updated this object.
labath requested changes to this revision.Sep 1 2016, 4:34 AM
labath edited edge metadata.

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.

This revision now requires changes to proceed.Sep 1 2016, 4:34 AM
nitesh.jain added inline comments.Sep 1 2016, 8:08 AM
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 added inline comments.Sep 1 2016, 9:49 AM
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 requested changes to this revision.Sep 1 2016, 10:15 AM
clayborg edited edge metadata.
clayborg added inline comments.
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 retitled this revision from [LLDB][MIPS] Fix register read/write for big endian to [LLDB][MIPS] Fix register read/write for 32 bit big endian system.
nitesh.jain updated this object.
nitesh.jain edited edge metadata.
nitesh.jain added inline comments.Sep 15 2016, 2:14 AM
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 marked 2 inline comments as done.Sep 15 2016, 2:15 AM

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

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 updated this object.
nitesh.jain edited edge metadata.

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.

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

clayborg accepted this revision.Sep 21 2016, 10:03 AM
clayborg edited edge metadata.

Much better.

labath accepted this revision.Sep 21 2016, 12:26 PM
labath edited edge metadata.
This revision is now accepted and ready to land.Sep 21 2016, 12:26 PM

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

This revision was automatically updated to reflect the committed changes.