This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Arm64/Linux Add MTE and Pointer Authentication registers
ClosedPublic

Authored by omjavaid on Feb 10 2021, 3:59 PM.

Details

Summary

This patch adds two new register sets for MTE and Pointer Authentication feature registers. These register sets are dynamic and will only be available if underlying hardware support either of these features. LLDB will pull in Aux vector information and create register infos based on that information.

A follow up patch will add a test case to test these feature registers.

Diff Detail

Event Timeline

omjavaid created this revision.Feb 10 2021, 3:59 PM
DavidSpickett added inline comments.
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
82

Should m_pac_mask also be initialised or is it already?

120

I think this should be more defensive:

if (auxv_at_hwcap && (*auxv_at_hwcap & HWCAP_PACA))

Otherwise you'll deference an invalid optional, which might work if its storage happens to be zeroed but it's not intended use.

126

Same here, auxv_at_hwcap2 could be llvm::None.

543

Can you simplify these?

return GetRegisterInfo().IsPAuthReg(reg);

(the others could do that too, but obviously we're ignoring those ones here)

1108

I think m_mte_ctrl_is_valid is missing here.

1209

Just to confirm I understand this logic.

If m_mte_ctrl_is_valid would mean that our cached value of the register is valid. So if something tries to read a new copy of it we fail because they should have used the cached version?

Then if m_mte_ctrl_is_valid is false, our cache is out of date so we do the read.

Seems odd to error on asking for a read of a value that is cached but then again I don't know the surrounding code too well. If that pattern is already established no point disturbing it now.

1227

This is a sanity check that we didn't ask to write this register on a system that doesn't have it?

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
182

This looks like a mistake.

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

Can you get this 2 from some list somewhere in this class? E.g. PauthRegs.Size(). Or add a comment if not.

g_register_infos_pauth perhaps?

327

Is it possible at this point that m_dynamic_reg_infos.size() is 0 making m_dynamic_reg_infos[pa_regnum + i - 1] into m_dynamic_reg_infos[0 + 0 - 1] ?

I guess this could happen if SVE wasn't present but maybe you already account for that.

329

Please comment what the [4] is getting.

333

You could simplify this with back (https://www.cplusplus.com/reference/vector/vector/back/) assuming this is a vector-ish type.

339

Same as above

Given that there's only one MTE register you could also remove the for since we'll only ever have i==0. If we want to be nice and handle future expansion some MTERegs.size() would be preferred.

434
return std::find(pauth_regnum_collection.begin(), pauth_regnum_collection.end(),
                reg) != pauth_regnum_collection.end();
440

as above

lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
537

DEFINE_PAUTH_REGS is an odd name considering we use it for MTE too. DEFINE_EXTENSION_REGS ?

Also I would make it singular "REG"

lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
64 ↗(On Diff #322851)

This seems unrelated

Subsequent update will resolve review comments. @DavidSpickett thanks for review. much appreciated

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
82

Ack. Forgot to do it.

120

Agreed

126

Ack.

1108

Ack.

1209

Status is used to hold error codes and default error code is 'success'. If we have a valid cache value that means we dont need to read and return success thats why returning error without setting the error code.

If we need to read then we call ReadRegisterSet which will set the error code in case of failure.

1227

This may only be relevant for optional regsets where we need to check if registers are readable before writing and registers which need partial modification and we need to have most recent value.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
182

yes!!!

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

k_num_pauth_register already populated and will be used here as well.

327

Its not possible for m_dynamic_reg_infos.size to be zero because it is populated with either g_register_infos_arm64_sve_le or g_register_infos_arm64_le register infos array.

329

Ack.

333

done.

339

Ack.

434

Ack.

440

Ack.

lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
537

Agreed.

lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
64 ↗(On Diff #322851)

Ack.

omjavaid updated this revision to Diff 324017.Feb 16 2021, 8:39 AM
omjavaid updated this revision to Diff 324018.Feb 16 2021, 8:41 AM
DavidSpickett added inline comments.Feb 16 2021, 8:46 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
1209

Doh, yes I missed that that "error" will not be a failure at that point.

omjavaid updated this revision to Diff 325435.Feb 22 2021, 6:32 AM

Rebased after updates to parent.

DavidSpickett added inline comments.Feb 22 2021, 8:01 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
1127–1146

Indent here

Actually, for these you could just do m_sve_header_is_valid = error.Success(), no if needed. Assuming setting it false here is also fine, which it might not be.

1161

Should be GetPACMaskSize

1164

see above

1218

see above

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

(like earlier) would it be ok to remove the if?

m_mte_regset_enabled = opt_regsets & eRegsetEnableMTE;
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
29

indentation

omjavaid added inline comments.Feb 22 2021, 10:49 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
1127–1146

I guess this is inline with rest of the code in this file. Also avoiding write every time may well be a tiny bit faster as we avoid flushing the cache line to which m_sve_header_is_valid belongs.

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

I think here as well if do write eveytime regardless of true/false we might end up refreshing the whole data set contained in cache against a RegisterInfoPOSIX_arm64 object. What do you think?

omjavaid updated this revision to Diff 325493.Feb 22 2021, 10:51 AM

Ran clang format and fixed typos.

DavidSpickett added inline comments.Feb 23 2021, 2:24 AM
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
276

I assume you mean the cpu cache not any lldb register cache, I don't see a need to care that much about it here.

The reason to always assign is so that there's no implication that there's a 3rd state. Like "false and not enabled" "true and enabled" "false and enabled" (which makes no sense but types wise, it's possible).

Granted it's a drop in the bucket vs the rest of the state we're handling here so I'll leave it up to you.

DavidSpickett accepted this revision.Feb 23 2021, 2:29 AM
This revision is now accepted and ready to land.Feb 23 2021, 2:29 AM
omjavaid updated this revision to Diff 329239.Mar 9 2021, 12:37 AM

Rebased after changes to parent.

omjavaid updated this revision to Diff 330189.Mar 12 2021, 2:51 AM

Updated after changes in parent. Also removed D96459

omjavaid updated this revision to Diff 331835.Mar 19 2021, 5:26 AM

Rebased after changes to parent D96458

labath requested changes to this revision.Mar 22 2021, 5:41 AM

This still suffers from the duplication of the auxv reading code from NativeProcessELF. Other than that, it's ok-ish.

This revision now requires changes to proceed.Mar 22 2021, 5:41 AM
omjavaid updated this revision to Diff 332617.Mar 23 2021, 4:37 AM

Pulling GetAuxValue from NativeProcessELF. @labath what do you think.

labath accepted this revision.Mar 30 2021, 2:47 AM

Looks good (and sorry for the delay). Just rebase to main to avoid the cast.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
62

After d1486e65a164, this cast should be unnecessary.

This revision is now accepted and ready to land.Mar 30 2021, 2:47 AM
This revision was landed with ongoing or failed builds.Mar 30 2021, 4:39 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2021, 4:39 PM

@omjavaid I get some new warnings when compiling this on x86:

[732/1117] Building CXX object too.../RegisterContextDarwin_arm64.cpp.o
In file included from /work/open_source/lldb-cross-compile/llvm-project/lldb/source/Plugins/Process/Utility/RegisterContextDarwin_arm64.cpp:70:
/work/open_source/lldb-cross-compile/llvm-project/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h:794:35: warning: ‘g_register_infos_mte’ defined but not used [-Wunused-variable]
  794 | static lldb_private::RegisterInfo g_register_infos_mte[] = {
      |                                   ^~~~~~~~~~~~~~~~~~~~
/work/open_source/lldb-cross-compile/llvm-project/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h:791:35: warning: ‘g_register_infos_pauth’ defined but not used [-Wunused-variable]
  791 | static lldb_private::RegisterInfo g_register_infos_pauth[] = {
      |                                   ^~~~~~~~~~~~~~~~~~~~~~

Makes some sense that they're not used but presumably we could mark them as such? In case it's g++ not seeing the existing macros properly, I'm using:

g++-9 (Ubuntu 9.3.0-23ubuntu1~16.04) 9.3.0
Via Web