Page MenuHomePhabricator

lldb more on register context arm64
ClosedPublic

Authored by pawelo on Aug 27 2014, 2:08 PM.

Details

Reviewers
tfiala
Summary

More on registering context on arm64. Depends on D4580 which although accepted, does not seem to be commited yet (I guess due to Host vs HostInfo problem).

Diff Detail

Event Timeline

pawelo updated this revision to Diff 13001.Aug 27 2014, 2:08 PM
pawelo retitled this revision from to lldb more on register context arm64.
pawelo updated this object.
pawelo edited the test plan for this revision. (Show Details)
pawelo set the repository for this revision to rL LLVM.
pawelo added a subscriber: Unknown Object (MLST).
tfiala added a subscriber: tfiala.Aug 27 2014, 2:20 PM

Hey Paul,

For D4580, I looked at the notes on it.

I mistook this comment:
D4488 http://reviews.llvm.org/D4488 was completely covered by later patch
already on LLVM's master.

to indicate that D4580 was already in, so I was just trying to clear out
this ticket.

What is the complete set of patches you are waiting on?

pawelo updated this revision to Diff 13127.Aug 31 2014, 2:12 AM

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.

LGTM, thanks for latest changes.

tfiala accepted this revision.Sep 2 2014, 7:56 AM
tfiala edited edge metadata.

Tested:

  • Ubuntu 14.04 x86_64, clang 3.5-built lldb.
  • MacOSX 10.9.4, Xcode6-Beta6-built lldb.
This revision is now accepted and ready to land.Sep 2 2014, 7:56 AM
tfiala closed this revision.Sep 2 2014, 8:00 AM

Closed with commit:

svn commit
Sending        source/Plugins/Process/POSIX/CMakeLists.txt
Sending        source/Plugins/Process/POSIX/POSIXThread.cpp
Adding         source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_arm64.cpp
Adding         source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_arm64.h
Sending        source/Plugins/Process/Utility/CMakeLists.txt
Adding         source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
Adding         source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
Transmitting file data .......
Committed revision 216907.

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.

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. :)