This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [ABI/AArch64] Add pseudo-regs if missing
ClosedPublic

Authored by mgorny on Sep 16 2021, 5:25 AM.

Details

Summary

Create pseudo-registers on the AArch64 target if they are not provided
by the remote server. This is the case for gdbserver. The created
registers are:

  • 32-bit wN partials for 64-bit xN registers
  • double precision floating-point dN registers (overlapping with vN)
  • single precision floating-point sN registers (overlapping with vN)

Diff Detail

Event Timeline

mgorny created this revision.Sep 16 2021, 5:25 AM
mgorny requested review of this revision.Sep 16 2021, 5:25 AM

(so far with ugly hacks, to get some initial ideas on how to proceed with this)

@labath, I think the biggest problem here is that DynamicRegisterInfo is defined only inside source tree. I'm not sure if I should continue with a forward declaration like this or aim to move DynamicRegisterInfo into public headers. if the former, should I move it into lldb_private namespace?

mgorny updated this revision to Diff 372907.Sep 16 2021, 5:35 AM

Use public API to determine next regnums.

I think we could move the DynamicRegisterInfo class into non-plugin code, but I need to give it more thought.

In the mean time (and maybe before that), I'd like to understand why do you need to mess with the process plugin register numbers in this class. It doesn't seem like a good idea for that class to be assuming anything about how those numbers are allocated and used..

In the mean time (and maybe before that), I'd like to understand why do you need to mess with the process plugin register numbers in this class. It doesn't seem like a good idea for that class to be assuming anything about how those numbers are allocated and used..

For some reason, I have assumed that I need to allocate some unique numbers for the new registers. Thinking about it, I'm going to try if things work if I just put LLDB_INVALID_REGNUM there.

Ok, I definitely need some 'process plugin' IDs to handle invalidate_regs between pseudo-registers.

mgorny updated this revision to Diff 373024.Sep 16 2021, 11:57 AM
mgorny retitled this revision from [lldb] [ABI/AArch64] Add 32-bit pseudo-regs if missing [WIP] to [lldb] [ABI/AArch64] Add pseudo-regs if missing [WIP].
mgorny edited the summary of this revision. (Show Details)

Cover FPU registers.

mgorny updated this revision to Diff 373452.Sep 19 2021, 4:19 AM

Rebase. Clang-format. Eliminate remote regnums entirely.

mgorny updated this revision to Diff 376549.Oct 1 2021, 8:35 AM
mgorny retitled this revision from [lldb] [ABI/AArch64] Add pseudo-regs if missing [WIP] to [lldb] [ABI/AArch64] Add pseudo-regs if missing.
mgorny added a reviewer: teemperor.

Update for DynamicRegisterInfo move.

labath added inline comments.Oct 5 2021, 1:05 AM
lldb/include/lldb/Target/ABI.h
131–134 ↗(On Diff #376549)

Ideally, I would like to subsume the AugmentRegisterInfo functionality into this function, so that one would just call abi->Augment(Dynamic)RegisterInfo(dyn_reg_info) and it would automatically fill it in with all the necessary information. The old AugmentRegisterInfo function could become a private/protected implementation detail.

Would such a thing be possible? Perhaps with some preparatory refactoring patch?

mgorny marked an inline comment as done.Oct 5 2021, 6:35 AM
mgorny updated this revision to Diff 377193.Oct 5 2021, 6:43 AM

Update to use the new AugmentRegisterInfo() API.

mgorny updated this revision to Diff 377792.Oct 7 2021, 3:24 AM

Rewrite for std::vector<RemoteRegisterInfo>-based augmentation.

mgorny updated this revision to Diff 378474.Oct 9 2021, 1:57 PM

Arrays need explicit initialization.

mgorny updated this revision to Diff 378515.Oct 10 2021, 9:15 AM

Remove unused next_regnum_lldb arg.

mgorny updated this revision to Diff 378526.Oct 10 2021, 12:13 PM

Rename the set to 'supplementary registers'.

labath accepted this revision.Oct 11 2021, 4:51 AM
labath added inline comments.
lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
117–123

This might be reason enough to make me replace array<uint32_t> with array<Optional<uint32_t> and array<bool> with std::bitset

147–151

It might be worth making a utility function for this as well (or maybe moving the for loop into the existing utility function).

lldb/test/API/functionalities/gdb_remote_client/TestGDBServerTargetXML.py
383

what's up with the [:]? Can we just drop that?

This revision is now accepted and ready to land.Oct 11 2021, 4:51 AM
mgorny marked 3 inline comments as done.Oct 11 2021, 7:49 AM
mgorny added inline comments.
lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
117–123

Makes sense.

147–151

I inlined the for loop and renamed the function to addPartialRegisters().

lldb/test/API/functionalities/gdb_remote_client/TestGDBServerTargetXML.py
383

It was necessary to modify the reg_data variable from higher function instead of declaring a new local variable but I think we can move it to class variable now to make it cleaner.

mgorny marked 3 inline comments as done.Oct 11 2021, 7:55 AM
This revision was landed with ongoing or failed builds.Oct 11 2021, 8:02 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2021, 8:02 AM