This is an archive of the discontinued LLVM Phabricator instance.

[lldb][AArch64] Add the tpidr2 TLS register that comes with SME
ClosedPublic

Authored by DavidSpickett on Jul 11 2023, 1:24 AM.

Details

Summary

This changes the TLS regset to not only be dynamic in that it could
exist or not (though it always does) but also of a dynamic size.

If SME is present then the regset is 16 bytes and contains both tpidr
and tpidr2.

Testing is the same as tpidr. Write from assembly, read from lldb and
vice versa since we have no way to predict what its value should be
by just running a program.

Diff Detail

Event Timeline

DavidSpickett created this revision.Jul 11 2023, 1:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2023, 1:24 AM
DavidSpickett requested review of this revision.Jul 11 2023, 1:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2023, 1:24 AM
omjavaid added inline comments.Jul 16 2023, 9:02 PM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
269

GetTLS could be GetTLSBuffer similar to GetGPRBuffer/GetFPRBuffer above.

lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
73

These three tests have a lot of commonalities may be merge them into one testing the whole logic. Doesn't look like we are getting much out of emitting three tests from this fairly basic test class.

lldb/test/API/linux/aarch64/tls_registers/main.c
38

It would be interesting to test reading/writing tpidr2 when SME is not enabled.

DavidSpickett added inline comments.Jul 17 2023, 12:44 AM
lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
73

The tradeoff is execution time vs. a HWCAP check in the program file and a bunch of ifs in Python.

Let me see what I can do, but I'm leaning toward the implementation complexity outweighing the performance gained.

90

And speaking of words, let me change this skip reason to "not be present".

lldb/test/API/linux/aarch64/tls_registers/main.c
38

Not enabled, or not present? (I admit, these two words are used interchangeably in places)

Not enabled is actually the state here, as there's no SMTART used here. Architecture wise, I don't see anything to indicate it makes a difference if SME is active or not.

Not present is covered by test_tpidr2_no_sme.

Add "Buffer" to method names.

enabled -> present in skipped messages. I realised that "enabled" is ambiguous
does it mean enabled in the CPU or in the process, present is more clearly
meaning is it on the CPU at all.

DavidSpickett marked an inline comment as done.Jul 18 2023, 8:24 AM

Note the behaviour of tpidr2 on a system without SME.

DavidSpickett added inline comments.Jul 18 2023, 8:52 AM
lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
73

I've looked into this. A major thing to note is that reading tpidr2 (or rather, the system register operands that represent it) on a system without SME will SIGILL. I tried this on a Mountain Jade system.

This means we cannot have the program file set both regardless and only test what we expect to be present. We still need the option to the program and still need to run it again each time we want to check a different register.

(I guess you could have an option for each register, but again we're trading complexity here)

That does mean that the program file is the same between the 3 tests, so you could merge them to only build once. That said I think the time saved rebuilding a small example is not much when put against the pile of if you would need to use to merge the 3 tests into one (and therefore have to unpick later).

I could merge test_tpidr2_no_sme into test_tpidr but again, I'd rather have the clear separation of it being its own test than add an if not self.isAArch64SME... in there.

If I'm addressing completely different aspects than you had considered here, let me know and I'll fix those.

omjavaid accepted this revision.Jul 21 2023, 2:14 AM

This looks good just a minor suggestion inline.

lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
84

Should we try to read/write both tpidr and tpidr2 here?

This revision is now accepted and ready to land.Jul 21 2023, 2:14 AM
DavidSpickett marked 2 inline comments as done.Jul 21 2023, 2:44 AM

Thanks for the review, I'll try making the test a bit more flexible so we can read both registers at once for a bit more coverage.

lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
84

We could but on a system with SME test_tpidr will also run. So you're checking both regardless.

I could change the test program so that it always sets tpidr, but only sets tpdir2 if told to. Then we could check both here just in case there's an issue reading one after the other. I'll give that a go.

Refactor the test so there are tests for:

  • reading tpidr on non-SME systems
  • reading tpidr and tpidr2 on SME systems
  • tpidr2 not being present on non-SME systems

You could try to fold the last one into the first one but
it gets fiddly for not much saving.

DavidSpickett marked an inline comment as done.Jul 21 2023, 7:29 AM

Correct missing "tls" in test name.

Rebase and put this after the SSVE registers patch.

This revision was landed with ongoing or failed builds.Jul 26 2023, 2:34 AM
This revision was automatically updated to reflect the committed changes.
lldb/test/API/linux/aarch64/tls_register/TestAArch64LinuxTLSRegister.py