[02/13,gdb/symtab] Check effect in parent_map::set_parent
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
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" == 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
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
@@ -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: