[Lldb-commits] [PATCH] D154930: [lldb][AArch64] Add the tpidr2 TLS register that comes with SME

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py:73
+
+    @skipUnlessArch("aarch64")
+    @skipUnlessPlatform(["linux"])
----------------
omjavaid wrote:
> 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.
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.


================
Comment at: lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py:90
+        if self.isAArch64SME():
+            self.skipTest("SME must not be enabled.")
+
----------------
And speaking of words, let me change this skip reason to "not be present".


================
Comment at: lldb/test/API/linux/aarch64/tls_registers/main.c:37
+  case '2':
+    getter = get_tpidr2;
+    setter = set_tpidr2;
----------------
omjavaid wrote:
> It would be interesting to test reading/writing tpidr2 when SME is not enabled.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154930/new/

https://reviews.llvm.org/D154930