[PATCH] [gdb/symtab] Add assert in free_cached_comp_units constructor

[PATCH] [gdb/symtab] Add assert in free_cached_comp_units constructor

[Tom de Vries]

I wrote a patch containing:
...
      dw2_instantiate_symtab (cu->per_cu, ...);
...
and ran into a use-after-free at a following use of cu.

The problem is that dw2_instantiate_symtab contains:
...
      free_cached_comp_units freer (per_objfile);
...
and that the destructor does:
...
  ~free_cached_comp_units ()
  {
    m_per_objfile->remove_all_cus ();
  }
...
which also frees the cu we used in the cu->per_cu argument to
dw2_instantiate_symtab.

Detect this situation using an assert in the free_cached_comp_units
constructor.

Tested on aarch64-linux.
---
 gdb/dwarf2/read.c | 3 +++
 gdb/dwarf2/read.h | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index c761b732819..0dd0e944597 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -902,6 +902,9 @@ class free_cached_comp_units
   explicit free_cached_comp_units (dwarf2_per_objfile *per_objfile)
     : m_per_objfile (per_objfile)
   {
+    /* The destructor frees all cached comp units, including ones currently
+       cached, so check that there are no currently cached comp units.  */
+    gdb_assert (m_per_objfile->nr_of_cus () == 0);
   }
 
   ~free_cached_comp_units ()
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 15dd2abf3a1..603fe089d6a 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -969,6 +969,12 @@ struct dwarf2_per_objfile
   /* Free all cached compilation units.  */
   void remove_all_cus ();
 
+  /* Return the number of cached compilation units.  */
+  size_t nr_of_cus () const
+  {
+    return m_dwarf2_cus.size ();
+  }
+
   /* Increase the age counter on each CU compilation unit and free
      any that are too old.  */
   void age_comp_units ();

base-commit: a8740b7533b4df34d4d7bc5be9bb2108d019fce5
-- 
2.51.0

Re: [PATCH] [gdb/symtab] Add assert in free_cached_comp_units constructor

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Detect this situation using an assert in the free_cached_comp_units
Tom> constructor.

Seems fine to me.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] [gdb/symtab] Add assert in free_cached_comp_units constructor

[Tom de Vries]

On 5/28/26 6:43 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Detect this situation using an assert in the free_cached_comp_units
> Tom> constructor.
> 
> Seems fine to me.

Thanks for the review.

I committed this, but afterwards ran into trouble on x86_64-linux with 
test-cases gdb.ada/uninitialized-variable-record.exp and 
gdb.ada/uninitialized_vars.exp on x86_64-linux, so I've reverted this.

Thanks,
- Tom

> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

Re: [PATCH] [gdb/symtab] Add assert in free_cached_comp_units constructor

[Tom de Vries]

On 5/30/26 2:52 PM, Tom de Vries wrote:
> On 5/28/26 6:43 PM, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> Detect this situation using an assert in the free_cached_comp_units
>> Tom> constructor.
>>
>> Seems fine to me.
> 
> Thanks for the review.
> 
> I committed this, but afterwards ran into trouble on x86_64-linux with 
> test-cases gdb.ada/uninitialized-variable-record.exp and gdb.ada/ 
> uninitialized_vars.exp on x86_64-linux, so I've reverted this.
> 

I've investigated this, and found that this is due to calls to load_cu 
in places other than dw2_do_instantiate_symtab.

For test-case gdb.ada/uninitialized_vars.exp, it's 
dwarf2_fetch_die_loc_cu_off.

I do wonder if ~free_cached_comp_units is a bit overeager, and should 
refrain from deleting cached comp units that were present at 
construction time.

Anyway, the assert detected the use-after-free I created, but just 
doesn't hold in general.

Thanks,
- Tom

> Thanks,
> - Tom
> 
>>
>> Approved-By: Tom Tromey <tom@tromey.com>
>>
>> Tom
> 

Re: [PATCH] [gdb/symtab] Add assert in free_cached_comp_units constructor

[Simon Marchi]

On 6/1/26 4:02 AM, Tom de Vries wrote:
> On 5/30/26 2:52 PM, Tom de Vries wrote:
>> On 5/28/26 6:43 PM, Tom Tromey wrote:
>>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>>
>>> Tom> Detect this situation using an assert in the free_cached_comp_units
>>> Tom> constructor.
>>>
>>> Seems fine to me.
>>
>> Thanks for the review.
>>
>> I committed this, but afterwards ran into trouble on x86_64-linux with test-cases gdb.ada/uninitialized-variable-record.exp and gdb.ada/ uninitialized_vars.exp on x86_64-linux, so I've reverted this.
>>
> 
> I've investigated this, and found that this is due to calls to load_cu in places other than dw2_do_instantiate_symtab.
> 
> For test-case gdb.ada/uninitialized_vars.exp, it's dwarf2_fetch_die_loc_cu_off.
> 
> I do wonder if ~free_cached_comp_units is a bit overeager, and should refrain from deleting cached comp units that were present at construction time.
> 
> Anyway, the assert detected the use-after-free I created, but just doesn't hold in general.

