[LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS

[slthakur]

Summary

This patch will clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS. The problem was that while emulating instructions, old and new pc values would have garbage value in their upper 32 bits. Therefore checking if pc was changed (old_pc == new_pc) would always return false, because of which pc was not getting updated.

/* If we haven't changed the PC, change it here */
if (old_pc == new_pc)
{
    new_pc += 4;
    Context context;
    if (!WriteRegisterUnsigned (context, eRegisterKindDWARF, >dwarf_pc_mips, new_pc))
        return false;
}

[tberghammer]

You are created a new ReadRegisterUnsigned method in the derived class what will hide a method from the base class with the same name but with different return type. I think from code health perspective it is a very bad idea and will lead to surprising differences between calling ReadRegisterUnsigned on an EmulateInstruction* or on an EmulateInstructionMIPS* with the same underlying object.

The route cause of your problem is that ReadRegisterUnsigned returns a value where the 32 MSB is garbage while the expected behavior is to zero out those bits (it works on i386 and arm AFAIK). You should find out why that is happening and fix the root cause of it what will fix your single stepping issue as well.

[clayborg]

I agree with tberghammer.

[dsanders]

The route cause of your problem is that ReadRegisterUnsigned returns a value where the 32 MSB is garbage while the
expected behavior is to zero out those bits (it works on i386 and arm AFAIK). You should find out why that is happening
and fix the root cause of it what will fix your single stepping issue as well.

I don't know if this is important to problem or not, but I think I should mention that the expectation of zero bits disagrees with MIPS hardware. MIPS hardware would expect the 32 MSB to be a sign-extension of the 32 LSB.

As part of our forward compatibility strategy, we widen registers (e.g. from MIPS32 to MIPS64) by sign extending the values produced by every operation that existed before and add additional operations as needed (e.g. MIPS64 adds 'lwu' which is a unsigned 32-bit load). Admittedly it's a bit unintuitive for an unsigned 32-bit value from a MIPS32 binary to be represented in a 64-bit register as, for example, 0xffffffff80000000 but the debugger shouldn't normally admit to the existence of the extra bits when debugging 32-bit code so the user won't normally see this.

Another result of this is that our address space grows at both ends and the previous address space is in the middle like so:

MIPS64's extra user space | MIPS32's user space | MIPS32's kernel space | MIPS64's extra kernel space.

Hope this helps

[slthakur]

Addressed review comments.

[slthakur]

Admittedly it's a bit unintuitive for an unsigned 32-bit value from a MIPS32 binary to be represented in a 64-bit register as, for example, 0xffffffff80000000 but the debugger shouldn't normally admit to the existence of the extra bits when debugging 32-bit code so the user won't normally see this.

In case of MIPS32 the 0xffffffff value in 32 MSB will always be truncated out of RegisterValue, since SetBytes() will only write lower 4 bytes of the value into RegisterValue. The problem here is that RegisterValue initially contains garbage value. Therefore clearing all bits in RegisterValue before writing actual 32-bit value solves the problem.

[tberghammer]

Looks much better, but I think the root cause of your problem is that you are using RegisterValue::SetBytes instead of RegisterValue::SetUInt. I would suggest to use a code like this (I don't have a mips environment at the moment to try it out):

Error
NativeRegisterContextLinux_mips64::DoReadRegisterValue(uint32_t offset,
                                                       const char* reg_name,
                                                       uint32_t size,
                                                       RegisterValue &value)
{
    GPR_linux_mips 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))
            value.SetUInt(*(uint64_t*)(((uint8_t*)&regs) + offset), arch.GetAddressByteSize());
        else
            error.SetErrorString("failed to get architecture");
    }
    return error;
}

source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp

1378

You use this variable without initializing it. You can call SetBytes with an explicit byte order value (e.g. eByteOrderLittle) as it isn't matter during 0 initialization.

[clayborg]

source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp

1378

Remove this.

1381

Just call the member function GetByteOrder() that is built into NativeRegisterContextLinux:

value.SetBytes((void *)(((unsigned char *)&regs) + offset), 8, GetByteOrder());

