[LLDB] Round XML register bitsize to byte boundary

[omjavaid]

Summary

This patch allows LLDB to accept register sizes which are not aligned
to 8 bits bitsize boundary. This fixes a crash in LLDB when connecting
to OpenOCD stub. GDB xml description allows for non-aligned bit lengths
but they are rounded off to nearest byte during transfer. In case of
OpenOCD some of SOC specific system registers were less than a single
byte in length and were causing LLDB to crash.

[mgorny]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4261

I suppose LLDB crashed because byte_size was zero? Maybe add an assert for that then.

[omjavaid]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4360–4361

@mgorny the assert already exists but then we also want to allow bit sized registers although they ll be viewed as byte length for now.

[mgorny]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4360–4361

Ah, right. I suppose you could skip zero-byte registers though. That should amend the assert with better release behavior.

[omjavaid]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4360–4361

on a second thought, I dont see a zero sized register being sent by stub as a big enough reason to abort the whole debug session unless its one of GPRs. May be we skip the assert altogether and replace it with an error message.
What do you think?

[mgorny]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4360–4361

Yes, you are correct. Probably LLDB_LOG would go in line with how we handle these things.

[omjavaid]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4360–4361

LLDB_LOG will hide message from user unless log is enabled. I think user must be notified that register is zero sized and thats why you wont be able to see it in register read. Similar to the way we notify user about unhandled register attibutes like "save-restore".

[labath]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4260–4262

llvm::divideCeil(reg_info.byte_size, CHAR_BIT), perhaps

4360–4361

Yeah, I don't think crashing is a good response to the stub sending us nonsensical register definitions. Though that seems like a separate issue..

[labath]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4360–4361

