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

[Tom de Vries <tdevries@suse.de>]

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