[Lldb-commits] [PATCH] D96463: [LLDB] Arm64/Linux test case for MTE and Pointer Authentication regset

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added inline comments.


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:35
+
+        p_value_bytes = ['0xff', '0x55', '0x11', '0x01', '0x00']
+        for i in range(16):
----------------
Can you explain the logic for the values here?


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:39
+                ' '.join(p_value_bytes[i % 5] for _ in range(p_reg_size)) + '}'
+            self.expect("register read " + 'p%i' % (i), substrs=[p_regs_value])
+
----------------
You don't need the brackets around i for `% (i)`.


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:49
+
+        for i in range(32):
+            self.runCmd('register write ' + 'z%i' %
----------------
Is it slightly more strict if we have one loop that reads then writes?
```
for i in range(32):
   write
   read
```

Since we write the same value, if you write them all then read them all you aren't totally sure that each one is lined up right. Then again I'm sure you've tested sve register writes elsewhere.

Anyway, a single loop for each would be a bit neater.


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:111
+
+        if not self.isAArch64SVE() and sve_regset_available:
+            self.fail(
----------------
If you move all this code from after the for, into the for loop, you could skip having the bools per register set.


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:14
+  asm volatile("setffr\n\t");
+  asm volatile("ptrue p0.b\n\t");
+  asm volatile("ptrue p1.h\n\t");
----------------
(I'm not familiar with SVE assembly but anyway..)

Is the .b/.h/.s etc. pattern specific or does it not matter?


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:31-32
+
+  asm volatile("cpy  z0.b, p0/z, #1\n\t");
+  asm volatile("cpy  z1.b, p5/z, #2\n\t");
+  asm volatile("cpy  z2.b, p10/z, #3\n\t");
----------------
Same here, is the p0/p5/p10/p15 pattern affecting values used in the test or just for fun? (which isn't a bad thing)


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:68
+  unsigned int mte_is_enabled = 0;
+  unsigned int pauth_is_enabled = 0;
+
----------------
This appears to be defined but not set.


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:73
+
+  if (hwcap & HWCAP_SVE) // check if SVE is present
+    set_sve_registers();
----------------
Should there be a `#ifndef HWCAP_SVE` above too?


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:82
+  if (hwcap2 & HWCAP2_MTE)
+    mte_is_enabled = 1;
+
----------------
Would be neat to do these vars like:
```
unsigned int mte_is_enabled = hwcap2 & HWCAP2_MTE;
```


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

https://reviews.llvm.org/D96463