Let's try to understand why things are the way they are currently.

Why do we want to delete the just created dwarf2_cus in
dw2_instantiate_symtab?  I presume it's because the chances of them
being useful again are slim.  We created the GDB types and symbols, we
don't need to keep the DIE structure  loaded in memory, so it's better
to free up the memory.

The cases where the dwarf2_cus are needed again later appear to be when
evaluating a some DWARF operator that refers to other DIEs directly,
such as DW_OP_call*, DW_OP_implicit_pointer, DW_OP_GNU_variable_value,
etc.

 - dwarf2_fetch_die_loc_sect_off
 - dwarf2_fetch_die_loc_cu_off
 - dwarf2_fetch_constant_bytes
 - dwarf2_fetch_die_type_sect_off

In those cases we re-load the right dwarf2_cu in memory to be able to
look up the DIE and get what we want from it.  In those cases, we don't
free up the just-loaded dwarf2_cu right away, because it presumably has
good chances of being needed again in the near future, for other
operators.  We instead use the "age_comp_units" mechanism, which frees
the dwarf2_cus once they have been sitting there unused for a while.

I am unable to reproduce the gdb.ada failures, but the case that you
looked at appears to be one where a dwarf2_cu was loaded by one of those
"dwarf2_fetch_*" functions, while evaluating a DWARF expression, and
then dw2_instantiate_symtab was called to expand a compunit into full
symbols.  So free_cached_comp_units deletes the dwarf2_cu previously
cached by the "dwarf2_fetch_*" functions.  I don't think this is wrong
(as in a correctness bug), so I don't think the assert was right, it
might just be ineffcient cache usage.  I don't know if it's problematic
enough to be worth fixing, but if you can think of a simple solution for
dw2_instantiate_symtab not to delete the pre-existing dwarf2_cus, we
could consider it.

It would perhaps be good to investigate whether this "age comp units"
machinery is still working as initially intended.  It's possible that
will all the refactors we've done over the years, it's not working as
intended.  And because it's just a cache, it would not break any tests,
but it could cause peformance problems.  While searching the code, I
looked where `cu->last_used` is reset, to prevent the CU from being
freed by age_comp_units.  It is reset in maybe_queue_comp_unit, which is
itself called in:

 - follow_die_sig_1
 - process_imported_unit_die
 - follow_die_offset

Note that those are all used in cross-CU reference cases, when a CU
needs something from another CU.

`age_comp_units()` is called in:

 - dw2_do_instantiate_symtab, when done expanding the symtab(s)
 - dwarf2_fetch_die_loc_sect_off, when done fetching the location info

Some fishy things I spotted:

 - The age_comp_units() call in dw2_do_instantiate_symtab suggests that
   the comp unit aging system was also meant to be used when expanding
   symtabs.  The fact that all the places that reset `cu->last_used` are
   some that handle cross-CU references also suggests this.  The idea
   might have been that if a CU refers to another CU (via DW_AT_import
   for instance), then there is a good chance that subsequent CUs will
   also refer to that second CUs.  So when you're done expanding the
   first CU, better keep that second CU in cache for when you'll be
   expanding more CUs.  However, if we free up all cached CUs in
   dw2_instantiate_symtab via free_cached_comp_units, doesn't it defeat
   the purpose?  Why call age_comp_units()  in dw2_do_instantiate_symtab
   if we're going to free them all up in the caller anyway?

 - Calling dwarf2_fetch_die_loc_sect_off ages the comp units, but
   shouldn't it also reset the `cu->last_used` field of the CU from
   which we found the info?  Otherwise, repeated calls to
   dwarf2_fetch_die_loc_sect_off targetting the same CU will cause that
   CU to be freed, even if we just used it and will keep needing it
   (causing it to be re-loaded from scratch the next time).

Simon

Re: [PATCH] [gdb/symtab] Add assert in free_cached_comp_units constructor

[Tom de Vries]

