[PATCH] gdb: restore nullptr check in compunit_symtab::find_call_site

[PATCH] gdb: restore nullptr check in compunit_symtab::find_call_site

[Simon Marchi]

Commit de2b4ab50de ("Convert dwarf2_cu::call_site_htab to new hash
table") removed this nullptr check for no good reason.  This causes a
crash if `m_call_site_htab` is not set, as shown in PR 32410.  My guess
is that when doing this change, I tried to make `m_call_site_htab` not a
pointer, removed this check, then realized it wasn't so obvious, and
forgot to re-add the check.

Change-Id: I455e00cdc0519dfb412dc7826d17a839b77aae69
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32410
---
 gdb/symtab.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index edc19ff5c155..30c22dcea38c 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -396,6 +396,9 @@ linetable_entry::pc (const struct objfile *objfile) const
 call_site *
 compunit_symtab::find_call_site (CORE_ADDR pc) const
 {
+  if (m_call_site_htab == nullptr)
+    return nullptr;
+
   CORE_ADDR delta = this->objfile ()->text_section_offset ();
 
   if (auto it = m_call_site_htab->find (static_cast<unrelocated_addr> (pc - delta));

base-commit: d556cf2ec79ed5afbbdd523484954c6a520b5b73
-- 
2.47.1

Re: [PATCH] gdb: restore nullptr check in compunit_symtab::find_call_site

[Tom de Vries]

On 12/3/24 16:52, Simon Marchi wrote:
> Commit de2b4ab50de ("Convert dwarf2_cu::call_site_htab to new hash
> table") removed this nullptr check for no good reason.  This causes a
> crash if `m_call_site_htab` is not set, as shown in PR 32410.  My guess
> is that when doing this change, I tried to make `m_call_site_htab` not a
> pointer, removed this check, then realized it wasn't so obvious, and
> forgot to re-add the check.
> 

Hi Simon,

thanks for the quick fix.

I've tested it with the test-case on which the PR was reported, and gdb 
no longer crashes.  The test-case still produces two FAILs, but that 
looks unrelated.

LGTM.

Approved-By: Tom de Vries <tdevries@suse.de>

Thanks,
- Tom

> Change-Id: I455e00cdc0519dfb412dc7826d17a839b77aae69
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32410
> ---
>   gdb/symtab.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index edc19ff5c155..30c22dcea38c 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -396,6 +396,9 @@ linetable_entry::pc (const struct objfile *objfile) const
>   call_site *
>   compunit_symtab::find_call_site (CORE_ADDR pc) const
>   {
> +  if (m_call_site_htab == nullptr)
> +    return nullptr;
> +
>     CORE_ADDR delta = this->objfile ()->text_section_offset ();
>   
>     if (auto it = m_call_site_htab->find (static_cast<unrelocated_addr> (pc - delta));
> 
> base-commit: d556cf2ec79ed5afbbdd523484954c6a520b5b73

Re: [PATCH] gdb: restore nullptr check in compunit_symtab::find_call_site

[Simon Marchi]

On 2024-12-03 11:17, Tom de Vries wrote:
> On 12/3/24 16:52, Simon Marchi wrote:
>> Commit de2b4ab50de ("Convert dwarf2_cu::call_site_htab to new hash
>> table") removed this nullptr check for no good reason.  This causes a
>> crash if `m_call_site_htab` is not set, as shown in PR 32410.  My guess
>> is that when doing this change, I tried to make `m_call_site_htab` not a
>> pointer, removed this check, then realized it wasn't so obvious, and
>> forgot to re-add the check.
>>
> 
> Hi Simon,
> 
> thanks for the quick fix.
> 
> I've tested it with the test-case on which the PR was reported, and gdb no longer crashes.  The test-case still produces two FAILs, but that looks unrelated.
> 
> LGTM.
> 
> Approved-By: Tom de Vries <tdevries@suse.de>

Thanks,  pushed.

Simon

Re: [PATCH] gdb: restore nullptr check in compunit_symtab::find_call_site

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> Commit de2b4ab50de ("Convert dwarf2_cu::call_site_htab to new hash
Simon> table") removed this nullptr check for no good reason.  This causes a
Simon> crash if `m_call_site_htab` is not set, as shown in PR 32410.  My guess
Simon> is that when doing this change, I tried to make `m_call_site_htab` not a
Simon> pointer, removed this check, then realized it wasn't so obvious, and
Simon> forgot to re-add the check.

Yeah, someday maybe symtabs should be allocated the ordinary way.

On the one hand, obstacks are nice since they reduce free overhead.
But on the other hand, they mean we can't use destructors... and for
this case, I think there normally aren't "too many" symtabs, so it's
probably fine to make the change.

Simon> Change-Id: I455e00cdc0519dfb412dc7826d17a839b77aae69
Simon> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32410

This looks good to me, even obvious.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] gdb: restore nullptr check in compunit_symtab::find_call_site

[Simon Marchi]

On 2024-12-03 12:49, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> Commit de2b4ab50de ("Convert dwarf2_cu::call_site_htab to new hash
> Simon> table") removed this nullptr check for no good reason.  This causes a
> Simon> crash if `m_call_site_htab` is not set, as shown in PR 32410.  My guess
> Simon> is that when doing this change, I tried to make `m_call_site_htab` not a
> Simon> pointer, removed this check, then realized it wasn't so obvious, and
> Simon> forgot to re-add the check.
> 
> Yeah, someday maybe symtabs should be allocated the ordinary way.
> 
> On the one hand, obstacks are nice since they reduce free overhead.
> But on the other hand, they mean we can't use destructors... and for
> this case, I think there normally aren't "too many" symtabs, so it's
> probably fine to make the change.

We can still use destructors, we might just need to call it manually.
In fact, I was wondering if `compunit_symtab::finalize()` couldn't just
become `compunit_symtab::~compunit_symtab()` (it would still be called
manually at the same place.

In fact, can't `allocate_on_obstack` help us with that? What if you
have:

    struct compunit_symtab : allocate_on_obstack<compunit_symtab>
    {
      ...
    };

and then use:

    std::unique_ptr<compunit_symtab> uptr;

My understanding (although I haven't tried) is: when `uptr` gets
destroyed, it will call compunit_symtab's destructor, but won't free any
memory because the delete operator of allocate_on_obstack is a noop.  Do
I understand it correctly?

If I find some time, I would still like to try to make compunit_symtab /
symtab allocated normally and measure how expensive it is to free a few
thousands of those.  If the difference isn't significant, I would be
happy to get rid of that complexity.

Simon

Re: [PATCH] gdb: restore nullptr check in compunit_symtab::find_call_site

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> On the one hand, obstacks are nice since they reduce free overhead.
>> But on the other hand, they mean we can't use destructors... and for
>> this case, I think there normally aren't "too many" symtabs, so it's
>> probably fine to make the change.

Simon> We can still use destructors, we might just need to call it manually.

Yeah, that just doesn't seem significantly better to me.  It is more
like a spelling change.

Tom

Re: [PATCH] gdb: restore nullptr check in compunit_symtab::find_call_site

[Simon Marchi]

On 2024-12-03 14:43, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
>>> On the one hand, obstacks are nice since they reduce free overhead.
>>> But on the other hand, they mean we can't use destructors... and for
>>> this case, I think there normally aren't "too many" symtabs, so it's
>>> probably fine to make the change.
> 
> Simon> We can still use destructors, we might just need to call it manually.
> 
> Yeah, that just doesn't seem significantly better to me.  It is more
> like a spelling change.

I don't think it's just a spelling change, because a destructor would
invoke the destructor of fields.  So I think that if we started invoking
~compunit_symtab() manually (instead of compunit_symtab::finalize()),
we could start using non-trivially-destructible stuff directly as fields
of compunit_symtab.  In other words, m_call_site_htab wouldn't have to
be a pointer anymore.

Simon

Re: [PATCH] gdb: restore nullptr check in compunit_symtab::find_call_site

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> Yeah, that just doesn't seem significantly better to me.  It is more
>> like a spelling change.

Simon> I don't think it's just a spelling change, because a destructor would
Simon> invoke the destructor of fields.  So I think that if we started invoking
Simon> ~compunit_symtab() manually (instead of compunit_symtab::finalize()),
Simon> we could start using non-trivially-destructible stuff directly as fields
Simon> of compunit_symtab.  In other words, m_call_site_htab wouldn't have to
Simon> be a pointer anymore.

I thought you meant something like 'symtab->m_mumble.~unique_ptr()', as
opposed to 'delete symtab->m_mumble'.

So I see what you mean, but this also seems less good to me.
Currently the obstack allocator prohibits non-trivial destructors, and
this is a good thing, because it means it is harder to make a mistake
here -- adding a field that requires destruction will cause a
compile-time error.

I'd be reluctant to lift that restriction.

Another way forward is a slab allocator that is type-specific so it
knows how to run the destructors.  Inheriting from an interface class
and keeping objects on a list would also work for this purpose.

I somewhat doubt we'll need this kind of thing but it's fine by me if we
do.

Tom