[Lldb-commits] [lldb] dbc34e2 - [LLDB] Discard register flags where the size doesn't match the register

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-04-20T08:36:49Z
New Revision: dbc34e2bed10c18c78100a9e8517d9a0b2f6639d

URL: https://github.com/llvm/llvm-project/commit/dbc34e2bed10c18c78100a9e8517d9a0b2f6639d
DIFF: https://github.com/llvm/llvm-project/commit/dbc34e2bed10c18c78100a9e8517d9a0b2f6639d.diff

LOG: [LLDB] Discard register flags where the size doesn't match the register

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.

Reviewed By: jasonmolenda

Differential Revision: https://reviews.llvm.org/D148715

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index e468c82382d1b..087344198e226 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4341,8 +4341,19 @@ bool ParseRegisters(
           // 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

diff  --git a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
index 2c930d6773c5d..3ccf3ccae6e9b 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
@@ -319,6 +319,20 @@ def test_flags_requried_attributes(self):
 
         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 @@ def test_xml_includes_multiple(self):
             '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 @@ def test_xml_includes_flags_redefined(self):
             '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)"])