diff mbox series

[02/13,gdb/symtab] Check effect in parent_map::set_parent

Message ID 20231002125051.29911-3-tdevries@suse.de
State Superseded
Headers
Series Fix gdb.cp/breakpoint-locs.exp |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Tom de Vries Oct. 2, 2023, 12:50 p.m. UTC 
  Set_parent uses m_die_range_map.set_empty to set the parent of a range.

If part of the range is already set, it remains the same.

If the entire range is already set, the set_empty has no effect, silently.

Fix this by verifying that calling set_empty has the desired effect on the
start and end points of the range.

Tested on x86_64-linux.
---
 gdb/dwarf2/cooked-index.h | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Tom Tromey Oct. 20, 2023, 7:51 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Set_parent uses m_die_range_map.set_empty to set the parent of a range.
Tom> If part of the range is already set, it remains the same.

Tom> If the entire range is already set, the set_empty has no effect, silently.

Tom> Fix this by verifying that calling set_empty has the desired effect on the
Tom> start and end points of the range.

This seems like it might be better as a unit test.

Tom
Tom de Vries Nov. 30, 2023, 2:05 p.m. UTC | #2 
On 10/20/23 21:51, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Set_parent uses m_die_range_map.set_empty to set the parent of a range.
> Tom> If part of the range is already set, it remains the same.
> 
> Tom> If the entire range is already set, the set_empty has no effect, silently.
> 
> Tom> Fix this by verifying that calling set_empty has the desired effect on the
> Tom> start and end points of the range.
> 
> This seems like it might be better as a unit test.

We're trying to check the uses of set_parent, not the implementation, so 
a unit test doesn't help there.

Consider the following sequence:
- set_parent (3, 6, b)
- set_parent (1, 10, a)

Now the parent map looks like:
- [1-2] : a
- [3-6] : b
- [7-10]: a

Now say we accidentally execute this in the opposite order:
- set_parent (1, 10, a)
- set_parent (3, 6, b)

Now the parent map looks like this instead:
- [1-10]: a

The second set_parent has no effect, and this happens silently.  The 
assert tries to make this mistake visible.

It could be fitting to add some kind of gdb_checking_assert scheme for 
this, which is off by default in releases, and can be switched on by 
some means, if this is considered too expensive.

Anyway, since it raises questions, I'll drop this patch for now, but I'd 
like to note that it was useful for me in development of this patch series.

Thanks,
- Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index a029fcf2cc9..9900dbb84a7 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -254,6 +254,13 @@  class parent_map
 		   const cooked_index_entry *parent_entry)
   {
     m_parent_map.set_empty (start, end, (void *)parent_entry);
+
+    /* Assert that set_parent has the desired effect.  This is not trivial due
+       to how set_empty works.  If the range already has been set before, it
+       has no effect.  */
+    gdb_assert (m_parent_map.find (start) == parent_entry);
+    if (end != start)
+      gdb_assert (m_parent_map.find (end) == parent_entry);
   }
 
 private: