Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
sagar updated this revision to Diff 41232.
sagar added a comment.
Addressed review comments
Repository:
rL LLVM
http://reviews.llvm.org/D14633
Files:
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
===
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
@@ -1376,6 +1376,9 @@
{
GPR_linux_mips regs;
::memset(, 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 *)) + offset), 8,
GetByteOrder());
Error error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGS,
m_thread.GetID(), NULL, , sizeof regs);
if (error.Success())
{
Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
===
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
@@ -1376,6 +1376,9 @@
{
GPR_linux_mips regs;
::memset(, 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 *)) + offset), 8, GetByteOrder());
Error error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGS, m_thread.GetID(), NULL, , sizeof regs);
if (error.Success())
{
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
tberghammer accepted this revision. tberghammer added a comment. 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() Repository: rL LLVM http://reviews.llvm.org/D14633 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
sagar added a comment. 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 Repository: rL LLVM http://reviews.llvm.org/D14633 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
sagar added a comment. 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:;01:;02:;03:;04:;05:;06:;07:;08:;09:; > 0a:;0b:;0c:;0d:;0e:;0f:;10:;11:;12:;13:; > 14:;15:;16:;17:;18:;19:;1a:;1b:;1c:;1d:; > 1e:;1f:;20:;21:;22:;23:;24:;25:;26:;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 Repository: rL LLVM http://reviews.llvm.org/D14633 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
tberghammer added a comment. 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. Repository: rL LLVM http://reviews.llvm.org/D14633 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
clayborg added a comment.
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..
Repository:
rL LLVM
http://reviews.llvm.org/D14633
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
sagar added a comment. 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 (, 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 Repository: rL LLVM http://reviews.llvm.org/D14633 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
clayborg requested changes to this revision. This revision now requires changes to proceed. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1378 @@ -1377,2 +1377,3 @@ GPR_linux_mips regs; +lldb_private::ArchSpec arch; ::memset(, 0, sizeof(GPR_linux_mips)); tberghammer wrote: > 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. Remove this. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1382 @@ -1379,1 +1381,3 @@ +// 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 *)) + offset), 8, arch.GetByteOrder()); Error error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGS, m_thread.GetID(), NULL, , sizeof regs); Just call the member function GetByteOrder() that is built into NativeRegisterContextLinux: ``` value.SetBytes((void *)(((unsigned char *)) + offset), 8, GetByteOrder()); ``` Repository: rL LLVM http://reviews.llvm.org/D14633 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
sagar updated this revision to Diff 40256.
sagar added a comment.
Addressed review comments.
Repository:
rL LLVM
http://reviews.llvm.org/D14633
Files:
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
===
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
@@ -1375,7 +1375,11 @@
RegisterValue )
{
GPR_linux_mips regs;
+lldb_private::ArchSpec arch;
::memset(, 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 *)) + offset), 8,
arch.GetByteOrder());
Error error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGS,
m_thread.GetID(), NULL, , sizeof regs);
if (error.Success())
{
Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
===
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
@@ -1375,7 +1375,11 @@
RegisterValue )
{
GPR_linux_mips regs;
+lldb_private::ArchSpec arch;
::memset(, 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 *)) + offset), 8, arch.GetByteOrder());
Error error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGS, m_thread.GetID(), NULL, , sizeof regs);
if (error.Success())
{
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
sagar added a comment. > 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, > 0x8000 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 0x 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. Repository: rL LLVM http://reviews.llvm.org/D14633 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
tberghammer added a comment.
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 )
{
GPR_linux_mips regs;
::memset(, 0, sizeof(GPR_linux_mips));
Error error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGS,
m_thread.GetID(), NULL, , sizeof regs);
if (error.Success())
{
lldb_private::ArchSpec arch;
if (m_thread.GetProcess()->GetArchitecture(arch))
value.SetUInt(*(uint64_t*)(((uint8_t*)) + offset),
arch.GetAddressByteSize());
else
error.SetErrorString("failed to get architecture");
}
return error;
}
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1378
@@ -1377,2 +1377,3 @@
GPR_linux_mips regs;
+lldb_private::ArchSpec arch;
::memset(, 0, sizeof(GPR_linux_mips));
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.
Repository:
rL LLVM
http://reviews.llvm.org/D14633
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
dsanders added a comment. > 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, 0x8000 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 Repository: rL LLVM http://reviews.llvm.org/D14633 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14633: [LLDB][MIPS] Clear bug 25194 - LLDB-Server Assertion raised when single stepping on MIPS
clayborg requested changes to this revision. clayborg added a comment. I agree with tberghammer. Repository: rL LLVM http://reviews.llvm.org/D14633 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
