diff mbox series

gdb: check for groups with duplicate names in reggroups:add

Message ID 20221018141733.29298-1-simon.marchi@efficios.com
State Committed
Commit 9454c9ce88b25646d279feed329c9cdba69b4905
Headers
Series gdb: check for groups with duplicate names in reggroups:add |

Commit Message

Simon Marchi Oct. 18, 2022, 2:17 p.m. UTC 
  In the downstream ROCm GDB port, we would create multiple register
groups with duplicate names.  While it did not really hurt, it certainly
wasn't the intent.  And I don't think it ever makes sense to do so.

To catch these, change the assert in reggroups::add to check for
duplicate names.  It's no longer necessary to check for duplicate
reggroup pointers, because adding the same group twice would be caught
by the duplicate name check.

Change-Id: Id216a58acf91f1b314d3cba2d02de73656f8851d
---
 gdb/reggroups.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Tom Tromey Oct. 18, 2022, 6:42 p.m. UTC | #1 
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> In the downstream ROCm GDB port, we would create multiple register
Simon> groups with duplicate names.  While it did not really hurt, it certainly
Simon> wasn't the intent.  And I don't think it ever makes sense to do so.

Simon> To catch these, change the assert in reggroups::add to check for
Simon> duplicate names.  It's no longer necessary to check for duplicate
Simon> reggroup pointers, because adding the same group twice would be caught
Simon> by the duplicate name check.

I haven't looked but would it be possible for malformed XML from the
target to trigger this assert?

Tom
Simon Marchi Oct. 18, 2022, 6:54 p.m. UTC | #2 
On 10/18/22 14:42, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> In the downstream ROCm GDB port, we would create multiple register
> Simon> groups with duplicate names.  While it did not really hurt, it certainly
> Simon> wasn't the intent.  And I don't think it ever makes sense to do so.
> 
> Simon> To catch these, change the assert in reggroups::add to check for
> Simon> duplicate names.  It's no longer necessary to check for duplicate
> Simon> reggroup pointers, because adding the same group twice would be caught
> Simon> by the duplicate name check.
> 
> I haven't looked but would it be possible for malformed XML from the
> target to trigger this assert?
> 
> Tom

I don't think so, because the target description support code creates
the groups as it finds them while iterating registers.  It only creates
a group if it doesn't exist already:

  https://gitlab.com/gnutools/binutils-gdb/-/blob/152cc35ebff44eb06a00364ca1dbcf5fca6772b4/gdb/target-descriptions.c#L1124-1125

In other words, the XML tdesc doesn't have a list of groups, where if a
group was duplicated, it could trigger this assert.

Simon
Tom Tromey Oct. 18, 2022, 7:01 p.m. UTC | #3 
Simon> I don't think so, because the target description support code creates
Simon> the groups as it finds them while iterating registers.

Thanks, in that case this seems like a good idea to me.

Tom
Simon Marchi Oct. 18, 2022, 8:47 p.m. UTC | #4 
On 10/18/22 15:01, Tom Tromey wrote:
> Simon> I don't think so, because the target description support code creates
> Simon> the groups as it finds them while iterating registers.
> 
> Thanks, in that case this seems like a good idea to me.
> 
> Tom

I am asking, because these things are still new, so better safe than
sorry: are you fine with me adding your Approved-By?

Simon
Tom Tromey Oct. 19, 2022, 1:54 a.m. UTC | #5 
Simon> I am asking, because these things are still new, so better safe than
Simon> sorry: are you fine with me adding your Approved-By?

Yes, definitely.

thanks,
Tom
Simon Marchi Oct. 19, 2022, 2:12 a.m. UTC | #6 
On 2022-10-18 21:54, Tom Tromey wrote:
> Simon> I am asking, because these things are still new, so better safe than
> Simon> sorry: are you fine with me adding your Approved-By?
> 
> Yes, definitely.
> 
> thanks,
> Tom

Thanks, pushed.

Simon
diff mbox series

Patch

diff --git a/gdb/reggroups.c b/gdb/reggroups.c
index 8e4af303c545..a012bf085265 100644
--- a/gdb/reggroups.c
+++ b/gdb/reggroups.c
@@ -71,8 +71,13 @@  struct reggroups
   void add (const reggroup *group)
   {
     gdb_assert (group != nullptr);
-    gdb_assert (std::find (m_groups.begin(), m_groups.end(), group)
-		== m_groups.end());
+
+    auto find_by_name = [group] (const reggroup *g)
+      {
+	return streq (group->name (), g->name ());
+      };
+    gdb_assert (std::find_if (m_groups.begin (), m_groups.end (), find_by_name)
+		== m_groups.end ());
 
     m_groups.push_back (group);
   }