Re: [PATCH v2] [gdb/symtab] Fix dwarf version of DWO TU

[Simon Marchi <simark@simark.ca>]

On 3/3/25 3:02 PM, Simon Marchi wrote:
> On 1/21/25 10:09 AM, Tom de Vries wrote:
>> On 1/21/25 14:32, Tom Tromey wrote:
>>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>>
>>> Tom> When running test-case gdb.ada/access_tagged_param.exp with target board
>>> Tom> fission, we run into:
>>> Tom> ...
>>> Tom> (gdb) break pck.adb:19^M
>>> Tom> gdb/dwarf2/read.h:289: internal-error: version: \
>>> Tom>   Assertion `m_dwarf_version != 0' failed.^M
>>> Tom> ...
>>>
>>> Tom> -      && (cu->per_cu->version () == 2
>>> Tom> +      && (cu->header.version == 2
>>>
>>> All the stuff in this area seems kind of horrible to me.  Like, first of
>>> all, how can this even fail?  It seems like the CU version should be set
>>> when setting up the reader. 
>>
>> Hi Tom,
>>
>> thanks for the review.
>>
>> The v1 ( https://sourceware.org/pipermail/gdb-patches/2024-October/212676.html ) takes the approach of making sure "cu->per_cu->version ()" is set.
>>
>> Do you prefer that one?
>>
>> Thanks,
>> - Tom
>>
>>> And if not, what's gone wrong there?
>>> Relatedly, can the other callers of version() fail in the same way?
>>> Finally there aren't really that many callers of this method so I wonder
>>> if it can be removed, maybe making this code less fragile.  Looking at
>>> it, I feel pretty sure if I needed a version check I'd probably write
>>> code to call this method but ... maybe that's unsafe?
>>>
>>> Tom
>>
> 
> My understanding of what Tom T said is that the
> dwarf2_per_cu_data::version method could be removed.  Callers could use
> the dwarf2_cu::header::version field, since they do have access to a
> dwarf2_cu anyway.  I will give that a shot.

I spoke too soon, there are uses of dwarf2_per_cu_data::version in loc.c
that my indexer hadn't picked up yet for some reason.  When those run,
the dwarf2_cu is probably gone.

It would be nice if we could ensure that the DWARF is set at the
creation of any dwarf2_per_cu_data, but it doesn't seem possible.  When
we read a .gdb_index index, we create some dwarf2_per_cu_data objects
out of it, but we don't know the DWARF version of those.  We will only
know when reading them with a cutu_reader.

So I see 3 options on the table:

 1. Tom's first patch would obtain the DWARF version of the from the
 skeleton unit, and pass it when creating the signatured_type object for
 the type unit that is stored in the dwo file

 2. A patch proposed in the bug just sets the field in the "type unit in
 dwo file" code path, like we do for other kinds of units.

 3. Another option would be to save the DWARF version in
 dwarf2_loclist_baton (those are the users of the version in loc.c I
 referred to above).  That would duplicate the value, versus having it
 in dwarf2_per_cu_data alone.  But it would make it so we never have a
 possibly uninitialized version field, which is easier to reason about.

When first looking at the problem, I independently arrived at solution
#2.  It means we can still have a gap of time between the creation of a
dwarf2_per_cu_data object and when we read its header where the DWARF
version is unknown (and the version field is unset), but it seems
inevitable.  We just need to ensure that in all paths where we read its
header, we set the field.

If prefer the peace of mind of knowing the version field can never be
"unset", then solution #3 is good I guess.

Simon