On 6/1/26 5:18 PM, Simon Marchi wrote:
> On 6/1/26 4:02 AM, Tom de Vries wrote:
>> On 5/30/26 2:52 PM, Tom de Vries wrote:
>>> On 5/28/26 6:43 PM, Tom Tromey wrote:
>>>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>>>
>>>> Tom> Detect this situation using an assert in the free_cached_comp_units
>>>> Tom> constructor.
>>>>
>>>> Seems fine to me.
>>>
>>> Thanks for the review.
>>>
>>> I committed this, but afterwards ran into trouble on x86_64-linux with test-cases gdb.ada/uninitialized-variable-record.exp and gdb.ada/ uninitialized_vars.exp on x86_64-linux, so I've reverted this.
>>>
>>
>> I've investigated this, and found that this is due to calls to load_cu in places other than dw2_do_instantiate_symtab.
>>
>> For test-case gdb.ada/uninitialized_vars.exp, it's dwarf2_fetch_die_loc_cu_off.
>>
>> I do wonder if ~free_cached_comp_units is a bit overeager, and should refrain from deleting cached comp units that were present at construction time.
>>
>> Anyway, the assert detected the use-after-free I created, but just doesn't hold in general.
> 
> Let's try to understand why things are the way they are currently.
> 
> Why do we want to delete the just created dwarf2_cus in
> dw2_instantiate_symtab?  I presume it's because the chances of them
> being useful again are slim.  We created the GDB types and symbols, we
> don't need to keep the DIE structure  loaded in memory, so it's better
> to free up the memory.
> 
> The cases where the dwarf2_cus are needed again later appear to be when
> evaluating a some DWARF operator that refers to other DIEs directly,
> such as DW_OP_call*, DW_OP_implicit_pointer, DW_OP_GNU_variable_value,
> etc.
> 
>   - dwarf2_fetch_die_loc_sect_off
>   - dwarf2_fetch_die_loc_cu_off
>   - dwarf2_fetch_constant_bytes
>   - dwarf2_fetch_die_type_sect_off
> 
> In those cases we re-load the right dwarf2_cu in memory to be able to
> look up the DIE and get what we want from it.  In those cases, we don't
> free up the just-loaded dwarf2_cu right away, because it presumably has
> good chances of being needed again in the near future, for other
> operators.  We instead use the "age_comp_units" mechanism, which frees
> the dwarf2_cus once they have been sitting there unused for a while.
> 
> I am unable to reproduce the gdb.ada failures, but the case that you
> looked at appears to be one where a dwarf2_cu was loaded by one of those
> "dwarf2_fetch_*" functions, while evaluating a DWARF expression, and
> then dw2_instantiate_symtab was called to expand a compunit into full
> symbols.  So free_cached_comp_units deletes the dwarf2_cu previously
> cached by the "dwarf2_fetch_*" functions.  I don't think this is wrong
> (as in a correctness bug), so I don't think the assert was right, it
> might just be ineffcient cache usage.  I don't know if it's problematic
> enough to be worth fixing, but if you can think of a simple solution for
> dw2_instantiate_symtab not to delete the pre-existing dwarf2_cus, we
> could consider it.
> 
> It would perhaps be good to investigate whether this "age comp units"
> machinery is still working as initially intended.  It's possible that
> will all the refactors we've done over the years, it's not working as
> intended.  And because it's just a cache, it would not break any tests,
> but it could cause peformance problems.  While searching the code, I
> looked where `cu->last_used` is reset, to prevent the CU from being
> freed by age_comp_units.  It is reset in maybe_queue_comp_unit, which is
> itself called in:
> 
>   - follow_die_sig_1
>   - process_imported_unit_die
>   - follow_die_offset
> 
> Note that those are all used in cross-CU reference cases, when a CU
> needs something from another CU.
> 
> `age_comp_units()` is called in:
> 
>   - dw2_do_instantiate_symtab, when done expanding the symtab(s)
>   - dwarf2_fetch_die_loc_sect_off, when done fetching the location info
> 
> Some fishy things I spotted:
> 
>   - The age_comp_units() call in dw2_do_instantiate_symtab suggests that
>     the comp unit aging system was also meant to be used when expanding
>     symtabs.  The fact that all the places that reset `cu->last_used` are
>     some that handle cross-CU references also suggests this.  The idea
>     might have been that if a CU refers to another CU (via DW_AT_import
>     for instance), then there is a good chance that subsequent CUs will
>     also refer to that second CUs.  So when you're done expanding the
>     first CU, better keep that second CU in cache for when you'll be
>     expanding more CUs.  However, if we free up all cached CUs in
>     dw2_instantiate_symtab via free_cached_comp_units, doesn't it defeat
>     the purpose?  Why call age_comp_units()  in dw2_do_instantiate_symtab
>     if we're going to free them all up in the caller anyway?
> 
>   - Calling dwarf2_fetch_die_loc_sect_off ages the comp units, but
>     shouldn't it also reset the `cu->last_used` field of the CU from
>     which we found the info?  Otherwise, repeated calls to
>     dwarf2_fetch_die_loc_sect_off targetting the same CU will cause that
>     CU to be freed, even if we just used it and will keep needing it
>     (causing it to be re-loaded from scratch the next time).
> 

Hi Simon,

thanks for the comments.

I'm not sure when I'll have the time to follow up on all this, so I've 
filed a review PR ( https://sourceware.org/bugzilla/show_bug.cgi?id=34243 ).

Thanks,
- Tom

> Simon