[Lldb-commits] [PATCH] lldb more on register context arm64

[Jason Molenda]

Yeah, sorry for not looking more closely at the patch earlier.

As to my "so we're going to call RegisterValue::SetUInt64 on that -- I don't see where we'd recognize we only want the lower 32 bits and return those" comment, we may just be lucking out when the target / host are both little endian.  When we ask for w0, we stuff the full 64-bit value of x0 in the RegisterValue object - the RegisterValue object knows that w0 is 32-bits and so it grabs the first 32-bits of the 8-byte data buffer and that happens to be the lower 32-bits of x0 in BE order.  So it works out but it's one of those sneaky problems to find when we're trying to run lldb on a BE PPC system or something. :)


> On Sep 2, 2014, at 12:50 PM, Jason Molenda <jmolenda at apple.com> wrote:
> 
> Thanks Paul.
> 
> Does the ReadRegister method really do the correct thing for the x0/w0 or d0/s0 registers?  If we ask for w0, it's going to realize this is a subreg of x0, and call it self again asking for x0:
> 
>        if (is_subreg)
>        {
>            // Read the full aligned 64-bit register.
>            full_reg = reg_info->invalidate_regs[0];
>        }
> 
> so we're going to call RegisterValue::SetUInt64 on that -- I don't see where we'd recognize we only want the lower 32 bits and return those.  Do you have this running on a live device?  Does this work?
> 
> (lldb) reg write x0 0x12345678abcdef12
> (lldb) reg read x0
>      x0 = 0x12345678abcdef12
> (lldb) reg read w0
>      w0 = 0xabcdef12
> 
> Or this?
> 
> (lldb) reg write d0 1.3
> (lldb) reg read -f x d0
>      d0 = 0x3ff4cccccccccccd
> (lldb) reg read -f x s0
>      s0 = 0xcccccccd
> (lldb) 
> 
> When you go to read the bytes out of the register context:
> 
>    // Get pointer to m_fpr variable and set the data from it.
>    assert (reg_info->byte_offset < sizeof m_fpr);
>    uint8_t *src = (uint8_t *)&m_fpr + reg_info->byte_offset;
>    switch (reg_info->byte_size)
> 
> you're offsetting unconditionally into the m_fpr -- the floating point part of the register context, aren't you?  Don't you want m_gpr_arm64 if this is a GPR and m_fpr if it is a fp/vector reg?
> 
> If you look at RegisterContextPOSIXProcessMonitor_x86_64::ReadRegister, it has a separate section for floating point registers and for gpr's (it checks if the register encoding is eEncodingVector which doesn't seem like the most straightforward way of indicating this).  I think you'll need to do the same thing in your ReadRegister method - have separate blocks of code for floating point registers (it will be the full 16-byte v0-v31 registers by the time it gets here) or gpr's.
> 
> 
> In the WriteRegister method, there's a big block of code to handle writing a sub-register value (e.g. w0) by only overwriting the lower 32 bits of the register.  It includes
> 
>                    // Copy the src bytes to the destination.
>                    ::memcpy (dst + (reg_info->byte_offset & 0x1), src, src_size);
> 
> The bitwise & of 1 against the byte_offset doesn't make sense to me.  First, RegisterInfo::byte_offset is the offset into the full register context -- not the offset into the containing register.  But by bit-anding it with 1 this is always zero -- this code is always memcpy (dst, src, src_size) -- and as long as the bytes are stored in little endian order, this will work out.  The arm64 subset registers are easy, they're always the lower 32/64 bits of the containing register.  armv7 would require more care to do correctly.
> 
> 
> 
>> On Aug 31, 2014, at 2:12 AM, Paul Osmialowski <pawelo at king.net.pl> wrote:
>> 
>> Corrected version, overhead 'if (success)' removed - it consisted of two if's, first was anded with (reg_info->byte_offset & 0x1) which can never be true on ARM64, second 'if' can never be true because first 'if' action would never happen.
>> Note that my RegisterContextPOSIXProcessMonitor_arm64.cpp file was based on RegistertContextPOSIXProcessMonitor_mips64.cpp which contains the same mistake and the same comment with x86 register names, I guess it was copy-pasted from x86 implementation.
>> 
>> http://reviews.llvm.org/D5089
>> 
>> Files:
>> source/Plugins/Process/POSIX/CMakeLists.txt
>> source/Plugins/Process/POSIX/POSIXThread.cpp
>> source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_arm64.cpp
>> source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_arm64.h
>> source/Plugins/Process/Utility/CMakeLists.txt
>> source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
>> source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
>> <D5089.13127.patch>_______________________________________________
>> lldb-commits mailing list
>> lldb-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>