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

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

[Tom de Vries]

When running test-case gdb.ada/access_tagged_param.exp with target board
fission, we run into:
...
(gdb) break pck.adb:19^M
gdb/dwarf2/read.h:289: internal-error: version: \
  Assertion `m_dwarf_version != 0' failed.^M
...

The assertion happens when calling cu->per_cu->version () in this workaround
in read_subroutine_type:
...
  /* PR gas/29517 occurs in 2.39, and is fixed in 2.40, but it's only fixed
     for dwarf version >= 3 which supports DW_TAG_unspecified_type.  */
  if (type->code () == TYPE_CODE_VOID
      && !type->is_stub ()
      && die->child == nullptr
      && (cu->per_cu->version () == 2
	  || producer_is_gas_2_39 (cu)))
    {
...
for a TU in a dwo file.

Fix this by using cu->header.version instead.

Tested on aarch64-linux.

PR symtab/32309
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32309
---
 gdb/dwarf2/read.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4503977d62b..824604261f5 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -14019,7 +14019,7 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
   if (type->code () == TYPE_CODE_VOID
       && !type->is_stub ()
       && die->child == nullptr
-      && (cu->per_cu->version () == 2
+      && (cu->header.version == 2
 	  || producer_is_gas_2_39 (cu)))
     {
       /* Work around PR gas/29517, pretend we have an DW_TAG_unspecified_type

base-commit: c99345db064e1bd645bc08db65174bae7ac20b0e
-- 
2.43.0

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

[Tom Tromey]

>>>>> "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.  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

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

[Tom de Vries]

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

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

[Simon Marchi]

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.

Simon

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

[Simon Marchi]

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

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

[Simon Marchi]

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