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
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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
