[Lldb-commits] [PATCH] D148715: [LLDB] Discard register flags where the size doesn't match the register

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

In the particular case I was looking at I autogenerated a 128 bit set
of flags that is only 64 bit. This doesn't crash lldb but it was certainly
not expected.

I suspect that we would have crashed if the top 64 bits weren't
marked as unused (or at least invoked some very undefined behaviour).

When this happens, log the details and ignore the flags. Like this:

  Size of register flags TTBR0_EL1_flags (16 bytes) for register TTBR0_EL1 does not match the register size (8 bytes). Ignoring this set of flags.

Turns out a few of the tests relied on this bug so I have updated
them and added a specific test for this case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148715

Files:
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py


Index: lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
===================================================================
--- lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
@@ -319,6 +319,20 @@
 
         self.expect("register read cpsr", substrs=["(C = 1)"])
 
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_flags_register_size_mismatch(self):
+        # If the size of the flag set found does not match the size of the
+        # register, we discard the flags.
+        self.setup_register_test("""\
+          <flags id="cpsr_flags" size="8">
+            <field name="C" start="0" end="0"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>""")
+
+        self.expect("register read cpsr", substrs=["(C = 1)"], matching=False)
+
     @skipIfXmlSupportMissing
     @skipIfRemote
     def test_flags_set_even_if_format_set(self):
@@ -439,7 +453,7 @@
             'core.xml' : dedent("""\
                 <?xml version="1.0"?>
                 <feature name="org.gnu.gdb.aarch64.core">
-                  <flags id="x0_flags" size="4">
+                  <flags id="x0_flags" size="8">
                     <field name="B" start="0" end="0"/>
                   </flags>
                   <reg name="pc" bitsize="64"/>
@@ -475,23 +489,23 @@
             'core.xml' : dedent("""\
                 <?xml version="1.0"?>
                 <feature name="org.gnu.gdb.aarch64.core">
-                  <flags id="my_flags" size="4">
+                  <flags id="my_flags" size="8">
                     <field name="correct" start="0" end="0"/>
                   </flags>
                   <reg name="pc" bitsize="64"/>
                   <reg name="x0" regnum="0" bitsize="64" type="my_flags"/>
                 </feature>"""),
-            # The my_flags here is ignored, so cpsr will use the my_flags from above.
+            # The my_flags here is ignored, so x1 will use the my_flags from above.
             'core-2.xml' : dedent("""\
                 <?xml version="1.0"?>
                 <feature name="org.gnu.gdb.aarch64.core">
-                  <flags id="my_flags" size="4">
+                  <flags id="my_flags" size="8">
                     <field name="incorrect" start="0" end="0"/>
                   </flags>
-                  <reg name="cpsr" regnum="33" bitsize="32" type="my_flags"/>
+                  <reg name="x1" regnum="33" bitsize="64" type="my_flags"/>
                 </feature>
             """),
         })
 
         self.expect("register read x0", substrs=["(correct = 1)"])
-        self.expect("register read cpsr", substrs=["(correct = 1)"])
+        self.expect("register read x1", substrs=["(correct = 1)"])
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4342,8 +4342,19 @@
           // gdb_type could reference some flags type defined in XML.
           llvm::StringMap<std::unique_ptr<RegisterFlags>>::iterator it =
               registers_flags_types.find(gdb_type);
-          if (it != registers_flags_types.end())
-            reg_info.flags_type = it->second.get();
+          if (it != registers_flags_types.end()) {
+            auto flags_type = it->second.get();
+            if (reg_info.byte_size == flags_type->GetSize())
+              reg_info.flags_type = flags_type;
+            else
+              LLDB_LOGF(log,
+                        "ProcessGDBRemote::ParseRegisters Size of register "
+                        "flags %s (%d bytes) for "
+                        "register %s does not match the register size (%d "
+                        "bytes). Ignoring this set of flags.",
+                        flags_type->GetID().c_str(), flags_type->GetSize(),
+                        reg_info.name.AsCString(), reg_info.byte_size);
+          }
 
           // There's a slim chance that the gdb_type name is both a flags type
           // and a simple type. Just in case, look for that too (setting both


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D148715.514942.patch
Type: text/x-patch
Size: 4324 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20230419/a34a94f4/attachment-0001.bin>