This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Move NativeRegisterContextLinux/RegisterContextPOSIX*_arm to RegisterInfoAndSetInterface
ClosedPublic

Authored by omjavaid on Sep 1 2020, 10:27 AM.

Details

Summary

This patch removes register set definitions and other redundant code from NativeRegisterContextLinux/RegisterContextPOSIX*_arm.
Register sets are now moved under RegisterInfosPOSIX_arm which now uses RegisterInfoAndSetInterface. This is similar to what we earlier did for AArch64.

Diff Detail

Event Timeline

omjavaid created this revision.Sep 1 2020, 10:27 AM
omjavaid requested review of this revision.Sep 1 2020, 10:27 AM

NativeRegisterContextLinux_arm still uses "Plugins/Process/Utility/lldb-arm-register-enums.h" which I intend to deal in a later patch.

omjavaid updated this revision to Diff 289218.Sep 1 2020, 10:33 AM

Fixed last diff it contained an invalid change.

omjavaid updated this revision to Diff 289361.Sep 2 2020, 1:07 AM

Adding changes to FreeBSD/* missed earlier.

omjavaid updated this revision to Diff 289362.Sep 2 2020, 1:09 AM
labath accepted this revision.Sep 2 2020, 8:00 AM

This is great, thanks for doing this. Just a couple of minor comments inline.

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

delete this break.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
61–63

Actually, I'd just change this to assert(target_arch.GetMachine() == llvm::Triple::arm)

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm.cpp
93–95

You can skip this if you want to keep things consistent (or put it in a separate patch, etc), but I just realized that this is completely unnecessary. Instead of the assert, we could just make the assert expression _be_ the definition of the constant (k_num_gpr_registers = ((sizeof g_gpr_regnums_arm / sizeof g_gpr_regnums_arm[0]) - 1)). Given that llvm::array_lengthof is constexpr, it might even work to make this llvm::array_lengthof(g_gpr_regnums_arm)-1.

lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
84–86

I think you should either keep both aarch64 and arm cases, or delete both of them. I'm fine with either.

This revision is now accepted and ready to land.Sep 2 2020, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2020, 9:13 PM