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 <[email protected]> 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 <[email protected]> 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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits > _______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
