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

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

We are using SVE read/write in this test to make sure we do not overlap register offsets while calculating offsets for the dynamic registers. Further dynamic register sets should also be readable.



================
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):
----------------
DavidSpickett wrote:
> Can you explain the logic for the values here?
P reg sets predicate lanes. P0 will have all lanes set while P5 will have no lanes set. These are just random values testing read/write.


================
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])
+
----------------
DavidSpickett wrote:
> You don't need the brackets around i for `% (i)`.
Ack.


================
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' %
----------------
DavidSpickett wrote:
> 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.
I think you are right. If we do a read after write in a single loop we ll be doing 64 ptrace calls on lldb-server side. More the better.


================
Comment at: lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:111
+
+        if not self.isAArch64SVE() and sve_regset_available:
+            self.fail(
----------------
DavidSpickett wrote:
> If you move all this code from after the for, into the for loop, you could skip having the bools per register set.
I was actually trying to see whether what cpuinfo reports and what is reported by prctl is same or not.
I am going to revisit this piece and adjust.


================
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");
----------------
DavidSpickett wrote:
> (I'm not familiar with SVE assembly but anyway..)
> 
> Is the .b/.h/.s etc. pattern specific or does it not matter?
Replied in comment below.


================
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");
----------------
DavidSpickett wrote:
> 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)
I copied this from SVE test case that I wrote last year. 

P is a predicate register and ptrue/pfalse instruction is used to set a predicate lane with a pattern. pattern is decide based on size specifier, b, h, s and d.

if size specified is b all lanes will be set to 1. which is needed to set al bytes in a  Z registers to the specified value.

We are setting p0, p5, p10 and p15 to enable all lanes other p registers are not enabling all lanes thats why they were not used as predicate for setting Z registers in following lines which set a constant value to Z register byte size elements.


================
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;
+
----------------
DavidSpickett wrote:
> This appears to be defined but not set.
Ack. I ll adjust these as per your suggestion below.


================
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();
----------------
DavidSpickett wrote:
> Should there be a `#ifndef HWCAP_SVE` above too?
Most distros have now backported ptrace.h to include HWCAP_SVE but other two are still in the process.


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


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

https://reviews.llvm.org/D96463