BTW, I came very close to deleting that printf when I was touching this code last month.
FWIW, my hierarchy is:
user-facing warning (though I don't know how would that be implemented here) >> log entry >> nothing >> raw printf

[ted]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4360–4361

+1 on not crashing. The remote sending us garbage shouldn't cause us to crash - we should try to make sense of the garbage, and if we can't, error out gracefully. Better to print an error and ignore the register than crash or abort the session.

[omjavaid]

This addresses review comments converts printfs to log messages.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

feature_node.ForEachChildElementWithName(
         reg_node.ForEachAttribute([&target_info, &gdb_group, &gdb_type,
                                    &encoding_set, &format_set, &reg_info](
                                       const llvm::StringRef &name,
                                       const llvm::StringRef &value) -> bool {
           if (name == "name") {
             reg_info.name.SetString(value);
           } else if (name == "bitsize") {
             if (llvm::to_integer(value, reg_info.byte_size))
-              reg_info.byte_size /= CHAR_BIT;
+              reg_info.byte_size =
+                  llvm::divideCeil(reg_info.byte_size, CHAR_BIT);
           } else if (name == "type") {
             gdb_type = value.str();
           } else if (name == "group") {
             gdb_group = value.str();
           } else if (name == "regnum") {
             llvm::to_integer(value, reg_info.regnum_remote);
           } else if (name == "offset") {
             llvm::to_integer(value, reg_info.byte_offset);
           } else if (name == "altname") {
feature_node.ForEachChildElementWithName(
 
             reg_info.dwarf_opcode_bytes.resize(dwarf_opcode_len);
             StringExtractor opcode_extractor(opcode_string);
             uint32_t ret_val =
                 opcode_extractor.GetHexBytesAvail(reg_info.dwarf_opcode_bytes);
             assert(dwarf_opcode_len == ret_val);
             UNUSED_IF_ASSERT_DISABLED(ret_val);
           } else {
-            printf("unhandled attribute %s = %s\n", name.data(), value.data());
+            Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(
+                GDBR_LOG_PROCESS));
+            LLDB_LOGF(log,
+                      "ProcessGDBRemote::%s unhandled reg attribute %s = %s",
+                      __FUNCTION__, name.data(), value.data());
           }
           return true; // Keep iterating through all attributes
         });
 
         if (!gdb_type.empty() && !(encoding_set || format_set)) {
           if (llvm::StringRef(gdb_type).startswith("int")) {
             reg_info.format = eFormatHex;
             reg_info.encoding = eEncodingUint;
feature_node.ForEachChildElementWithName(
           if (!gdb_group.empty()) {
             reg_info.set_name.SetCString(gdb_group.c_str());
           } else {
             // If no register group name provided anywhere,
             // we'll create a 'general' register set
             reg_info.set_name.SetCString("general");
           }
         }
 
-        assert(reg_info.byte_size != 0);
+        if (reg_info.byte_size == 0) {
+          Log *log(
+              ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
+          LLDB_LOGF(log,
+                    "ProcessGDBRemote::%s Skipping zero bitsize register %s",
+                    __FUNCTION__, reg_info.name.AsCString());
+        } else
           registers.push_back(reg_info);
+
         return true; // Keep iterating through all "reg" elements
       });
   return true;
 }
 
 } // namespace
 
 // This method fetches a register description feature xml file from

lldb/test/API/functionalities/gdb_remote_client/TestArmRegisterDefinition.py

def test(self):
                         <reg name="r8" bitsize="32" type="uint32" group="general"/>
                         <reg name="r9" bitsize="32" type="uint32" group="general"/>
                         <reg name="r10" bitsize="32" type="uint32" group="general"/>
                         <reg name="r11" bitsize="32" type="uint32" group="general"/>
                         <reg name="r12" bitsize="32" type="uint32" group="general"/>
                         <reg name="sp" bitsize="32" type="data_ptr" group="general"/>
                         <reg name="lr" bitsize="32" type="uint32" group="general"/>
                         <reg name="pc" bitsize="32" type="code_ptr" group="general"/>
+                        <reg name="SYS0" bitsize="9" regnum="21" type="uint32" group="system"/>
+                        <reg name="SYS1" bitsize="8" regnum="22" type="uint32" group="system"/>
+                        <reg name="SYS2" bitsize="1" regnum="23" type="uint32" group="system"/>
+                        <reg name="SYS3" bitsize="7" regnum="24" type="uint32" group="system"/>
                         <reg name="xpsr" bitsize="32" regnum="25" type="uint32" group="general"/>
                         <reg name="MSP" bitsize="32" regnum="26" type="uint32" group="general"/>
                         <reg name="PSP" bitsize="32" regnum="27" type="uint32" group="general"/>
                         <reg name="PRIMASK" bitsize="32" regnum="28" type="uint32" group="general"/>
                         <reg name="BASEPRI" bitsize="32" regnum="29" type="uint32" group="general"/>
                         <reg name="FAULTMASK" bitsize="32" regnum="30" type="uint32" group="general"/>
                         <reg name="CONTROL" bitsize="32" regnum="31" type="uint32" group="general"/>
                         <reg name="FPSCR" bitsize="32" type="uint32" group="float"/>
def test(self):
                         </target>""", False
                 else:
                     return None, False
 
             def readRegister(self, regnum):
                 return "E01"
 
             def readRegisters(self):
-                return "20000000f8360020001000002fcb0008f8360020a0360020200c0020000000000000000000000000000000000000000000000000b87f0120b7d100082ed2000800000001b87f01200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
+                return "20000000f8360020001000002fcb0008f8360020a0360020200c0020000000000000000000000000000000000000000000000000b87f0120b7d100082ed20008addebeafbc00000001b87f01200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
 
             def haltReason(self):
                 return "S05"
 
             def qfThreadInfo(self):
                 return "mdead"
             
             def qC(self):
def test(self):
             interp.HandleCommand("target list", result)
             print(result.GetOutput())
 
         r0_valobj = process.GetThreadAtIndex(0).GetFrameAtIndex(0).FindRegister("r0")
         self.assertEqual(r0_valobj.GetValueAsUnsigned(), 0x20)
 
         pc_valobj = process.GetThreadAtIndex(0).GetFrameAtIndex(0).FindRegister("pc")
         self.assertEqual(pc_valobj.GetValueAsUnsigned(), 0x0800d22e)
+
+        sys_valobj = process.GetThreadAtIndex(0).GetFrameAtIndex(0).FindRegister("SYS0")
+        self.assertEqual(sys_valobj.GetValueAsUnsigned(), 0xdead)
+
+        sys_valobj = process.GetThreadAtIndex(0).GetFrameAtIndex(0).FindRegister("SYS1")
+        self.assertEqual(sys_valobj.GetValueAsUnsigned(), 0xbe)
+
+        sys_valobj = process.GetThreadAtIndex(0).GetFrameAtIndex(0).FindRegister("SYS2")
+        self.assertEqual(sys_valobj.GetValueAsUnsigned(), 0xaf)
+
+        sys_valobj = process.GetThreadAtIndex(0).GetFrameAtIndex(0).FindRegister("SYS3")
+        self.assertEqual(sys_valobj.GetValueAsUnsigned(), 0xbc)