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

[Simon Marchi <simark@simark.ca>]

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