diff mbox series

[v2,03/13,gdb/symtab] Handle nullptr parent in parent_map::set_parent

Message ID 20231212173239.16793-4-tdevries@suse.de
State New
Headers
Series Fix gdb.cp/breakpoint-locs.exp |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm 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 Dec. 12, 2023, 5:32 p.m. UTC 
  Set_parent uses m_die_range_map.set_empty, which doesn't allow
parent_entry == nullptr.

So it may be necessary to guard calls to set_parent with
"if (parent_entry != nullptr)".

Fix this by handling the parent_entry == nullptr case in set_parent.

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

Comments

Tom Tromey Dec. 12, 2023, 6:34 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Set_parent uses m_die_range_map.set_empty, which doesn't allow
Tom> parent_entry == nullptr.

Tom> So it may be necessary to guard calls to set_parent with
Tom> "if (parent_entry != nullptr)".

Tom> Fix this by handling the parent_entry == nullptr case in set_parent.

It seems like this must be a programming error somewhere?
Currently the only caller is guarded:

  if (parent_entry != nullptr)
...
      m_die_range_map.set_empty (start, end, (void *) parent_entry);

Tom
Tom de Vries Dec. 13, 2023, 8:25 a.m. UTC | #2 
On 12/12/23 19:34, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Set_parent uses m_die_range_map.set_empty, which doesn't allow
> Tom> parent_entry == nullptr.
> 
> Tom> So it may be necessary to guard calls to set_parent with
> Tom> "if (parent_entry != nullptr)".
> 
> Tom> Fix this by handling the parent_entry == nullptr case in set_parent.
> 
> It seems like this must be a programming error somewhere?
> Currently the only caller is guarded:
> 
>    if (parent_entry != nullptr)
> ...
>        m_die_range_map.set_empty (start, end, (void *) parent_entry);

Yes, and I've left that in place because I couldn't convince myself that 
this wouldn't introduce a performance regression, but perhaps it doesn't 
matter and we should drop the check there.

I'm not sure why you say programming error.  I know using 
addrmap::set_empty (..., nullptr) is a programming error.

This patch attempts to make sure that using parent_map::set_parent (..., 
nullptr) is not a programming error.

Anyway, this is needed for a set_parent added by "[gdb/symtab] Keep 
track of all parents for cooked index".

Thanks,
- Tom
Tom Tromey Dec. 13, 2023, 8:11 p.m. UTC | #3 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I'm not sure why you say programming error.  I know using
Tom> addrmap::set_empty (..., nullptr) is a programming error.

I just mean that, if it happens, it seems like it must be a bug
elsewhere.  But it doesn't matter much I suppose.

Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index fa274b37c20..3c43717fc33 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -263,7 +263,9 @@  class parent_map
   void set_parent (CORE_ADDR start, CORE_ADDR end,
 		   const cooked_index_entry *parent_entry)
   {
-    m_parent_map.set_empty (start, end, (void *)parent_entry);
+    /* Calling set_empty with nullptr is currently not allowed.  */
+    if (parent_entry != nullptr)
+      m_parent_map.set_empty (start, end, (void *)parent_entry);
   }
 
 private: