[PATCH] gdb: check for groups with duplicate names in reggroups:add

[PATCH] gdb: check for groups with duplicate names in reggroups:add

[Simon Marchi]

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(-)

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);
   }
-- 
2.38.0

Re: [PATCH] gdb: check for groups with duplicate names in reggroups:add

[Tom Tromey]

>>>>> "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

Re: [PATCH] gdb: check for groups with duplicate names in reggroups:add

[Simon Marchi]

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

Re: [PATCH] gdb: check for groups with duplicate names in reggroups:add

[Tom Tromey]

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

Re: [PATCH] gdb: check for groups with duplicate names in reggroups:add

[Simon Marchi]

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

Re: [PATCH] gdb: check for groups with duplicate names in reggroups:add

[Tom Tromey]

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

Re: [PATCH] gdb: check for groups with duplicate names in reggroups:add

[Simon Marchi]

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