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

[Simon Marchi <simark@simark.ca>]

On 3/3/25 3:44 PM, Simon Marchi wrote:
> 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.

Actually, dwarf2_loclist_baton has some wasted space (padding) at the
end, so now I vote for option #3 for simplicity - not thread safety
issue, no field possible left unset.  Patch here:

https://inbox.sourceware.org/gdb-patches/20250303212126.728156-1-simon.marchi@efficios.com/T/#u

Simon