This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Combine multiple defs of arm64 register sets
ClosedPublic

Authored by omjavaid on May 17 2020, 11:42 PM.

Details

Summary

This patch aims to combine similar arm64 register set definitions defined in NativeRegisterContextLinux_arm64 and RegisterContextPOSIX_arm64.
I have implemented a register set interface out of RegisterInfoInterface class and moved arm64 register sets into RegisterInfosPOSIX_arm64 which is similar to Utility/RegisterContextLinux_* implemented by various other targets. This will help in managing register sets of new ARM64 architecture features in one place.

Built and tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabihf targets.

Diff Detail

Event Timeline

omjavaid created this revision.May 17 2020, 11:42 PM
omjavaid updated this revision to Diff 274382.Jun 30 2020, 3:15 AM

In this updated I have segregated RegisterInfoInterface and RegisterInfoAndSetInterface as two mutually exclusive interfaces. RegisterInfoPosix_arm64 is currently the only class making use of set interface but I am going to extend this once we reach a consensus. All occurrences of RegisterInfoPosix_arm64 have been updated accordingly and for the case of NativeRegisterContextLinux_arm64 RegisterInfoInterface shared pointer is casted into a RegisterInfoAndSetInterface shared pointer. Once we have this implemented for other architecture there wont be a need for having a shared pointer of RegisterInfoAndSetInterface in all variants of NativeRegisterContextLinux_*

@labath what are your thoughts on this ?

labath added a comment.Jul 3 2020, 6:26 AM

I like this.

lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
167 ↗(On Diff #274382)

It would be good to merge these two switches. Then the reg(set)_interface variables would be local to each case label and it would not be so weird that we sometimes use one and sometimes the other.

lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h
21 ↗(On Diff #274382)

While we're changing interfaces, let's also make this unique_ptr<RegisterInfoAndSetInterface> to document the ownership transfer. (I might also drop the concrete_frame_idx argument, as that is always zero.)

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
71–72

The way I'd recommend dealing with this is to have a RegisterInfoPOSIX_arm64& GetRegisterInfo() const method which performs a cast on the m_register_info_interface_up pointer, and then have everything call that. If you place that method in close proximity to this constructor, then it should be clear that this operation is always safe. There's already something similar being done in e.g. NativeThreadLinux::GetProcess.

lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h
36

With the above idea, it should no longer be needed to change this into a shared_ptr.

lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
23–24

similar thoughts about unique_ptr and concrete_frame_idx. In fact, thinking ahead to the next patch, I think we could make this an unique_ptr<RegisterInfoPOSIX_arm64> even.

lldb/source/Plugins/Process/Utility/RegisterInfoAndSetInterface.h
24 ↗(On Diff #274382)

s/virtual/override (or just omit the destructor completely)

35–37 ↗(On Diff #274382)

These aren't really register *set* related, are they? Could they go on the base interface instead?

Although, THB, the way I'd handle this is by having something a ArrayRef<RegisterInfo> RegInfos() interface, and then having the callers do reg_info_interface->RegInfos()[reg_index].offset

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
18

How about we put this inside the RegisterInfoPOSIX_arm64 class, and drop RegisterSetPOSIX_ARM64 from the name. (RegisterInfoPOSIX_arm64::GPRegSet ?) (there's already something similar inside NativeRegisterContextNetBSD_x86_64).

20–28

I'm not sure what's the advantage of making this a struct vs. just splating it into the containing class. Maybe that made more sense when originally when the class was doing a lot more work, but here I'd just replace this by a list of members.

lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
82 ↗(On Diff #274382)

The reg(set)_interface and register_context switches could be merged here too in a similar way...

emaste removed a subscriber: jhibbits.
emaste added a subscriber: mhorne.
omjavaid marked 10 inline comments as done.Jul 6 2020, 5:52 PM
omjavaid added inline comments.
lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
167 ↗(On Diff #274382)

I have tried a couple of options but the no of different combinations involved I feel current implementation should stay untill we incrementally move remaining architectures to user RegisterInfosAndSet interface.

lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h
21 ↗(On Diff #274382)

Agreed. I will update this in updated revision.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
71–72

yes this would be a lot cleaner than what I am doing currently. I am going to update this in next revision.

lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
82 ↗(On Diff #274382)

Here also incremental merger will be a better approach IMO.

omjavaid updated this revision to Diff 275869.Jul 6 2020, 6:00 PM

This new revision incorporates all suggestions on previous version.

@labath What do you think? I have skipped merging switch statements and have picked all other suggestions you highlighted. If this is LGTM then i can rebase SVE patches on top of this?

labath marked an inline comment as done.Jul 7 2020, 2:12 AM

This looks great. I just have a quick question about the GPR vs GPRegSet thingy...

lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
167 ↗(On Diff #274382)

I like this solution.

lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
50

move this to the initializer list

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
85

I'd probably just delete this comment (or merge it with the leading comment above the array definition), and then let clang-format lay this out normally...

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
20

Why these names? I think [GF]PRegSet would be better for two reasons:

  • the same names with the same purpose already exist in NativeRegisterContextNetBSD_x86_64.h
  • it seems like a better way to differentiate from the [GF]PR structs below than adding a redundant ARM64 prefix.
omjavaid marked 3 inline comments as done.Jul 7 2020, 5:10 AM
omjavaid added inline comments.
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
50

ACK.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
85

Let me check what clang-format emits.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
20

Indeed ARM64 is redundant now that we have these enums in RegisterInfosPOSIX_arm64 . I will fix this in updated revision.

omjavaid updated this revision to Diff 276015.Jul 7 2020, 5:26 AM

This revision fixed issues highlighted in last review.

LGTM?

labath accepted this revision.Jul 7 2020, 5:33 AM

ship it.

This revision is now accepted and ready to land.Jul 7 2020, 5:33 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2020, 8:25 AM