[slthakur]

Hi @tberghammer,

I tried using RegisterValue::SetUInt() instead of RegisterValue::SetBytes(). When using RegisterValue::SetUInt() all register values we get are zero in case of mips32 big endian machine. The GDBRemoteCommunicationServerLLGS::SendStopReplyPacketForThread() and GDBRemoteCommunicationServerLLGS::Handle_p() function implementations retrieve a pointer to register value by calling RegisterValue::GetBytes() which in turn retrieves a pointer to value from Scalar::GetBytes() in cases if type of RegisterValue is eTypeUInt*. But Scalar::GetBytes() does not seem to handle endianness correctly. As a workaround I made RegisterValue::GetBytes() retrieve the pointer to register value from its bytes buffer for all cases and also called SetBytes from RegisterValue::SetUInt() like this :

diff --git a/source/Core/RegisterValue.cpp b/source/Core/RegisterValue.cpp
index d4ba998..ec8bfb8 100644
--- a/source/Core/RegisterValue.cpp
+++ b/source/Core/RegisterValue.cpp
@@ -822,7 +822,7 @@ RegisterValue::GetBytes () const
case eTypeUInt128:
case eTypeFloat:
case eTypeDouble:
- case eTypeLongDouble: return m_scalar.GetBytes();
+ case eTypeLongDouble:
case eTypeBytes: return buffer.bytes;
}
return NULL;
@@ -841,7 +841,7 @@ RegisterValue::GetBytes ()
case eTypeUInt128:
case eTypeFloat:
case eTypeDouble:
- case eTypeLongDouble: return m_scalar.GetBytes();
+ case eTypeLongDouble:
case eTypeBytes: return buffer.bytes;
}
return NULL;
@@ -896,6 +896,7 @@ RegisterValue::SetUInt (uint64_t uint, uint32_t byte_size)
}
else
return false;
+ SetBytes (&uint, byte_size, GetByteOrder());
return true;
}

If this looks good I will submit a separate patch for the same.
Should we use the above mentioned workaround or use RegisterValue::SetBytes as before? Kindly let me know if you have any other suggestions.

Regards,
Sagar

[tberghammer]

As far as I know the gdb remote protocol says that the registers in the 'p' packet should be displayed in target byte order, but the protocol isn't too well specified (and in my opinion target byte order is a silly decision).

If we accept that the 'p' packet is in target byte order then I think using SetUInt works as intended in the current implementation and then we should fix the client side to interpret the register value as target byte order.

I am also happy with the option of saying that we always send the register values as little endian but in that case my preferred way would be to use SetUInt without changing it's meaning and then doing the byte swapping (if needed) when we write the data into the response buffer (WriteRegisterValueInHexFixedWidth). The reason behind it is that if we want to use the ReadRegister call in lldb-server (e.g. software single stepping) then I would expect it to return the data in target byte order and this change would break this assumption.

Additional info is that the current implementation works well if the client and the server use the same byte order. If we fix the communication to be in little endian then we will still have to fix the server (GDBRemoteRegisterContext::PrivateSetRegisterValue) to handle the case when lldb runs on a big endian machine.

[clayborg]

GDB remote protocol specifies that register values are sent in target byte order. We shouldn't change this. A big endian system should not send things as little endian. That being said, the current register context assumes you have a buffer that can contain all registers and that buffer is encoded using the byte order that is specified in the DataExtractor.

It is also quite silly that the MIPS register context is reading all registers from the ptrace wrapper just to get one single register. RegisterContextDarwin reads all GPRs in a single call and marks all of them valid at once and then extracting a register goes something like:

if (RegisterIsGPR(value.info))
{

ReadGPRsIfWeAlreadyHavent();
// Extract register from buffer.

}
else if (RegiserIsFPU(value.info))
{

ReadFPUsIfWeAlreadyHavent();
// Extract register from buffer.

}

etc..

[slthakur]

Hi,

