Carefully testing SME and non-SME hardware showed some testsuite issues

TestRegisters.py expected there to be no registers unable to be
read.  With an SME system when in non-SME mode, all of the SVE/SME
registers will show as unreadable.

TestGdbRemoteGPacket.py was testing the g/G packet with debugserver
(which lldb doesn't use any more) and this exposed the fact that I
wasn't handling SME/SVE registers correctly in
DNBArchImplArm64::GetRegisterContext/SetRegisterContext.  Fix those
to correctly account for the size of these register contexts (the
ZA register is stored in a vector, instead of having a compile-time
register context size).  Also, RNBRemote::HandlePacket_G() was not
handling the thread specifier properly (it would try to interpret
it as content for the packet, and the hex decoding would fail).
I think this test might be setting the current thread with Hg<tid>
before it sends only "g" or "G", that might work.

In debugserver, changed all stack-allocated DNBRegisterValue objects
to be unique_ptr managed so they're on heap.  This object is now
64k and putting that on the stack could be a problem.  We don't
have multiple DNBRegisterValue objects alive at the same time, so
I didn't dynamically size it to the maximum ZA register size on
the current machine.

Fix the comment formatting suggestions from Jonas.

Author: jasonmolenda

lldb/test/API/commands/register/register/register_command/TestRegisters.py

@@ -21,6 +21,24 @@ def tearDown(self):
         self.dbg.GetSelectedTarget().GetProcess().Destroy()
         TestBase.tearDown(self)
 
+    # on macOS, detect if the current machine is arm64 and supports SME
+    def get_sme_available(self):
+        if self.getArchitecture() != "arm64":
+            return None
+        try:
+            sysctl_output = subprocess.check_output(
+                ["sysctl", "hw.optional.arm.FEAT_SME"]
+            ).decode("utf-8")
+        except subprocess.CalledProcessError:
+            return None
+        m = re.match(r"hw\.optional\.arm\.FEAT_SME: (\w+)", sysctl_output)
+        if m:
+            if int(m.group(1)) == 1:
+                return True
+            else:
+                return False
+        return None
+
     @skipIfiOSSimulator
     @skipIf(archs=no_match(["amd64", "arm", "i386", "x86_64"]))
     @expectedFailureAll(oslist=["freebsd", "netbsd"], bugnumber="llvm.org/pr48371")
@@ -32,11 +50,19 @@ def test_register_commands(self):
         # verify that logging does not assert
         self.log_enable("registers")
 
+        error_str_matched = False
+        if self.get_sme_available() == True and self.platformIsDarwin():
+            # On Darwin AArch64 SME machines, we will have unavailable
+            # registers when not in Streaming SVE Mode/SME, so
+            # `register read -a` will report that some registers
+            # could not be read.  This is expected.
+            error_str_matched = True
+
         self.expect(
             "register read -a",
             MISSING_EXPECTED_REGISTERS,
             substrs=["registers were unavailable"],
-            matching=False,
+            matching=error_str_matched,
         )
 
         all_registers = self.res.GetOutput()
@@ -60,7 +86,7 @@ def test_register_commands(self):
                 self.runCmd("register read q15")  # may be available
 
         self.expect(
-            "register read -s 4", substrs=["invalid register set index: 4"], error=True
+            "register read -s 8", substrs=["invalid register set index: 8"], error=True
         )
 
     @skipIfiOSSimulator

lldb/tools/debugserver/source/MacOSX/MachProcess.mm

@@ -1417,15 +1417,17 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
       continue;
     for (uint32_t reg = 0; reg < reg_sets[set].num_registers; ++reg) {
       if (strcmp(reg_sets[set].registers[reg].name, "esr") == 0) {
-        DNBRegisterValue reg_value;
-        if (GetRegisterValue(tid, set, reg, &reg_value)) {
-          esr = reg_value.value.uint64;
+        std::unique_ptr<DNBRegisterValue> reg_value =
+            std::make_unique<DNBRegisterValue>();
+        if (GetRegisterValue(tid, set, reg, reg_value.get())) {
+          esr = reg_value->value.uint64;
         }
       }
       if (strcmp(reg_sets[set].registers[reg].name, "far") == 0) {
-        DNBRegisterValue reg_value;
-        if (GetRegisterValue(tid, set, reg, &reg_value)) {
-          far = reg_value.value.uint64;
+        std::unique_ptr<DNBRegisterValue> reg_value =
+            std::make_unique<DNBRegisterValue>();
+        if (GetRegisterValue(tid, set, reg, reg_value.get())) {
+          far = reg_value->value.uint64;
         }
       }
     }

lldb/tools/debugserver/source/MacOSX/MachThread.cpp

@@ -509,10 +509,12 @@ void MachThread::DumpRegisterState(nub_size_t regSet) {
     if (m_arch_up->RegisterSetStateIsValid((int)regSet)) {
       const size_t numRegisters = GetNumRegistersInSet(regSet);
       uint32_t regIndex = 0;
-      DNBRegisterValueClass reg;
+      std::unique_ptr<DNBRegisterValueClass> reg =
+          std::make_unique<DNBRegisterValueClass>();
       for (regIndex = 0; regIndex < numRegisters; ++regIndex) {
-        if (m_arch_up->GetRegisterValue((uint32_t)regSet, regIndex, &reg)) {
-          reg.Dump(NULL, NULL);
+        if (m_arch_up->GetRegisterValue((uint32_t)regSet, regIndex,
+                                        reg.get())) {
+          reg->Dump(NULL, NULL);
         }
       }
     } else {

lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp

@@ -463,6 +463,15 @@ kern_return_t DNBArchMachARM64::GetSVEState(bool force) {
   if (!CPUHasSME())
     return KERN_INVALID_ARGUMENT;
 
+  // If the processor is not in Streaming SVE Mode, these thread_get_states
+  // will fail, and we may return uninitialized data in the register context.
+  memset(&m_state.context.sve.z[0], 0,
+         ARM_SVE_Z_STATE_COUNT * sizeof(uint32_t));
+  memset(&m_state.context.sve.z[16], 0,
+         ARM_SVE_Z_STATE_COUNT * sizeof(uint32_t));
+  memset(&m_state.context.sve.p[0], 0,
+         ARM_SVE_P_STATE_COUNT * sizeof(uint32_t));
+
   // Read the registers from our thread
   mach_msg_type_number_t count = ARM_SVE_Z_STATE_COUNT;
   kern_return_t kret =
@@ -502,6 +511,11 @@ kern_return_t DNBArchMachARM64::GetSMEState(bool force) {
   if (!CPUHasSME())
     return KERN_INVALID_ARGUMENT;
 
+  // If the processor is not in Streaming SVE Mode, these thread_get_states
+  // will fail, and we may return uninitialized data in the register context.
+  memset(&m_state.context.sme.svcr, 0, ARM_SME_STATE_COUNT * sizeof(uint32_t));
+  memset(m_state.context.sme.za.data(), 0, m_state.context.sme.za.size());
+
   // Read the registers from our thread
   mach_msg_type_number_t count = ARM_SME_STATE_COUNT;
   kern_return_t kret =
@@ -539,7 +553,9 @@ kern_return_t DNBArchMachARM64::GetSMEState(bool force) {
   }
 
   if (CPUHasSME2()) {
-    count = ARM_SME2_STATE;
+    memset(&m_state.context.sme.zt0, 0,
+           ARM_SME2_STATE_COUNT * sizeof(uint32_t));
+    count = ARM_SME2_STATE_COUNT;
     kret = thread_get_state(m_thread->MachPortNumber(), ARM_SME2_STATE,
                             (thread_state_t)&m_state.context.sme.zt0, &count);
     m_state.SetError(set, Read, kret);
@@ -2903,9 +2919,12 @@ kern_return_t DNBArchMachARM64::GetRegisterState(int set, bool force) {
   case e_regSetALL: {
     kern_return_t retval = GetGPRState(force) | GetVFPState(force) |
                            GetEXCState(force) | GetDBGState(force);
+    // If the processor is not in Streaming SVE Mode currently, these
+    // two will fail to read.  Don't return that as an error, it will
+    // be the most common case.
     if (CPUHasSME()) {
-      retval |= GetSVEState(force);
-      retval |= GetSMEState(force);
+      GetSVEState(force);
+      GetSMEState(force);
     }
     return retval;
   }
@@ -2964,7 +2983,11 @@ nub_size_t DNBArchMachARM64::GetRegisterContext(void *buf, nub_size_t buf_len) {
   const bool cpu_has_sme = CPUHasSME();
   if (cpu_has_sme) {
     size += sizeof(m_state.context.sve);
-    size += sizeof(m_state.context.sme);
+    // ZA register is in a std::vector<uint8_t> so we need to add
+    // the sizes of the SME manually.
+    size += ARM_SME_STATE_COUNT * sizeof(uint32_t);
+    size += m_state.context.sme.za.size();
+    size += ARM_SME2_STATE_COUNT * sizeof(uint32_t);
   }
 
   if (buf && buf_len) {
@@ -2974,9 +2997,13 @@ nub_size_t DNBArchMachARM64::GetRegisterContext(void *buf, nub_size_t buf_len) {
     bool force = false;
     if (GetGPRState(force) | GetVFPState(force) | GetEXCState(force))
       return 0;
-    if (cpu_has_sme)
-      if (GetSVEState(force) | GetSMEState(force))
-        return 0;
+    // Don't error out if SME/SVE fail to read. These can only be read
+    // when the process is in Streaming SVE Mode, so the failure to read
+    // them will be common.
+    if (cpu_has_sme) {
+      GetSVEState(force);
+      GetSMEState(force);
+    }
 
     // Copy each struct individually to avoid any padding that might be between
     // the structs in m_state.context
@@ -2988,8 +3015,17 @@ nub_size_t DNBArchMachARM64::GetRegisterContext(void *buf, nub_size_t buf_len) {
     if (cpu_has_sme) {
       ::memcpy(p, &m_state.context.sve, sizeof(m_state.context.sve));
       p += sizeof(m_state.context.sve);
-      ::memcpy(p, &m_state.context.sme, sizeof(m_state.context.sme));
-      p += sizeof(m_state.context.sme);
+
+      memcpy(p, &m_state.context.sme.svcr,
+             ARM_SME_STATE_COUNT * sizeof(uint32_t));
+      p += ARM_SME_STATE_COUNT * sizeof(uint32_t);
+      memcpy(p, m_state.context.sme.za.data(), m_state.context.sme.za.size());
+      p += m_state.context.sme.za.size();
+      if (CPUHasSME2()) {
+        memcpy(p, &m_state.context.sme.zt0,
+               ARM_SME2_STATE_COUNT * sizeof(uint32_t));
+        p += ARM_SME2_STATE_COUNT * sizeof(uint32_t);
+      }
     }
     ::memcpy(p, &m_state.context.exc, sizeof(m_state.context.exc));
     p += sizeof(m_state.context.exc);
@@ -3010,6 +3046,15 @@ nub_size_t DNBArchMachARM64::SetRegisterContext(const void *buf,
                                                 nub_size_t buf_len) {
   nub_size_t size = sizeof(m_state.context.gpr) + sizeof(m_state.context.vfp) +
                     sizeof(m_state.context.exc);
+  if (CPUHasSME()) {
+    // m_state.context.za is three status registers, then a std::vector<uint8_t>
+    // for ZA, then zt0, so the size of the data is not statically knowable.
+    nub_size_t sme_size = ARM_SME_STATE_COUNT * sizeof(uint32_t);
+    sme_size += m_state.context.sme.za.size();
+    sme_size += ARM_SME2_STATE_COUNT * sizeof(uint32_t);
+
+    size += sizeof(m_state.context.sve) + sme_size;
+  }
 
   if (buf == NULL || buf_len == 0)
     size = 0;
@@ -3025,6 +3070,20 @@ nub_size_t DNBArchMachARM64::SetRegisterContext(const void *buf,
     p += sizeof(m_state.context.gpr);
     ::memcpy(&m_state.context.vfp, p, sizeof(m_state.context.vfp));
     p += sizeof(m_state.context.vfp);
+    if (CPUHasSME()) {
+      memcpy(&m_state.context.sve, p, sizeof(m_state.context.sve));
+      p += sizeof(m_state.context.sve);
+      memcpy(&m_state.context.sme.svcr, p,
+             ARM_SME_STATE_COUNT * sizeof(uint32_t));
+      p += ARM_SME_STATE_COUNT * sizeof(uint32_t);
+      memcpy(m_state.context.sme.za.data(), p, m_state.context.sme.za.size());
+      p += m_state.context.sme.za.size();
+      if (CPUHasSME2()) {
+        memcpy(&m_state.context.sme.zt0, p,
+               ARM_SME2_STATE_COUNT * sizeof(uint32_t));
+        p += ARM_SME2_STATE_COUNT * sizeof(uint32_t);
+      }
+    }
     ::memcpy(&m_state.context.exc, p, sizeof(m_state.context.exc));
     p += sizeof(m_state.context.exc);
 
@@ -3033,6 +3092,10 @@ nub_size_t DNBArchMachARM64::SetRegisterContext(const void *buf,
     assert(bytes_written == size);
     SetGPRState();
     SetVFPState();
+    if (CPUHasSME()) {
+      SetSVEState();
+      SetSMEState();
+    }
     SetEXCState();
   }
   DNBLogThreadedIf(

lldb/tools/debugserver/source/MacOSX/arm64/sme_thread_status.h

@@ -1,7 +1,7 @@
 #include <mach/mach.h>
 #include <stdint.h>
 
-// define the SVE/SME/SME2 thread status structures
+// Define the SVE/SME/SME2 thread status structures
 // flavors, and sizes so this can build against an
 // older SDK which does not have these definitions
 // yet.