@tberghammer : For both mips32 and mips64 big endian 'T' packet response contains the register values in target byte order only. But for mips32 big endian when we set the value of the register in RegisterValue using RegisterValue::SetUInt() the upper half of the container in RegisterValue contains zero value and the lower half contains the actual value. And when we fetch a pointer to the container in RegisterValue using RegisterValue::GetBytes() we get the start address of upper half of the container. Therefore while constructing 'T' packet response the function AppendHexValue() in GDBRemoteCommunicationServerLLGS.cpp called from WriteRegisterValueInHexFixedWidth() will read only the next 4 bytes it sees which are all zero. Therefore we get zero values for all registers at the client side:

< 5> send packet: $?#3f
< 680> read packet:
$T17thread:774d;name:step_32eb.elf;threads:774d;jstopinfo:5b7b226e616d65223a22737465705f333265622e656c66222c22726561736f6e223a227369676e616c222c227369676e616c223a32332c22746964223a33303534317d5d;
00:00000000;01:00000000;02:00000000;03:00000000;04:00000000;05:00000000;06:00000000;07:00000000;08:00000000;09:00000000;
0a:00000000;0b:00000000;0c:00000000;0d:00000000;0e:00000000;0f:00000000;10:00000000;11:00000000;12:00000000;13:00000000;
14:00000000;15:00000000;16:00000000;17:00000000;18:00000000;19:00000000;1a:00000000;1b:00000000;1c:00000000;1d:00000000;
1e:00000000;1f:00000000;20:00000000;21:00000000;22:00000000;23:00000000;24:00000000;25:00000000;26:00000000;reason:signal;#47

@clayborg: After this change will submit a separate patch to read all GPRs in a single call for once and then extract the register value as needed in the MIPS register context.

Regards,
Sagar

[tberghammer]

Hi,

I checked out the implementation of RegisterValue and this is my understanding what is happening:

• When we call RegisterValue::SetUInt it will be creating a 32bit unsigned llvm::APInt
• The llvm::APInt will store the data in a uint64_t
• Calling RegisterValue::GetBytes() will return with APInt::getRawData() what will return a pointer to the uint64_t

The problem is that the pointer returned from getRawData isn't pointing to the 32bit data of the underlying storage but it points to the full 64bit storage slot.

I don't know what is the best solution in the current situation but using SetBytes directly and managing the endianness by hand looks like a bad solution for me because then we practically say that SetUInt (and all other similar functions) are not big endian compatible (what should be fixed or they should be removed).

I see 2 option at the moment but none of them is perfect:

• Add a new function to llvm::APInt what returns the pointer to the actual data in case of both endian (possibly the best option)
• Don't use llvm::APInt inside Scalar as we don't need most of it's functionality and just have a big enough byte buffer in RegisterValue

Cheers,
Tamas

[clayborg]

I would prefix the first fix:

Add a new function to llvm::APInt what returns the pointer to the actual data in case of both endian (possibly the best option)

Using llvm::APInt gets us unlimited integer size abilities (support 128 bit ints and higher) along with any math that is required on those integers.

[slthakur]

Hi,

Could we use SetBytes for now for clearing the bug 25194? I have tried using SetBytes(), it does not cause any issue on MIPS for both endian. Once we have a new function to llvm::APInt to access actual data I will revert back to using SetUInt. Kindly let me know if you agree with this.

Regards,
Sagar

[tberghammer]

If you want to get this in with using SetBytes I am fine with it but we should keep an eye on it as I won't be surprised if it will break when somebody try to read out the data from the RegisterValue object with GetUInt()

[slthakur]

Addressed review comments

[slthakur]

Committed in revision 254379

source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp

 }
 Error
 NativeRegisterContextLinux_mips64::DoReadRegisterValue(uint32_t offset,
                                                        const char* reg_name,
                                                        uint32_t size,
                                                        RegisterValue &value)
 {
     GPR_linux_mips regs;
     ::memset(&regs, 0, sizeof(GPR_linux_mips));
+
+    // 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))
             value.SetBytes((void *)(((unsigned char *)&regs) + offset + 4 * (arch.GetMachine() == llvm::Triple::mips)), arch.GetAddressByteSize(), arch.GetByteOrder());
         else
             error.SetErrorString("failed to get architecture");