[PATCH 0/6] Type unit + split DWARF fixes (PR 33307)

[PATCH 0/6] Type unit + split DWARF fixes (PR 33307)

[Simon Marchi]

This series fixes a regression that I introduced a few months ago,
causing a crash in very specific circumstances: type unit in a .dwo file
+ stub in the main file + gdb index.  Luckily, this was caught by test
gdb.dwarf2/fission-reread.exp when ran with the cc-with-gdb-index board.

Patches 1-3 are improvements to the testsuite DWARF assembler, allowing
to generate better type units and split DWARF tests.

Patch 4 fixes the reported crash.

Patch 5 fixes another case of the same crash, for which I provide a test
/ reproducer.

Patch 6 is a trivial cleanup.

Simon Marchi (6):
  gdb/testsuite/dwarf: use single abbrev table in .dwo files
  gdb/testsuite/dwarf: convert _section proc to use parse_options
  gdb/testsuite/dwarf: emit type unit sections as COMDAT
  gdb/dwarf: when in dwarf2_cu, read addr_size from dwarf2_cu::header
  gdb/dwarf: store addr/offset/ref_addr sizes in dwarf2_per_cu
  gdb/dwarf: use dwarf2_per_cu::ref_addr_size in one spot

 gdb/dwarf2/cu.c                               |   7 +-
 gdb/dwarf2/read.c                             |  71 ++------
 gdb/dwarf2/read.h                             |  56 ++++--
 .../gdb.dwarf2/fission-type-unit-locexpr.c    |  25 +++
 .../gdb.dwarf2/fission-type-unit-locexpr.exp  | 161 ++++++++++++++++++
 .../gdb.dwarf2/fission-with-type-unit.exp     |  29 ++--
 gdb/testsuite/lib/dwarf.exp                   | 132 +++++++++++---
 7 files changed, 362 insertions(+), 119 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp


base-commit: 767c92a13e92d3c34b8ed6f3b6f1bf17eca57cbd
-- 
2.51.2

[PATCH 1/6] gdb/testsuite/dwarf: use single abbrev table in .dwo files

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

When I wrote test gdb.dwarf2/fission-with-type-unit.exp, I did not use
build_executable_and_dwo_files, because it wouldn't work to have
multiple units in the .dwo file, each referring to their own abbrev
table using labels.  build_executable_and_dwo_files extracts the .dwo
file content from the .o using objcopy (just like gcc does, I learned),
meaning that the .dwo file never runs through a linker.  Anything
needing relocation (like labels pointing to abbrev tables) doesn't work.

I instead opted to use gdb_compile_shlib to build the .dwo file on its
own, so that those labels would get resolved.  That causes problems now
that I'm trying to write a test with multiple type units in a .dwo file,
where each type unit should be in its own .debug_types section.  Running
the .dwo file through the linker causes all the .debug_types section to
be collapsed into one.  And generally, I think it was a bad idea to
generate a .dwo file using the linker, since the idea behind .dwo files
is that they do not need to be linked (therefore improving link
times).  We want to produce files as close to what an actual compiler
would produce.

This patch fixes this by doing what compilers do in the same situation:
use a single abbrev table shared by all units in the .dwo file.  This
requires the following changes in lib/dwarf.exp:

 - Declare a new variable _dwo_abbrev_num, which holds the next
   abbrev number to use in the .dwo file's abbrev section
   (.debug_abbrev.dwo).  Initialize this variable to 1.
 - When producing a CU or TU in a .dwo file, use 0 as the abbrev table
   offset.
 - When generating a DIE, return $_dwo_abbrev_num or $_abbrev_num,
   depending on whether the current CU is in a .dwo file.
 - After producing a CU or TU in a .dwo file, don't append the
   terminator byte.
 - After finishing producing the CUs and TUs, append the terminator byte
   in .debug_abbrev.dwo if we did output anything there.

Update gdb.dwarf2/fission-with-type-units.exp to use
build_executable_and_dwo_files, as it should.

Change-Id: Iabbcf00db97faf2a4fa5fc71652ad273081189f9
---
 .../gdb.dwarf2/fission-with-type-unit.exp     | 29 +++-----
 gdb/testsuite/lib/dwarf.exp                   | 69 +++++++++++++++----
 2 files changed, 67 insertions(+), 31 deletions(-)

diff --git a/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp b/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp
index 58cda296c67b..5c73001ec66b 100644
--- a/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp
+++ b/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp
@@ -21,25 +21,22 @@ load_lib dwarf.exp
 # This test can only be run on targets which support DWARF-2 and use gas.
 require dwarf2_support
 
-standard_testfile .c -dw.S -dwo.S
+standard_testfile .c -dw.S
 
-set main_asm_file [standard_output_file $srcfile2]
-set dwo_asm_file [standard_output_file $srcfile3]
+set asm_file [standard_output_file $srcfile2]
 
-# Debug info in the main file.
-Dwarf::assemble $main_asm_file {
+Dwarf::assemble $asm_file {
+    # In the main file.
     cu {
 	version 5
 	dwo_id 0xF00D
     } {
 	compile_unit {
-	    DW_AT_dwo_name ${::gdb_test_file_name}.dwo DW_FORM_strp
+	    DW_AT_dwo_name ${::gdb_test_file_name}-dw.dwo DW_FORM_strp
 	} {}
     }
-}
 
-# Debug info in the DWO file.
-Dwarf::assemble $dwo_asm_file {
+    # In the .dwo file.
     tu {
 	fission 1
 	version 5
@@ -79,15 +76,11 @@ Dwarf::assemble $dwo_asm_file {
     }
 }
 
-# Build main file.
-if { [build_executable "${testfile}.exp" $binfile \
-	[list ${srcfile} ${main_asm_file}] {nodebug}] } {
-    return
-}
-
-# Build DWO file.
-set dwo_file [standard_output_file ${testfile}.dwo]
-if { [gdb_compile_shlib $dwo_asm_file $dwo_file nodebug] != "" } {
+set obj [standard_output_file "${testfile}-dw.o"]
+set dwo_file [standard_output_file "${testfile}.dwo"]
+if {[build_executable_and_dwo_files "$testfile.exp" "${binfile}" {} \
+	 [list $asm_file {nodebug split-dwo} $obj] \
+	 [list $srcfile  {nodebug}]]} {
     return
 }
 
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index ba8197d0bb8e..919e2e05687f 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -563,6 +563,9 @@ namespace eval Dwarf {
     # table.
     variable _abbrev_num
 
+    # The next available abbrev number in the (single) DWO abbrev table.
+    variable _dwo_abbrev_num
+
     # The string table for this assembly.  The key is the string; the
     # value is the label for that string.
     variable _strings
@@ -1008,8 +1011,17 @@ namespace eval Dwarf {
     # table.
     proc _get_abbrev_num {} {
 	variable _abbrev_num
-	set res $_abbrev_num
-	incr _abbrev_num
+	variable _dwo_abbrev_num
+	variable _cu_is_fission
+
+	if { $_cu_is_fission } {
+	    set res $_dwo_abbrev_num
+	    incr _dwo_abbrev_num
+	} else {
+	    set res $_abbrev_num
+	    incr _abbrev_num
+	}
+
 	return $res
     }
 
@@ -1538,8 +1550,17 @@ namespace eval Dwarf {
 	_section $section
 
 	set cu_num [incr _cu_count]
-	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
-	set _abbrev_num 1
+
+	# Since .dwo files are not linked, we can't use a label to point to a
+	# specific place in the .debug_abbrev section.  For .dwo files, we
+	# therefore use a single abbrev table (at offset 0) shared by all units
+	# in that file.
+	if { $_cu_is_fission } {
+	    set my_abbrevs 0
+	} else {
+	    set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
+	    set _abbrev_num 1
+	}
 
 	set _cu_label [_compute_label "cu${cu_num}_begin"]
 	set start_label [_compute_label "cu${cu_num}_start"]
@@ -1606,9 +1627,11 @@ namespace eval Dwarf {
 
 	uplevel $_level $body
 
-	_defer_output $_abbrev_section {
-	    # Emit the terminator.
-	    _op .byte 0x0 "Abbrev end - Terminator"
+	if { !$_cu_is_fission } {
+	    _defer_output $_abbrev_section {
+		# Emit the terminator.
+		_op .byte 0x0 "Abbrev end - Terminator"
+	    }
 	}
 
 	define_label $end_label
@@ -1681,8 +1704,17 @@ namespace eval Dwarf {
 	_section $section
 
 	set cu_num [incr _cu_count]
-	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
-	set _abbrev_num 1
+
+	# Since .dwo files are not linked, we can't use a label to point to a
+	# specific place in the .debug_abbrev section.  For .dwo files, we
+	# therefore use a single abbrev table (at offset 0) shared by all units
+	# in that file.
+	if { $_cu_is_fission } {
+		set my_abbrevs 0
+	} else {
+		set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
+		set _abbrev_num 1
+	}
 
 	set _cu_label [_compute_label "cu${cu_num}_begin"]
 	set start_label [_compute_label "cu${cu_num}_start"]
@@ -1736,9 +1768,11 @@ namespace eval Dwarf {
 
 	uplevel $_level $body
 
-	_defer_output $_abbrev_section {
-	    # Emit the terminator.
-	    _op .byte 0x0 "Abbrev end - Terminator"
+	if { !$_cu_is_fission } {
+	    _defer_output $_abbrev_section {
+		# Emit the terminator.
+		_op .byte 0x0 "Abbrev end - Terminator"
+	    }
 	}
 
 	define_label $end_label
@@ -3507,6 +3541,7 @@ namespace eval Dwarf {
 	variable _debug_ranges_64_bit
 	variable _debug_addr_index
 	variable _level
+	variable _dwo_abbrev_num
 
 	if { [llength $options] == 1 } {
 	    set options [list filename [lindex $options 0]]
@@ -3539,8 +3574,8 @@ namespace eval Dwarf {
 
 	set _line_count 0
 	set _debug_ranges_64_bit [is_64_target]
-
 	set _debug_addr_index 0
+	set _dwo_abbrev_num 1
 
 	# Dummy CU at the start to ensure that the first CU in $body is not
 	# the first in .debug_info.
@@ -3562,6 +3597,14 @@ namespace eval Dwarf {
 	    dummy_cu
 	}
 
+	# If we wrote any abbrev in .debug_abbrev.dwo, write the terminator.
+	if { $_dwo_abbrev_num > 1 } {
+	    _defer_output .debug_abbrev.dwo {
+		# Emit the terminator.
+		_op .byte 0x0 "Abbrev end - Terminator"
+	    }
+	}
+
 	_write_deferred_output
 
 	_section .note.GNU-stack "" progbits
-- 
2.51.2

[PATCH 2/6] gdb/testsuite/dwarf: convert _section proc to use parse_options

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

The following patch will add more options to the _section proc, so
convert it to use options, to be more user-friendly.

Change-Id: I971e4e10e55d63af2a5e29dc5e1d00f368b3ecaa
---
 gdb/testsuite/lib/dwarf.exp | 45 +++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 919e2e05687f..5536b7656b8c 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -1071,17 +1071,33 @@ namespace eval Dwarf {
 	}
     }
 
-    proc _section {name {flags ""} {type ""}} {
-	if {$name == ".debug_str"} {
-	    # Hard-code this because it's always desirable.
-	    _emit "        .section $name, \"MS\", %progbits, 1"
-	} elseif {$flags == "" && $type == ""} {
-	    _emit "        .section $name"
-	} elseif {$type == ""} {
-	    _emit "        .section $name, \"$flags\""
-	} else {
-	    _emit "        .section $name, \"$flags\", %$type"
+    proc _section {name {options {}}} {
+	parse_options {
+	    { flags "" }
+	    { type "" }
 	}
+
+	set directive ".section $name"
+
+	if { $name == ".debug_str" } {
+	    # Hard-code this because it's always desirable.
+	    append directive ", \"MS\", %progbits, 1"
+	}
+
+	if { $flags != "" } {
+	    append directive ",\"$flags\""
+	}
+
+	if { $type != "" } {
+	    # Can't specify a type without flags.
+ 	    if { $flags == "" } {
+		append directive ", \"\""
+	    }
+
+	    append directive ", %$type"
+	}
+
+	_emit "        $directive"
     }
 
     # SECTION_SPEC is a list of arguments to _section.
@@ -3173,7 +3189,10 @@ namespace eval Dwarf {
 
     # Emit a note section holding the given build-id.
     proc build_id {buildid} {
-	_defer_output {.note.gnu.build-id a note} {
+	_defer_output {.note.gnu.build-id {
+			   flags a
+			   type note
+		       }} {
 	    # From elf/common.h.
 	    set NT_GNU_BUILD_ID 3
 
@@ -3607,7 +3626,9 @@ namespace eval Dwarf {
 
 	_write_deferred_output
 
-	_section .note.GNU-stack "" progbits
+	_section .note.GNU-stack {
+	    type progbits
+	}
 
 	if { $file_id == "" } {
 	    catch {close $_output_file}
-- 
2.51.2

[PATCH 3/6] gdb/testsuite/dwarf: emit type unit sections as COMDAT

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

In an effort to generate ELF files and DWARF info as close as possible
as an actual compiler would generate, change how we emit type unit
sections to emit each type unit in its own section, in COMDAT section
groups.

We currently emit all type units in a single, standard section (either
.debug_info or .debug_types, depending on the DWARF version).  Compilers
emit each type unit in its own .debug_types section.  Each section is
placed in a COMDAT section group with a signature based on the type
unit's signature.  This lets the linker deduplicate identical type units
by discarding section groups with identical signatures (keeping only one
group with a given signature).

Looking at a .s file generated by gcc, we can see that this is how it
specifies the section for a type unit:

    .section .debug_info,"G",@progbits,wi.006fd2152a3054a6,comdat

The "G" flag tells the assembler to place the section in a section
group with signature "wi.006fd2152a3054a6".  That string was generated
from the type unit, signature.  Finally, the comdat linkage field
indicates that the section group should have the COMDAT flag.

Update the tu proc to emit a section with these properties.  In this
case, it's mandatory to specify the type of the section (progbits).

Update the _section proc to accept the new options "group_signature" and
"linkage".

As a result, if you look at the .debug_types section in a .o file from
gcc:

    $ readelf -g main.o
    COMDAT group section [    1] `.group' [wt.006fd2152a3054a6] contains 2 sections:
       [Index]    Name
       [   11]   .debug_types
       [   12]   .rela.debug_types

    COMDAT group section [    2] `.group' [wt.c621aa8e3c23e450] contains 2 sections:
       [Index]    Name
       [   13]   .debug_types
       [   14]   .rela.debug_types

And a .o file created by our DWARF assembler:

    $ readelf -g testsuite/outputs/gdb.dwarf2/struct-with-sig/struct-with-sig1.o
    COMDAT group section [    1] `.group' [sig.0x0000000000000001] contains 2 sections:
       [Index]    Name
       [   10]   .debug_types
       [   11]   .rela.debug_types

    COMDAT group section [    2] `.group' [sig.0x0000000000000002] contains 2 sections:
       [Index]    Name
       [   12]   .debug_types
       [   13]   .rela.debug_types

In both cases, in the fully linked files, there is a single .debug_types section
containing the two type units, as expected.

Before this patch, when I run gdb.dwarf2/fission-with-type-unit.exp, the
resulting .dwo file has a single .debug_info.dwo.  After this patch it
has two: one with the type unit and one with the compile unit (the test
uses DWARF 5).  Based on what I see gcc generate when using "-gdwarf-5
-gsplit-dwarf -fdebug-types-section", this is what we expect.

Change-Id: Ie1954dc697fe100b5dbe664d148c71fa02888d96
---
 gdb/testsuite/lib/dwarf.exp | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 5536b7656b8c..c3c35d43607d 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -1075,6 +1075,8 @@ namespace eval Dwarf {
 	parse_options {
 	    { flags "" }
 	    { type "" }
+	    { group_signature "" }
+	    { linkage "" }
 	}
 
 	set directive ".section $name"
@@ -1097,6 +1099,14 @@ namespace eval Dwarf {
 	    append directive ", %$type"
 	}
 
+	if { $group_signature != "" } {
+	    append directive ", $group_signature"
+	}
+
+	if { $linkage != "" } {
+	    append directive ", $linkage"
+	}
+
 	_emit "        $directive"
     }
 
@@ -1717,7 +1727,13 @@ namespace eval Dwarf {
 	    set _abbrev_section "$_abbrev_section.dwo"
 	}
 
-	_section $section
+	# The specific format of the signature is arbitrary.
+	_section $section {
+	    flags G
+	    type progbits
+	    group_signature sig.$signature
+	    linkage comdat
+	}
 
 	set cu_num [incr _cu_count]
 
-- 
2.51.2

[PATCH 4/6] gdb/dwarf: when in dwarf2_cu, read addr_size from dwarf2_cu::header

[Simon Marchi]

This patch fixes a crash caused by GDB trying to read from a section not
read in.  The bug happens in those specific circumstances:

 - reading a type unit from .dwo
 - that type unit has a stub in the main file
 - there is a GDB index (.gdb_index) present

This crash is the cause of the following test failure:

    $ Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/fission-reread.exp ...
    ERROR: GDB process no longer exists

Or, manually:

    $ ./gdb -nx -q --data-directory=data-directory /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/fission-reread/fission-reread -ex "p 1"
    Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/fission-reread/fission-reread...

    Fatal signal: Segmentation fault

For this last one, you need to interrupt the test (e.g. add a return)
before the test deletes the .dwo file.

The backtrace at the moment of the crash is:

    #0  0x0000555566968f7f in bfd_getl32 (p=0x0) at /home/simark/src/binutils-gdb/bfd/libbfd.c:846
    #1  0x00005555642e561d in read_initial_length (abfd=0x7d1ff1eb0e40, buf=0x0, bytes_read=0x7bfff0962fa0, handle_nonstd=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/leb.c:92
    #2  0x00005555647ca9ea in read_unit_head (header=0x7d0ff1e068b0, info_ptr=0x0, section=0x7c3ff1dea7d0, section_kind=ruh_kind::COMPILE) at /home/simark/src/binutils-gdb/gdb/dwarf2/unit-head.c:44
    #3  0x000055556452e37e in dwarf2_per_cu::get_header (this=0x7d0ff1e06880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18531
    #4  0x000055556452e574 in dwarf2_per_cu::addr_size (this=0x7d0ff1e06880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18544
    #5  0x000055556406af91 in dwarf2_cu::addr_type (this=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/cu.c:124
    #6  0x0000555564534e48 in set_die_type (die=0x7e0ff1f23dd0, type=0x7e0ff1f027f0, cu=0x7d7ff1e20880, skip_data_location=false) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:19020
    #7  0x00005555644dcc7b in read_structure_type (die=0x7e0ff1f23dd0, cu=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:11239
    #8  0x000055556451c834 in read_type_die_1 (die=0x7e0ff1f23dd0, cu=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16878
    #9  0x000055556451c5e0 in read_type_die (die=0x7e0ff1f23dd0, cu=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16861
    #10 0x0000555564526f3a in get_signatured_type (die=0x7e0ff1f0ffb0, signature=10386129560629316377, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:17998
    #11 0x000055556451c23b in lookup_die_type (die=0x7e0ff1f0ffb0, attr=0x7e0ff1f10008, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16808
    #12 0x000055556451b2e9 in die_type (die=0x7e0ff1f0ffb0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16684
    #13 0x000055556451457f in new_symbol (die=0x7e0ff1f0ffb0, type=0x0, cu=0x7d7ff1e0f480, space=0x0) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16089
    #14 0x00005555644c52a4 in read_variable (die=0x7e0ff1f0ffb0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:9119
    #15 0x0000555564494072 in process_die (die=0x7e0ff1f0ffb0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:5197
    #16 0x000055556449c88e in read_file_scope (die=0x7e0ff1f0fdd0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:6125
    #17 0x0000555564493671 in process_die (die=0x7e0ff1f0fdd0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:5098
    #18 0x00005555644912f5 in process_full_comp_unit (cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:4851
    #19 0x0000555564485e18 in process_queue (per_objfile=0x7d6ff1e71100) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:4161
    #20 0x000055556446391d in dw2_do_instantiate_symtab (per_cu=0x7ceff1de42d0, per_objfile=0x7d6ff1e71100, skip_partial=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:1650
    #21 0x0000555564463b3c in dw2_instantiate_symtab (per_cu=0x7ceff1de42d0, per_objfile=0x7d6ff1e71100, skip_partial=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:1671
    #22 0x00005555644687fd in dwarf2_base_index_functions::expand_all_symtabs (this=0x7c1ff1e04990, objfile=0x7d5ff1e46080) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:1990
    #23 0x0000555564381050 in cooked_index_functions::expand_all_symtabs (this=0x7c1ff1e04990, objfile=0x7d5ff1e46080) at /home/simark/src/binutils-gdb/gdb/dwarf2/cooked-index.h:237
    #24 0x0000555565df5b0d in objfile::expand_all_symtabs (this=0x7d5ff1e46080) at /home/simark/src/binutils-gdb/gdb/symfile-debug.c:372
    #25 0x0000555565eafc4a in maintenance_expand_symtabs (args=0x0, from_tty=1) at /home/simark/src/binutils-gdb/gdb/symmisc.c:914

The main file contains a stub (skeleton) for a compilation unit and a
stub for a type unit.   The .dwo file contains a compilation unit and a
type unit matching those stubs.  When doing the initial scan of the main
file, the DWARF reader parses the CU/TU list from the GDB index
(.gdb_index), and thus creates a signatured_type object based on that.
The section field of this signatured_type points to the .debug_types
section in the main file, the one containing the stub.  And because GDB
trusts the GDB index, it never needs to look at that .debug_types
section in the main file.  That section remains not read in.

When expanding the compilation unit, GDB encounters a type unit
reference (by signature) corresponding to the type in the type unit.  We
get in lookup_dwo_signatured_type, trying to see if there is a type unit
matching that signature in the current .dwo file.  We proceed to read
and expand that type unit, until we eventually get to a
dwarf2_cu::addr_type() call, which doess:

     int addr_size = this->per_cu->addr_size ();

dwarf2_per_cu::addr_size() tries to read the header from the section
pointed to by dwarf2_per_cu::section which, if you recall, is the
.debug_types section in the main file that was never read in.  That
causes the segfault.

All this was working fine before these patches of mine, that tried to do
some cleanups:

    a47e2297fc28 ("gdb/dwarf: pass section offset to dwarf2_per_cu_data constructor")
    c44ab627b021 ("gdb/dwarf: pass section to dwarf2_per_cu_data constructor")
    39ee8c928551 ("gdb/dwarf: pass unit length to dwarf2_per_cu_data constructor")

Before these patches, the fill_in_sig_entry_from_dwo_entry function
(called from lookup_dwo_signatured_type, among others) would overwrite
some dwarf2_per_cu fields (including the section) to point to the .dwo,
rather than represent what's in the main file.  Therefore, the header
would have been read from the unit in the .dwo file, and things would
have been fine.

When doing these changes, I mistakenly assumed that the section written
by fill_in_sig_entry_from_dwo_entry was the same as the section already
there, which is why I removed the statements overwriting the section
field (and the two others).  To my defense, here's the comment on
dwarf2_per_cu::section:

    /* The section this CU/TU lives in.
       If the DIE refers to a DWO file, this is always the original die,
       not the DWO file.  */
    struct dwarf2_section_info *section = nullptr;

I would prefer to not reintroduce the behavior of overwriting the
section info in dwarf2_per_cu, because:

 1. I find it confusing, I like the invariant of dwarf2_per_cu::section
    points to the stub, and dwarf2_cu::section points to where we
    actually read the debug info from.
 2. The dwarf2_per_bfd::all_units vector is nowadays sorted by (section,
    section offset).  If we change the section and section offset of a
    dwarf2_per_cu, then we can no longer do binary searches in it, we
    would have to re-sort the vector (not a big deal, but still adds to
    the confusion).

One possible fix would be to make sure that the section is read in when
reading the header, probably in dwarf2_per_cu::get_header.  An approach
like that was proposed by Andrew initially, here:

  https://inbox.sourceware.org/gdb-patches/60ba2b019930fd6164f8e6ab6cb2e396c32c6ac2.1759486109.git.aburgess@redhat.com/

It would work, but there is a more straightforward fix for this
particular problem, I believe.  In dwarf2_cu, we have access to the
header read from the unit we're actually reading the DWARF from.  In the
DWO case, that is the header read from the .dwo file.  We can get the
address size from there instead of going through the dwarf2_per_cu
object.  This is what this patch does.

However, there are other case where we get the address (or offset) size
from the dwarf2_per_cu in the DWARF expression evaluator (expr.c,
loc.c), that could cause a similar crash. The next patch handles these
cases.

Finally, just to put things in context, having a stub in the main file
for a type unit is obsolete.  It happened in the gcc 4.x days, until
this commit:

    commit 4dd7c3b285daf030da0ff9c0d5e2f79b24943d1e
    Author: Cary Coutant <ccoutant@google.com>
    Date:   Fri Aug 8 20:33:26 2014 +0000

        Remove skeleton type units that were being produced with -gsplit-dwarf.

In DWARF 5, split type units don't have stubs, only split compilations
units do.

Change-Id: Icc5014276c75bf3126ccb43a4424e96ca1a51f06
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33307
---
 gdb/dwarf2/cu.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index 86b77f5951ef..15a1b0243f8c 100644
--- a/gdb/dwarf2/cu.c
+++ b/gdb/dwarf2/cu.c
@@ -59,8 +59,8 @@ dwarf2_cu::dwarf2_cu (dwarf2_per_cu *per_cu, dwarf2_per_objfile *per_objfile)
 struct type *
 dwarf2_cu::addr_sized_int_type (bool unsigned_p) const
 {
-  int addr_size = this->per_cu->addr_size ();
-  return objfile_int_type (this->per_objfile->objfile, addr_size, unsigned_p);
+  return objfile_int_type (this->per_objfile->objfile,
+			   this->header.addr_size, unsigned_p);
 }
 
 /* Start a symtab for DWARF.  NAME, COMP_DIR, LOW_PC are passed to the
@@ -121,9 +121,8 @@ dwarf2_cu::addr_type () const
   struct objfile *objfile = this->per_objfile->objfile;
   struct type *void_type = builtin_type (objfile)->builtin_void;
   struct type *addr_type = lookup_pointer_type (void_type);
-  int addr_size = this->per_cu->addr_size ();
 
-  if (addr_type->length () == addr_size)
+  if (addr_type->length () == this->header.addr_size)
     return addr_type;
 
   addr_type = addr_sized_int_type (addr_type->is_unsigned ());
-- 
2.51.2

[PATCH 5/6] gdb/dwarf: store addr/offset/ref_addr sizes in dwarf2_per_cu

[Simon Marchi]

In some specific circumstances, it is possible for GDB to read a type
unit from a .dwo file without ever reading in the section of the stub in
the main file.  In that case, calling any of these methods:

  - dwarf2_per_cu::addr_size()
  - dwarf2_per_cu::offset_size()
  - dwarf2_per_cu::ref_addr_size()

will cause a crash, because they will try to read the unit header from
the not-read-in section buffer.  See the previous patch for more
details.

The remaining calls to these methods are in the loc.c and expr.c
files.  That is, in the location and expression machinery.  It is
possible to set things up to cause them to trigger a crash, as shown by
the new test, when running it with the cc-with-gdb-index board:

    $ make check TESTS="gdb.dwarf2/fission-type-unit-locexpr.exp" RUNTESTFLAGS="--target_board=cc-with-gdb-index"
    Running /home/simark/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp ...
    ERROR: GDB process no longer exists

The backtrace at the moment of the crash is:

    #0  0x0000555566968b1f in bfd_getl32 (p=0x78) at /home/simark/src/binutils-gdb/bfd/libbfd.c:846
    #1  0x00005555642e51b7 in read_initial_length (abfd=0x7d1ff1eb0e40, buf=0x78 <error: Cannot access memory at address 0x78>, bytes_read=0x7bfff09daca0, handle_nonstd=true)
        at /home/simark/src/binutils-gdb/gdb/dwarf2/leb.c:92
    #2  0x00005555647ca584 in read_unit_head (header=0x7d0ff1e06c70, info_ptr=0x78 <error: Cannot access memory at address 0x78>, section=0x7c3ff1dea7d0, section_kind=ruh_kind::COMPILE)
        at /home/simark/src/binutils-gdb/gdb/dwarf2/unit-head.c:44
    #3  0x000055556452df18 in dwarf2_per_cu::get_header (this=0x7d0ff1e06c40) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18531
    #4  0x000055556452e10e in dwarf2_per_cu::addr_size (this=0x7d0ff1e06c40) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18544
    #5  0x0000555564314ac3 in dwarf2_locexpr_baton_eval (dlbaton=0x7bfff0c9a508, frame=..., addr_stack=0x7bfff0b59150, valp=0x7bfff0c9a430, push_values=..., is_reference=0x7bfff0d33030)
        at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1593
    #6  0x0000555564315bd2 in dwarf2_evaluate_property (prop=0x7bfff0c9a450, initial_frame=..., addr_stack=0x7bfff0b59150, value=0x7bfff0c9a430, push_values=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1668
    #7  0x0000555564a14ee1 in resolve_dynamic_field (field=..., addr_stack=0x7bfff0b59150, frame=...) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:2758
    #8  0x0000555564a15e24 in resolve_dynamic_struct (type=0x7e0ff1f02550, addr_stack=0x7bfff0b59150, frame=...) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:2839
    #9  0x0000555564a17061 in resolve_dynamic_type_internal (type=0x7e0ff1f02550, addr_stack=0x7bfff0b59150, frame=..., top_level=true) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:2972
    #10 0x0000555564a17899 in resolve_dynamic_type (type=0x7e0ff1f02550, valaddr=..., addr=0x4010, in_frame=0x7bfff0d32e60) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:3019
    #11 0x000055556675fb34 in value_from_contents_and_address (type=0x7e0ff1f02550, valaddr=0x0, address=0x4010, frame=...) at /home/simark/src/binutils-gdb/gdb/value.c:3674
    #12 0x00005555666ce911 in get_value_at (type=0x7e0ff1f02550, addr=0x4010, frame=..., lazy=1) at /home/simark/src/binutils-gdb/gdb/valops.c:992
    #13 0x00005555666ceb89 in value_at_lazy (type=0x7e0ff1f02550, addr=0x4010, frame=...) at /home/simark/src/binutils-gdb/gdb/valops.c:1039
    #14 0x000055556491909f in language_defn::read_var_value (this=0x5555725fce40 <minimal_language_defn>, var=0x7e0ff1f02500, var_block=0x7e0ff1f025d0, frame_param=...)
        at /home/simark/src/binutils-gdb/gdb/findvar.c:504
    #15 0x000055556491961b in read_var_value (var=0x7e0ff1f02500, var_block=0x7e0ff1f025d0, frame=...) at /home/simark/src/binutils-gdb/gdb/findvar.c:518
    #16 0x00005555666d1861 in value_of_variable (var=0x7e0ff1f02500, b=0x7e0ff1f025d0) at /home/simark/src/binutils-gdb/gdb/valops.c:1384
    #17 0x00005555647f7099 in evaluate_var_value (noside=EVAL_NORMAL, blk=0x7e0ff1f025d0, var=0x7e0ff1f02500) at /home/simark/src/binutils-gdb/gdb/eval.c:533
    #18 0x00005555647f740c in expr::var_value_operation::evaluate (this=0x7c2ff1e3b690, expect_type=0x0, exp=0x7c2ff1e3aa00, noside=EVAL_NORMAL) at /home/simark/src/binutils-gdb/gdb/eval.c:559
    #19 0x00005555647f3347 in expression::evaluate (this=0x7c2ff1e3aa00, expect_type=0x0, noside=EVAL_NORMAL) at /home/simark/src/binutils-gdb/gdb/eval.c:109
    #20 0x000055556543ac2f in process_print_command_args (args=0x7fffffffe728 "global_var", print_opts=0x7bfff0be4a30, voidprint=true) at /home/simark/src/binutils-gdb/gdb/printcmd.c:1328
    #21 0x000055556543ae65 in print_command_1 (args=0x7fffffffe728 "global_var", voidprint=1) at /home/simark/src/binutils-gdb/gdb/printcmd.c:1341
    #22 0x000055556543b707 in print_command (exp=0x7fffffffe728 "global_var", from_tty=1) at /home/simark/src/binutils-gdb/gdb/printcmd.c:1408

The problem to solve is: in order to evaluate a location expression, we
need to know some information (the various sizes) found in the unit
header.  In that context, it's not possible to get it from
dwarf2_cu::header, like the previous patch did: at the time the
expression is evaluated, the corresponding dwarf2_cu might have been
freed.  We don't want to re-build a dwarf2_cu just for that, it would be
very inefficient.  We could force reading in the dwarf2_per_cu::section
section (in the main file), but we never needed to read that section
before, so it would be better to avoid reading it unnecessarily.

My initial attempt was to store this information in baton objects
(dwarf2_locexpr_baton & co), so that it can be retrieved when the time
comes to evaluate the expressions.  However, it quickly became obvious
that storing it there would be redundant and wasteful.

I instead opted to store this information directly inside dwarf2_per_cu,
making it easily available when evaluating expressions.  These fields
are initially have the value 0, and are set by cutu_reader whenever the
unit is parsed.  The various getters (dwarf2_per_cu::addr_size & al) now
just return these fields.

Doing so allows removing anything related to reading the header from
dwarf2_per_cu, which I think is a nice simplification.  This means that
nothing ever needs to read the header from just a dwarf2_per_cu.

It also happens to shrink the dwarf2_per_cu object size a bit, going
from:

    (top-gdb) p sizeof(dwarf2_per_cu)
    $1 = 176

to

    (top-gdb) p sizeof(dwarf2_per_cu)
    $1 = 120

I placed the new fields at this strange location in dwarf2_per_cu
because there happened to be a nice 3 bytes hole there (on Linux amd64
at least).

The new test set things up as described previously.  Note that the crash
only occurs if using the cc-with-gdb-index board.

Change-Id: I50807a1bbb605f0f92606a9e61c026e3376a4fcf
---
 gdb/dwarf2/read.c                             |  64 ++-----
 gdb/dwarf2/read.h                             |  56 ++++--
 .../gdb.dwarf2/fission-type-unit-locexpr.c    |  25 +++
 .../gdb.dwarf2/fission-type-unit-locexpr.exp  | 161 ++++++++++++++++++
 4 files changed, 241 insertions(+), 65 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index df6e784025dc..a8b9798c4e8e 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -2832,6 +2832,14 @@ cutu_reader::read_cutu_die_from_dwo (dwarf2_cu *cu, dwo_unit *dwo_unit,
       dwo_unit->length = cu->header.get_length_with_initial ();
     }
 
+  /* Record some information found in the header.  This will be needed when
+     evaluating DWARF expressions in the context of this unit, for instance.  */
+  per_cu->set_addr_size (cu->header.addr_size);
+  per_cu->set_offset_size (cu->header.offset_size);
+  per_cu->set_ref_addr_size (cu->header.version == 2
+			     ? cu->header.addr_size
+			     : cu->header.offset_size);
+
   dwo_abbrev_section->read (objfile);
   m_dwo_abbrev_table
     = abbrev_table::read (dwo_abbrev_section, cu->header.abbrev_sect_off);
@@ -3075,6 +3083,15 @@ cutu_reader::cutu_reader (dwarf2_per_cu &this_cu,
 	  gdb_assert (this_cu.sect_off () == cu->header.sect_off);
 	  this_cu.set_length (cu->header.get_length_with_initial ());
 	}
+
+	/* Record some information found in the header.  This will be needed
+	   when evaluating DWARF expressions in the context of this unit, for
+	   instance.  */
+	this_cu.set_addr_size (cu->header.addr_size);
+	this_cu.set_offset_size (cu->header.offset_size);
+	this_cu.set_ref_addr_size (cu->header.version == 2
+				   ? cu->header.addr_size
+				   : cu->header.offset_size);
     }
 
   /* Skip dummy compilation units.  */
@@ -18520,53 +18537,6 @@ dwarf2_symbol_mark_computed (const struct attribute *attr, struct symbol *sym,
 
 /* See read.h.  */
 
-const unit_head *
-dwarf2_per_cu::get_header () const
-{
-  if (!m_header_read_in)
-    {
-      const gdb_byte *info_ptr
-	= this->section ()->buffer + to_underlying (this->sect_off ());
-
-      read_unit_head (&m_header, info_ptr, this->section (), ruh_kind::COMPILE);
-
-      m_header_read_in = true;
-    }
-
-  return &m_header;
-}
-
-/* See read.h.  */
-
-int
-dwarf2_per_cu::addr_size () const
-{
-  return this->get_header ()->addr_size;
-}
-
-/* See read.h.  */
-
-int
-dwarf2_per_cu::offset_size () const
-{
-  return this->get_header ()->offset_size;
-}
-
-/* See read.h.  */
-
-int
-dwarf2_per_cu::ref_addr_size () const
-{
-  const unit_head *header = this->get_header ();
-
-  if (header->version == 2)
-    return header->addr_size;
-  else
-    return header->offset_size;
-}
-
-/* See read.h.  */
-
 void
 dwarf2_per_cu::set_lang (enum language lang, dwarf_source_language dw_lang)
 {
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 95f00e617c7a..c09536e73412 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -126,7 +126,6 @@ struct dwarf2_per_cu
       tu_read (false),
       lto_artificial (false),
       queued (false),
-      m_header_read_in (false),
       files_read (false),
       scanned (false),
       m_section (section),
@@ -189,11 +188,6 @@ struct dwarf2_per_cu
      any of the current compilation units are processed.  */
   packed<bool, 1> queued;
 
-private:
-  /* True if HEADER has been read in.  */
-  mutable packed<bool, 1> m_header_read_in;
-
-public:
   /* True if we've tried to read the file table.  There will be no
      point in trying to read it again next time.  */
   packed<bool, 1> files_read;
@@ -219,6 +213,14 @@ struct dwarf2_per_cu
      not.  */
   std::atomic<bool> scanned;
 
+private:
+  /* Sizes for an address, an offset, and a section offset.  These fields are
+     set by cutu_reader when the unit is read.  */
+  unsigned char m_addr_size = 0;
+  unsigned char m_offset_size = 0;
+  unsigned char m_ref_addr_size = 0;
+
+public:
   /* Our index in the unshared "symtabs" vector.  */
   unsigned index = 0;
 
@@ -231,12 +233,6 @@ struct dwarf2_per_cu
   /* Backlink to the owner of this.  */
   dwarf2_per_bfd *m_per_bfd;
 
-  /* DWARF header of this unit.  Note that dwarf2_cu reads its own version of
-     the header, which may differ from this one, since it may pass
-     rch_kind::TYPE to read_unit_head, whereas for dwarf2_per_cu we always pass
-     ruh_kind::COMPILE.  */
-  mutable unit_head m_header;
-
 public:
   /* The file and directory for this CU.  This is cached so that we
      don't need to re-examine the DWO in some situations.  This may be
@@ -281,20 +277,44 @@ struct dwarf2_per_cu
   bool is_dwz () const
   { return m_is_dwz; }
 
-  /* Get the header of this per_cu, reading it if necessary.  */
-  const unit_head *get_header () const;
-
   /* Return the address size given in the compilation unit header for
      this CU.  */
-  int addr_size () const;
+  int addr_size () const
+  {
+    gdb_assert (m_addr_size != 0);
+    return m_addr_size;
+  }
+
+  /* Set the address size given in the compilation unit header for
+     this CU.  */
+  void set_addr_size (int addr_size)
+  { m_addr_size = addr_size; }
 
   /* Return the offset size given in the compilation unit header for
      this CU.  */
-  int offset_size () const;
+  int offset_size () const
+  {
+    gdb_assert (m_offset_size != 0);
+    return m_offset_size;
+  }
+
+  /* Set the offset size given in the compilation unit header for
+     this CU.  */
+  void set_offset_size (int offset_size)
+  {  m_offset_size = offset_size; }
 
   /* Return the DW_FORM_ref_addr size given in the compilation unit
      header for this CU.  */
-  int ref_addr_size () const;
+  int ref_addr_size () const
+  {
+    gdb_assert (m_ref_addr_size != 0);
+    return m_ref_addr_size;
+  }
+
+  /* Return the DW_FORM_ref_addr size given in the compilation unit
+     header for this CU.  */
+  void set_ref_addr_size (int ref_addr_size)
+  { m_ref_addr_size = ref_addr_size; }
 
   /* Return length of this CU.  */
   unsigned int length () const
diff --git a/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.c b/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.c
new file mode 100644
index 000000000000..1a84f5096334
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2025 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+unsigned char buf[] = { 0x11, 0x22, 0x22, 0x11, };
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp b/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp
new file mode 100644
index 000000000000..43bbe8f48436
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp
@@ -0,0 +1,161 @@
+# Copyright 2025 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This test is meant to reproduce a bug occurring in those specific
+# circumstances:
+#
+#  - Split DWARF (debug info in a .dwo file)
+#  - A type unit in the .dwo file, with a corresponding stub in the main file
+#  - A GDB index
+#  - We evaluate a DWARF expression in the context of that type unit
+#
+# The test doesn't create a GDB index, but that can be done by running it
+# with the cc-with-gdb-index board.
+
+load_lib dwarf.exp
+
+require dwarf2_support
+
+standard_testfile .c -dw.S
+
+set asm_file [standard_output_file $srcfile2]
+
+Dwarf::assemble $asm_file {
+    set outdir [standard_output_file ""]
+    set cu_debug_addr_label "invalid"
+
+    # Debug info in the DWO file.
+
+    # Definition of "int".
+    tu {
+	fission 1
+	version 4
+    } 0xCAFE1 the_type_1 {
+	type_unit {} {
+	    the_type_1: base_type {
+		DW_AT_byte_size 4 DW_FORM_sdata
+		DW_AT_encoding  @DW_ATE_signed
+		DW_AT_name      int
+	    }
+	}
+    }
+
+    # Definition of structure type "struct_type".
+    tu {
+	fission 1
+	version 4
+    } 0xCAFE2 the_type_2 {
+	type_unit {} {
+	    the_type_2: DW_TAG_structure_type {
+		DW_AT_name struct_type
+		DW_AT_byte_size 4 DW_FORM_data1
+	    } {
+		DW_TAG_member {
+			DW_AT_name field
+			DW_AT_type 0xCAFE1 DW_FORM_ref_sig8
+
+			# This is the expression that is going to be evaluated
+			# by printing the variable.
+			DW_AT_data_member_location {
+			    # In order to coax GDB to actually run this
+			    # expression in the evaluator, it needs to contain
+			    # something that will make decode_locdesc unable
+			    # to reduce it to a constant.  That's what these 3
+			    # operations (which are ultimately a no-op) are for.
+			    # Note that there is an implicit
+			    # DW_OP_push_object_address done before the
+			    # expression is evaluated.
+			    DW_OP_dup
+			    DW_OP_deref_size 1
+			    DW_OP_drop
+			} SPECIAL_expr
+		}
+	    }
+	}
+    }
+
+    # Definition of the CU.
+    cu {
+	fission 1
+	version 4
+    } {
+	set cu_debug_addr_label [debug_addr_label]
+
+	compile_unit {
+	    DW_AT_GNU_dwo_id 0xF00D DW_FORM_data8
+	} {
+	    DW_TAG_variable {
+		DW_AT_name global_var
+		DW_AT_type 0xCAFE2 DW_FORM_ref_sig8
+		DW_AT_location {
+		    DW_OP_GNU_addr_index [gdb_target_symbol buf]
+		} SPECIAL_expr
+	    }
+	}
+    }
+
+    # Debug info in the main file.
+
+    # Stub for "int" type unit.
+    tu {
+	version 4
+    } 0xCAFE1 "" {
+	type_unit {
+	    DW_AT_GNU_dwo_name ${::gdb_test_file_name}.dwo DW_FORM_strp
+	    DW_AT_comp_dir ${outdir}
+	} {
+	}
+    }
+
+    # Stub for "struct_type" type unit.
+    tu {
+	version 4
+    } 0xCAFE2 "" {
+	type_unit {
+	    DW_AT_GNU_dwo_name ${::gdb_test_file_name}.dwo DW_FORM_strp
+	    DW_AT_comp_dir ${outdir}
+	} {
+	}
+    }
+
+    # Stub for the CU.
+    cu {
+	version 4
+    } {
+	compile_unit {
+	    DW_AT_GNU_dwo_name ${::gdb_test_file_name}.dwo DW_FORM_strp
+	    DW_AT_comp_dir ${outdir}
+	    DW_AT_GNU_dwo_id 0xF00D DW_FORM_data8
+	    DW_AT_GNU_addr_base $cu_debug_addr_label
+	} {}
+    }
+}
+
+set object_file [standard_output_file ${testfile}.o]
+if { [build_executable_and_dwo_files "${testfile}.exp" ${binfile} {nodebug} \
+	  [list $asm_file {nodebug split-dwo} ${object_file}] \
+	  [list $srcfile {nodebug}]] } {
+    return -1
+}
+
+if { [is_remote host] } {
+    gdb_remote_download host $dwo_file
+}
+
+clean_restart ${testfile}
+
+# This print would cause the DW_AT_data_member_location expression to be
+# evaluated and cause the crash.
+gdb_test "print/x global_var" " = {field = 0x11222211}"
-- 
2.51.2

[PATCH 6/6] gdb/dwarf: use dwarf2_per_cu::ref_addr_size in one spot

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

I noticed this spot that could use dwarf2_per_cu::ref_addr_size, instead
of implementing the same logic.

Change-Id: I894ef1dcaa687c659e49bc1eb433e42818038cf2
---
 gdb/dwarf2/read.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index a8b9798c4e8e..c355cc2bb5cb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -3866,12 +3866,7 @@ cutu_reader::skip_one_attribute (dwarf_form form, const gdb_byte *info_ptr)
   switch (form)
     {
     case DW_FORM_ref_addr:
-      /* In DWARF 2, DW_FORM_ref_addr is address sized; in DWARF 3
-	       and later it is offset sized.  */
-      if (m_cu->header.version == 2)
-	return info_ptr + m_cu->header.addr_size;
-      else
-	return info_ptr + m_cu->header.offset_size;
+      return info_ptr + m_cu->per_cu->ref_addr_size ();
 
     case DW_FORM_GNU_ref_alt:
       return info_ptr + m_cu->header.offset_size;
-- 
2.51.2

Re: [PATCH 1/6] gdb/testsuite/dwarf: use single abbrev table in .dwo files

[Simon Marchi]

On 11/7/25 4:10 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> When I wrote test gdb.dwarf2/fission-with-type-unit.exp, I did not use
> build_executable_and_dwo_files, because it wouldn't work to have
> multiple units in the .dwo file, each referring to their own abbrev
> table using labels.  build_executable_and_dwo_files extracts the .dwo
> file content from the .o using objcopy (just like gcc does, I learned),
> meaning that the .dwo file never runs through a linker.  Anything
> needing relocation (like labels pointing to abbrev tables) doesn't work.
> 
> I instead opted to use gdb_compile_shlib to build the .dwo file on its
> own, so that those labels would get resolved.  That causes problems now
> that I'm trying to write a test with multiple type units in a .dwo file,
> where each type unit should be in its own .debug_types section.  Running
> the .dwo file through the linker causes all the .debug_types section to
> be collapsed into one.  And generally, I think it was a bad idea to
> generate a .dwo file using the linker, since the idea behind .dwo files
> is that they do not need to be linked (therefore improving link
> times).  We want to produce files as close to what an actual compiler
> would produce.
> 
> This patch fixes this by doing what compilers do in the same situation:
> use a single abbrev table shared by all units in the .dwo file.  This
> requires the following changes in lib/dwarf.exp:
> 
>  - Declare a new variable _dwo_abbrev_num, which holds the next
>    abbrev number to use in the .dwo file's abbrev section
>    (.debug_abbrev.dwo).  Initialize this variable to 1.
>  - When producing a CU or TU in a .dwo file, use 0 as the abbrev table
>    offset.
>  - When generating a DIE, return $_dwo_abbrev_num or $_abbrev_num,
>    depending on whether the current CU is in a .dwo file.
>  - After producing a CU or TU in a .dwo file, don't append the
>    terminator byte.
>  - After finishing producing the CUs and TUs, append the terminator byte
>    in .debug_abbrev.dwo if we did output anything there.
> 
> Update gdb.dwarf2/fission-with-type-units.exp to use
> build_executable_and_dwo_files, as it should.

I spotted that running this test with the cc-with-gdb-index board now
shows a new internal error.  I just wanted to note that this is not a
regression, it's actually the test going further and catching an actual
bug in GDB.  Please consider that the following text is appended to the
commit message.

---
    With this change, running with the cc-with-gdb-index board, I see:

        (gdb) maint expand-symtabs
        /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:3056: internal-error: cutu_reader: Assertion `sig_type->signature == cu->header.signature' failed.
        A problem internal to GDB has been detected,
        further debugging may prove unreliable.
        ----- Backtrace -----
        FAIL: gdb.dwarf2/fission-with-type-unit.exp: maint expand-symtabs (GDB internal error)

    This is actually an improvement, as the test case didn't run properly
    before.  The compilation failed with:

        gdb compile failed, During symbol reading: Could not find DWO CU fission-with-type-unit.dwo(0xf00d) referenced by CU at offset 0xc [in module /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/fission-with-type-unit/.tmp/fission-with-type-unit]

    The reason was that the old code would try to generate the GDB index
    during this step:

        # Build main file.
        if { [build_executable "${testfile}.exp" $binfile \
             [list ${srcfile} ${main_asm_file}] {nodebug}] } {
            return
        }

    ... which is even the DWO file is even generated.  With this patch
    things are done in the correct order:

     - The -dw.S file is generated
     - The -dw.o file is compiled from the -dw.S
     - The .dwo sections are extracted to the .dwo file, and stripped from
       the -dw.o file
     - The executable is linked from the .o and -dw.o
     - gdb-add-index is ran on the executable

    When gdb-add-index runs, the .dwo file exists, so GDB is able to produce
    an index.  That index is bogus though, because the .gdb_index format is
    unable to describe skeletonless type units.  And then GDB gets confused
    trying to use that index, leading to the internal error.
---

Simon

Re: [PATCH 1/6] gdb/testsuite/dwarf: use single abbrev table in .dwo files

[Andrew Burgess]

Simon Marchi <simon.marchi@polymtl.ca> writes:

> On 11/7/25 4:10 PM, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> 
>> When I wrote test gdb.dwarf2/fission-with-type-unit.exp, I did not use
>> build_executable_and_dwo_files, because it wouldn't work to have
>> multiple units in the .dwo file, each referring to their own abbrev
>> table using labels.  build_executable_and_dwo_files extracts the .dwo
>> file content from the .o using objcopy (just like gcc does, I learned),
>> meaning that the .dwo file never runs through a linker.  Anything
>> needing relocation (like labels pointing to abbrev tables) doesn't work.
>> 
>> I instead opted to use gdb_compile_shlib to build the .dwo file on its
>> own, so that those labels would get resolved.  That causes problems now
>> that I'm trying to write a test with multiple type units in a .dwo file,
>> where each type unit should be in its own .debug_types section.  Running
>> the .dwo file through the linker causes all the .debug_types section to
>> be collapsed into one.  And generally, I think it was a bad idea to
>> generate a .dwo file using the linker, since the idea behind .dwo files
>> is that they do not need to be linked (therefore improving link
>> times).  We want to produce files as close to what an actual compiler
>> would produce.
>> 
>> This patch fixes this by doing what compilers do in the same situation:
>> use a single abbrev table shared by all units in the .dwo file.  This
>> requires the following changes in lib/dwarf.exp:
>> 
>>  - Declare a new variable _dwo_abbrev_num, which holds the next
>>    abbrev number to use in the .dwo file's abbrev section
>>    (.debug_abbrev.dwo).  Initialize this variable to 1.
>>  - When producing a CU or TU in a .dwo file, use 0 as the abbrev table
>>    offset.
>>  - When generating a DIE, return $_dwo_abbrev_num or $_abbrev_num,
>>    depending on whether the current CU is in a .dwo file.
>>  - After producing a CU or TU in a .dwo file, don't append the
>>    terminator byte.
>>  - After finishing producing the CUs and TUs, append the terminator byte
>>    in .debug_abbrev.dwo if we did output anything there.
>> 
>> Update gdb.dwarf2/fission-with-type-units.exp to use
>> build_executable_and_dwo_files, as it should.
>
> I spotted that running this test with the cc-with-gdb-index board now
> shows a new internal error.  I just wanted to note that this is not a
> regression, it's actually the test going further and catching an actual
> bug in GDB.  Please consider that the following text is appended to the
> commit message.
>
> ---
>     With this change, running with the cc-with-gdb-index board, I see:
>
>         (gdb) maint expand-symtabs
>         /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:3056: internal-error: cutu_reader: Assertion `sig_type->signature == cu->header.signature' failed.
>         A problem internal to GDB has been detected,
>         further debugging may prove unreliable.
>         ----- Backtrace -----
>         FAIL: gdb.dwarf2/fission-with-type-unit.exp: maint expand-symtabs (GDB internal error)
>
>     This is actually an improvement, as the test case didn't run properly
>     before.  The compilation failed with:
>
>         gdb compile failed, During symbol reading: Could not find DWO CU fission-with-type-unit.dwo(0xf00d) referenced by CU at offset 0xc [in module /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/fission-with-type-unit/.tmp/fission-with-type-unit]
>
>     The reason was that the old code would try to generate the GDB index
>     during this step:
>
>         # Build main file.
>         if { [build_executable "${testfile}.exp" $binfile \
>              [list ${srcfile} ${main_asm_file}] {nodebug}] } {
>             return
>         }
>
>     ... which is even the DWO file is even generated.  With this patch
>     things are done in the correct order:

I cannot parse the "which is even the DWO file is even generated" part
here.  Can you reword this?

>
>      - The -dw.S file is generated
>      - The -dw.o file is compiled from the -dw.S
>      - The .dwo sections are extracted to the .dwo file, and stripped from
>        the -dw.o file
>      - The executable is linked from the .o and -dw.o
>      - gdb-add-index is ran on the executable

Not critical, but I think s/ran/run/ would be better.

Thanks,
Andrew


>
>     When gdb-add-index runs, the .dwo file exists, so GDB is able to produce
>     an index.  That index is bogus though, because the .gdb_index format is
>     unable to describe skeletonless type units.  And then GDB gets confused
>     trying to use that index, leading to the internal error.
> ---
>
> Simon

Re: [PATCH 1/6] gdb/testsuite/dwarf: use single abbrev table in .dwo files

[Simon Marchi]

On 11/18/25 8:32 AM, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>> On 11/7/25 4:10 PM, Simon Marchi wrote:
>>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>>
>>> When I wrote test gdb.dwarf2/fission-with-type-unit.exp, I did not use
>>> build_executable_and_dwo_files, because it wouldn't work to have
>>> multiple units in the .dwo file, each referring to their own abbrev
>>> table using labels.  build_executable_and_dwo_files extracts the .dwo
>>> file content from the .o using objcopy (just like gcc does, I learned),
>>> meaning that the .dwo file never runs through a linker.  Anything
>>> needing relocation (like labels pointing to abbrev tables) doesn't work.
>>>
>>> I instead opted to use gdb_compile_shlib to build the .dwo file on its
>>> own, so that those labels would get resolved.  That causes problems now
>>> that I'm trying to write a test with multiple type units in a .dwo file,
>>> where each type unit should be in its own .debug_types section.  Running
>>> the .dwo file through the linker causes all the .debug_types section to
>>> be collapsed into one.  And generally, I think it was a bad idea to
>>> generate a .dwo file using the linker, since the idea behind .dwo files
>>> is that they do not need to be linked (therefore improving link
>>> times).  We want to produce files as close to what an actual compiler
>>> would produce.
>>>
>>> This patch fixes this by doing what compilers do in the same situation:
>>> use a single abbrev table shared by all units in the .dwo file.  This
>>> requires the following changes in lib/dwarf.exp:
>>>
>>>  - Declare a new variable _dwo_abbrev_num, which holds the next
>>>    abbrev number to use in the .dwo file's abbrev section
>>>    (.debug_abbrev.dwo).  Initialize this variable to 1.
>>>  - When producing a CU or TU in a .dwo file, use 0 as the abbrev table
>>>    offset.
>>>  - When generating a DIE, return $_dwo_abbrev_num or $_abbrev_num,
>>>    depending on whether the current CU is in a .dwo file.
>>>  - After producing a CU or TU in a .dwo file, don't append the
>>>    terminator byte.
>>>  - After finishing producing the CUs and TUs, append the terminator byte
>>>    in .debug_abbrev.dwo if we did output anything there.
>>>
>>> Update gdb.dwarf2/fission-with-type-units.exp to use
>>> build_executable_and_dwo_files, as it should.
>>
>> I spotted that running this test with the cc-with-gdb-index board now
>> shows a new internal error.  I just wanted to note that this is not a
>> regression, it's actually the test going further and catching an actual
>> bug in GDB.  Please consider that the following text is appended to the
>> commit message.
>>
>> ---
>>     With this change, running with the cc-with-gdb-index board, I see:
>>
>>         (gdb) maint expand-symtabs
>>         /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:3056: internal-error: cutu_reader: Assertion `sig_type->signature == cu->header.signature' failed.
>>         A problem internal to GDB has been detected,
>>         further debugging may prove unreliable.
>>         ----- Backtrace -----
>>         FAIL: gdb.dwarf2/fission-with-type-unit.exp: maint expand-symtabs (GDB internal error)
>>
>>     This is actually an improvement, as the test case didn't run properly
>>     before.  The compilation failed with:
>>
>>         gdb compile failed, During symbol reading: Could not find DWO CU fission-with-type-unit.dwo(0xf00d) referenced by CU at offset 0xc [in module /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/fission-with-type-unit/.tmp/fission-with-type-unit]
>>
>>     The reason was that the old code would try to generate the GDB index
>>     during this step:
>>
>>         # Build main file.
>>         if { [build_executable "${testfile}.exp" $binfile \
>>              [list ${srcfile} ${main_asm_file}] {nodebug}] } {
>>             return
>>         }
>>
>>     ... which is even the DWO file is even generated.  With this patch
>>     things are done in the correct order:
> 
> I cannot parse the "which is even the DWO file is even generated" part
> here.  Can you reword this?

Oops.  I meant: ... which is before the DWO file is even generated.

If the main file exists but the DWO file doesn't exist yet, and you try
to generate an .gdb_index from that, it won't go very well, hopefully
that's clear.

> 
>>
>>      - The -dw.S file is generated
>>      - The -dw.o file is compiled from the -dw.S
>>      - The .dwo sections are extracted to the .dwo file, and stripped from
>>        the -dw.o file
>>      - The executable is linked from the .o and -dw.o
>>      - gdb-add-index is ran on the executable
> 
> Not critical, but I think s/ran/run/ would be better.

Noted, will fix.

Simon

Re: [PATCH 1/6] gdb/testsuite/dwarf: use single abbrev table in .dwo files

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> When I wrote test gdb.dwarf2/fission-with-type-unit.exp, I did not use
> build_executable_and_dwo_files, because it wouldn't work to have
> multiple units in the .dwo file, each referring to their own abbrev
> table using labels.  build_executable_and_dwo_files extracts the .dwo
> file content from the .o using objcopy (just like gcc does, I learned),
> meaning that the .dwo file never runs through a linker.  Anything
> needing relocation (like labels pointing to abbrev tables) doesn't work.
>
> I instead opted to use gdb_compile_shlib to build the .dwo file on its
> own, so that those labels would get resolved.  That causes problems now
> that I'm trying to write a test with multiple type units in a .dwo file,
> where each type unit should be in its own .debug_types section.  Running
> the .dwo file through the linker causes all the .debug_types section to
> be collapsed into one.  And generally, I think it was a bad idea to
> generate a .dwo file using the linker, since the idea behind .dwo files
> is that they do not need to be linked (therefore improving link
> times).  We want to produce files as close to what an actual compiler
> would produce.
>
> This patch fixes this by doing what compilers do in the same situation:
> use a single abbrev table shared by all units in the .dwo file.  This
> requires the following changes in lib/dwarf.exp:
>
>  - Declare a new variable _dwo_abbrev_num, which holds the next
>    abbrev number to use in the .dwo file's abbrev section
>    (.debug_abbrev.dwo).  Initialize this variable to 1.
>  - When producing a CU or TU in a .dwo file, use 0 as the abbrev table
>    offset.
>  - When generating a DIE, return $_dwo_abbrev_num or $_abbrev_num,
>    depending on whether the current CU is in a .dwo file.
>  - After producing a CU or TU in a .dwo file, don't append the
>    terminator byte.
>  - After finishing producing the CUs and TUs, append the terminator byte
>    in .debug_abbrev.dwo if we did output anything there.
>
> Update gdb.dwarf2/fission-with-type-units.exp to use
> build_executable_and_dwo_files, as it should.
>
> Change-Id: Iabbcf00db97faf2a4fa5fc71652ad273081189f9
> ---
>  .../gdb.dwarf2/fission-with-type-unit.exp     | 29 +++-----
>  gdb/testsuite/lib/dwarf.exp                   | 69 +++++++++++++++----
>  2 files changed, 67 insertions(+), 31 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp b/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp
> index 58cda296c67b..5c73001ec66b 100644
> --- a/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp
> +++ b/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp
> @@ -21,25 +21,22 @@ load_lib dwarf.exp
>  # This test can only be run on targets which support DWARF-2 and use gas.
>  require dwarf2_support
>  
> -standard_testfile .c -dw.S -dwo.S
> +standard_testfile .c -dw.S
>  
> -set main_asm_file [standard_output_file $srcfile2]
> -set dwo_asm_file [standard_output_file $srcfile3]
> +set asm_file [standard_output_file $srcfile2]
>  
> -# Debug info in the main file.
> -Dwarf::assemble $main_asm_file {
> +Dwarf::assemble $asm_file {
> +    # In the main file.
>      cu {
>  	version 5
>  	dwo_id 0xF00D
>      } {
>  	compile_unit {
> -	    DW_AT_dwo_name ${::gdb_test_file_name}.dwo DW_FORM_strp
> +	    DW_AT_dwo_name ${::gdb_test_file_name}-dw.dwo DW_FORM_strp
>  	} {}
>      }
> -}
>  
> -# Debug info in the DWO file.
> -Dwarf::assemble $dwo_asm_file {
> +    # In the .dwo file.
>      tu {
>  	fission 1
>  	version 5
> @@ -79,15 +76,11 @@ Dwarf::assemble $dwo_asm_file {
>      }
>  }
>  
> -# Build main file.
> -if { [build_executable "${testfile}.exp" $binfile \
> -	[list ${srcfile} ${main_asm_file}] {nodebug}] } {
> -    return
> -}
> -
> -# Build DWO file.
> -set dwo_file [standard_output_file ${testfile}.dwo]
> -if { [gdb_compile_shlib $dwo_asm_file $dwo_file nodebug] != "" } {
> +set obj [standard_output_file "${testfile}-dw.o"]
> +set dwo_file [standard_output_file "${testfile}.dwo"]

The name of the dwo file here is now wrong.  The auto generated name
will be '${testfile}-dw.dwo', which is what you embed in the DWARF
above.

I was wondering why this wasn't causing problems for the later block in
this file (not touched in this patch):

  if { [is_remote host] } {
      gdb_remote_download host $dwo_file
  }

But it turns out that ...

> +if {[build_executable_and_dwo_files "$testfile.exp" "${binfile}" {} \

... build_executable_and_dwo_files includes this check:

    # Must be run on local host due to use of objcopy.
    if {[is_remote host]} {
	return -1
    }

So this test never gets past this point for remote host boards.

I think you should:

  1. Remove the 'set dwo_file ...' line.

  2. Remove the 'gdb_remote_download host $dwo_file' line, and its
     containing 'if' block.

  3. Add a 'require {!is_remote host}' line at the start of the file.

Otherwise, this change looks good to me (along with the additional text
you posted in the follow up, which I also replied to).

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


> +	 [list $asm_file {nodebug split-dwo} $obj] \
> +	 [list $srcfile  {nodebug}]]} {
>      return
>  }
>  
> diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
> index ba8197d0bb8e..919e2e05687f 100644
> --- a/gdb/testsuite/lib/dwarf.exp
> +++ b/gdb/testsuite/lib/dwarf.exp
> @@ -563,6 +563,9 @@ namespace eval Dwarf {
>      # table.
>      variable _abbrev_num
>  
> +    # The next available abbrev number in the (single) DWO abbrev table.
> +    variable _dwo_abbrev_num
> +
>      # The string table for this assembly.  The key is the string; the
>      # value is the label for that string.
>      variable _strings
> @@ -1008,8 +1011,17 @@ namespace eval Dwarf {
>      # table.
>      proc _get_abbrev_num {} {
>  	variable _abbrev_num
> -	set res $_abbrev_num
> -	incr _abbrev_num
> +	variable _dwo_abbrev_num
> +	variable _cu_is_fission
> +
> +	if { $_cu_is_fission } {
> +	    set res $_dwo_abbrev_num
> +	    incr _dwo_abbrev_num
> +	} else {
> +	    set res $_abbrev_num
> +	    incr _abbrev_num
> +	}
> +
>  	return $res
>      }
>  
> @@ -1538,8 +1550,17 @@ namespace eval Dwarf {
>  	_section $section
>  
>  	set cu_num [incr _cu_count]
> -	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> -	set _abbrev_num 1
> +
> +	# Since .dwo files are not linked, we can't use a label to point to a
> +	# specific place in the .debug_abbrev section.  For .dwo files, we
> +	# therefore use a single abbrev table (at offset 0) shared by all units
> +	# in that file.
> +	if { $_cu_is_fission } {
> +	    set my_abbrevs 0
> +	} else {
> +	    set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> +	    set _abbrev_num 1
> +	}
>  
>  	set _cu_label [_compute_label "cu${cu_num}_begin"]
>  	set start_label [_compute_label "cu${cu_num}_start"]
> @@ -1606,9 +1627,11 @@ namespace eval Dwarf {
>  
>  	uplevel $_level $body
>  
> -	_defer_output $_abbrev_section {
> -	    # Emit the terminator.
> -	    _op .byte 0x0 "Abbrev end - Terminator"
> +	if { !$_cu_is_fission } {
> +	    _defer_output $_abbrev_section {
> +		# Emit the terminator.
> +		_op .byte 0x0 "Abbrev end - Terminator"
> +	    }
>  	}
>  
>  	define_label $end_label
> @@ -1681,8 +1704,17 @@ namespace eval Dwarf {
>  	_section $section
>  
>  	set cu_num [incr _cu_count]
> -	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> -	set _abbrev_num 1
> +
> +	# Since .dwo files are not linked, we can't use a label to point to a
> +	# specific place in the .debug_abbrev section.  For .dwo files, we
> +	# therefore use a single abbrev table (at offset 0) shared by all units
> +	# in that file.
> +	if { $_cu_is_fission } {
> +		set my_abbrevs 0
> +	} else {
> +		set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> +		set _abbrev_num 1
> +	}
>  
>  	set _cu_label [_compute_label "cu${cu_num}_begin"]
>  	set start_label [_compute_label "cu${cu_num}_start"]
> @@ -1736,9 +1768,11 @@ namespace eval Dwarf {
>  
>  	uplevel $_level $body
>  
> -	_defer_output $_abbrev_section {
> -	    # Emit the terminator.
> -	    _op .byte 0x0 "Abbrev end - Terminator"
> +	if { !$_cu_is_fission } {
> +	    _defer_output $_abbrev_section {
> +		# Emit the terminator.
> +		_op .byte 0x0 "Abbrev end - Terminator"
> +	    }
>  	}
>  
>  	define_label $end_label
> @@ -3507,6 +3541,7 @@ namespace eval Dwarf {
>  	variable _debug_ranges_64_bit
>  	variable _debug_addr_index
>  	variable _level
> +	variable _dwo_abbrev_num
>  
>  	if { [llength $options] == 1 } {
>  	    set options [list filename [lindex $options 0]]
> @@ -3539,8 +3574,8 @@ namespace eval Dwarf {
>  
>  	set _line_count 0
>  	set _debug_ranges_64_bit [is_64_target]
> -
>  	set _debug_addr_index 0
> +	set _dwo_abbrev_num 1
>  
>  	# Dummy CU at the start to ensure that the first CU in $body is not
>  	# the first in .debug_info.
> @@ -3562,6 +3597,14 @@ namespace eval Dwarf {
>  	    dummy_cu
>  	}
>  
> +	# If we wrote any abbrev in .debug_abbrev.dwo, write the terminator.
> +	if { $_dwo_abbrev_num > 1 } {
> +	    _defer_output .debug_abbrev.dwo {
> +		# Emit the terminator.
> +		_op .byte 0x0 "Abbrev end - Terminator"
> +	    }
> +	}
> +
>  	_write_deferred_output
>  
>  	_section .note.GNU-stack "" progbits
> -- 
> 2.51.2

Re: [PATCH 1/6] gdb/testsuite/dwarf: use single abbrev table in .dwo files

[Simon Marchi]

On 11/18/25 11:55 AM, Andrew Burgess wrote:
>> -# Build main file.
>> -if { [build_executable "${testfile}.exp" $binfile \
>> -	[list ${srcfile} ${main_asm_file}] {nodebug}] } {
>> -    return
>> -}
>> -
>> -# Build DWO file.
>> -set dwo_file [standard_output_file ${testfile}.dwo]
>> -if { [gdb_compile_shlib $dwo_asm_file $dwo_file nodebug] != "" } {
>> +set obj [standard_output_file "${testfile}-dw.o"]
>> +set dwo_file [standard_output_file "${testfile}.dwo"]
> 
> The name of the dwo file here is now wrong.  The auto generated name
> will be '${testfile}-dw.dwo', which is what you embed in the DWARF
> above.

Oh, indeed.

> I was wondering why this wasn't causing problems for the later block in
> this file (not touched in this patch):
> 
>   if { [is_remote host] } {
>       gdb_remote_download host $dwo_file
>   }
> 
> But it turns out that ...
> 
>> +if {[build_executable_and_dwo_files "$testfile.exp" "${binfile}" {} \
> 
> ... build_executable_and_dwo_files includes this check:
> 
>     # Must be run on local host due to use of objcopy.
>     if {[is_remote host]} {
> 	return -1
>     }

I don't understand why that wouldn't be possible if debugging on a
remote host.  Here, we are building target objects, we have a toolchain
for the target, so we should have an objcopy for the target that we can
run.  It should be possible, in theory.  Am I missing something?

> 
> So this test never gets past this point for remote host boards.
> 
> I think you should:
> 
>   1. Remove the 'set dwo_file ...' line.
> 
>   2. Remove the 'gdb_remote_download host $dwo_file' line, and its
>      containing 'if' block.
> 
>   3. Add a 'require {!is_remote host}' line at the start of the file.

Since build_executable_and_dwo_files returns -1 if the host is remote,
causing the test to exit early an cleanly (although we could perhaps add
an untested call somewhere), do we really need the require at the top?
If we ever lift the remote host limitation of
build_executable_and_dwo_files, then we'll want the test to run.

Simon

Re: [PATCH 2/6] gdb/testsuite/dwarf: convert _section proc to use parse_options

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> The following patch will add more options to the _section proc, so
> convert it to use options, to be more user-friendly.

LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> Change-Id: I971e4e10e55d63af2a5e29dc5e1d00f368b3ecaa
> ---
>  gdb/testsuite/lib/dwarf.exp | 45 +++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
> index 919e2e05687f..5536b7656b8c 100644
> --- a/gdb/testsuite/lib/dwarf.exp
> +++ b/gdb/testsuite/lib/dwarf.exp
> @@ -1071,17 +1071,33 @@ namespace eval Dwarf {
>  	}
>      }
>  
> -    proc _section {name {flags ""} {type ""}} {
> -	if {$name == ".debug_str"} {
> -	    # Hard-code this because it's always desirable.
> -	    _emit "        .section $name, \"MS\", %progbits, 1"
> -	} elseif {$flags == "" && $type == ""} {
> -	    _emit "        .section $name"
> -	} elseif {$type == ""} {
> -	    _emit "        .section $name, \"$flags\""
> -	} else {
> -	    _emit "        .section $name, \"$flags\", %$type"
> +    proc _section {name {options {}}} {
> +	parse_options {
> +	    { flags "" }
> +	    { type "" }
>  	}
> +
> +	set directive ".section $name"
> +
> +	if { $name == ".debug_str" } {
> +	    # Hard-code this because it's always desirable.
> +	    append directive ", \"MS\", %progbits, 1"
> +	}
> +
> +	if { $flags != "" } {
> +	    append directive ",\"$flags\""
> +	}
> +
> +	if { $type != "" } {
> +	    # Can't specify a type without flags.
> + 	    if { $flags == "" } {
> +		append directive ", \"\""
> +	    }
> +
> +	    append directive ", %$type"
> +	}
> +
> +	_emit "        $directive"
>      }
>  
>      # SECTION_SPEC is a list of arguments to _section.
> @@ -3173,7 +3189,10 @@ namespace eval Dwarf {
>  
>      # Emit a note section holding the given build-id.
>      proc build_id {buildid} {
> -	_defer_output {.note.gnu.build-id a note} {
> +	_defer_output {.note.gnu.build-id {
> +			   flags a
> +			   type note
> +		       }} {
>  	    # From elf/common.h.
>  	    set NT_GNU_BUILD_ID 3
>  
> @@ -3607,7 +3626,9 @@ namespace eval Dwarf {
>  
>  	_write_deferred_output
>  
> -	_section .note.GNU-stack "" progbits
> +	_section .note.GNU-stack {
> +	    type progbits
> +	}
>  
>  	if { $file_id == "" } {
>  	    catch {close $_output_file}
> -- 
> 2.51.2

Re: [PATCH 3/6] gdb/testsuite/dwarf: emit type unit sections as COMDAT

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> In an effort to generate ELF files and DWARF info as close as possible
> as an actual compiler would generate, change how we emit type unit
> sections to emit each type unit in its own section, in COMDAT section
> groups.
>
> We currently emit all type units in a single, standard section (either
> .debug_info or .debug_types, depending on the DWARF version).  Compilers
> emit each type unit in its own .debug_types section.  Each section is
> placed in a COMDAT section group with a signature based on the type
> unit's signature.  This lets the linker deduplicate identical type units
> by discarding section groups with identical signatures (keeping only one
> group with a given signature).
>
> Looking at a .s file generated by gcc, we can see that this is how it
> specifies the section for a type unit:
>
>     .section .debug_info,"G",@progbits,wi.006fd2152a3054a6,comdat
>
> The "G" flag tells the assembler to place the section in a section
> group with signature "wi.006fd2152a3054a6".  That string was generated
> from the type unit, signature.  Finally, the comdat linkage field
> indicates that the section group should have the COMDAT flag.
>
> Update the tu proc to emit a section with these properties.  In this
> case, it's mandatory to specify the type of the section (progbits).
>
> Update the _section proc to accept the new options "group_signature" and
> "linkage".
>
> As a result, if you look at the .debug_types section in a .o file from
> gcc:
>
>     $ readelf -g main.o
>     COMDAT group section [    1] `.group' [wt.006fd2152a3054a6] contains 2 sections:
>        [Index]    Name
>        [   11]   .debug_types
>        [   12]   .rela.debug_types
>
>     COMDAT group section [    2] `.group' [wt.c621aa8e3c23e450] contains 2 sections:
>        [Index]    Name
>        [   13]   .debug_types
>        [   14]   .rela.debug_types
>
> And a .o file created by our DWARF assembler:
>
>     $ readelf -g testsuite/outputs/gdb.dwarf2/struct-with-sig/struct-with-sig1.o
>     COMDAT group section [    1] `.group' [sig.0x0000000000000001] contains 2 sections:
>        [Index]    Name
>        [   10]   .debug_types
>        [   11]   .rela.debug_types
>
>     COMDAT group section [    2] `.group' [sig.0x0000000000000002] contains 2 sections:
>        [Index]    Name
>        [   12]   .debug_types
>        [   13]   .rela.debug_types
>
> In both cases, in the fully linked files, there is a single .debug_types section
> containing the two type units, as expected.
>
> Before this patch, when I run gdb.dwarf2/fission-with-type-unit.exp, the
> resulting .dwo file has a single .debug_info.dwo.  After this patch it
> has two: one with the type unit and one with the compile unit (the test
> uses DWARF 5).  Based on what I see gcc generate when using "-gdwarf-5
> -gsplit-dwarf -fdebug-types-section", this is what we expect.

I think I followed all of this.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> Change-Id: Ie1954dc697fe100b5dbe664d148c71fa02888d96
> ---
>  gdb/testsuite/lib/dwarf.exp | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
> index 5536b7656b8c..c3c35d43607d 100644
> --- a/gdb/testsuite/lib/dwarf.exp
> +++ b/gdb/testsuite/lib/dwarf.exp
> @@ -1075,6 +1075,8 @@ namespace eval Dwarf {
>  	parse_options {
>  	    { flags "" }
>  	    { type "" }
> +	    { group_signature "" }
> +	    { linkage "" }
>  	}
>  
>  	set directive ".section $name"
> @@ -1097,6 +1099,14 @@ namespace eval Dwarf {
>  	    append directive ", %$type"
>  	}
>  
> +	if { $group_signature != "" } {
> +	    append directive ", $group_signature"
> +	}
> +
> +	if { $linkage != "" } {
> +	    append directive ", $linkage"
> +	}
> +
>  	_emit "        $directive"
>      }
>  
> @@ -1717,7 +1727,13 @@ namespace eval Dwarf {
>  	    set _abbrev_section "$_abbrev_section.dwo"
>  	}
>  
> -	_section $section
> +	# The specific format of the signature is arbitrary.
> +	_section $section {
> +	    flags G
> +	    type progbits
> +	    group_signature sig.$signature
> +	    linkage comdat
> +	}
>  
>  	set cu_num [incr _cu_count]
>  
> -- 
> 2.51.2

Re: [PATCH 4/6] gdb/dwarf: when in dwarf2_cu, read addr_size from dwarf2_cu::header

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> This patch fixes a crash caused by GDB trying to read from a section not
> read in.  The bug happens in those specific circumstances:
>
>  - reading a type unit from .dwo
>  - that type unit has a stub in the main file
>  - there is a GDB index (.gdb_index) present
>
> This crash is the cause of the following test failure:
>
>     $ Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/fission-reread.exp ...
>     ERROR: GDB process no longer exists

It's probably worth mentioning that this failure only occurs when using
the cc-with-gdb-index board.

>
> Or, manually:
>
>     $ ./gdb -nx -q --data-directory=data-directory /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/fission-reread/fission-reread -ex "p 1"
>     Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/fission-reread/fission-reread...
>
>     Fatal signal: Segmentation fault
>
> For this last one, you need to interrupt the test (e.g. add a return)
> before the test deletes the .dwo file.
>
> The backtrace at the moment of the crash is:
>
>     #0  0x0000555566968f7f in bfd_getl32 (p=0x0) at /home/simark/src/binutils-gdb/bfd/libbfd.c:846
>     #1  0x00005555642e561d in read_initial_length (abfd=0x7d1ff1eb0e40, buf=0x0, bytes_read=0x7bfff0962fa0, handle_nonstd=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/leb.c:92
>     #2  0x00005555647ca9ea in read_unit_head (header=0x7d0ff1e068b0, info_ptr=0x0, section=0x7c3ff1dea7d0, section_kind=ruh_kind::COMPILE) at /home/simark/src/binutils-gdb/gdb/dwarf2/unit-head.c:44
>     #3  0x000055556452e37e in dwarf2_per_cu::get_header (this=0x7d0ff1e06880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18531
>     #4  0x000055556452e574 in dwarf2_per_cu::addr_size (this=0x7d0ff1e06880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18544
>     #5  0x000055556406af91 in dwarf2_cu::addr_type (this=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/cu.c:124
>     #6  0x0000555564534e48 in set_die_type (die=0x7e0ff1f23dd0, type=0x7e0ff1f027f0, cu=0x7d7ff1e20880, skip_data_location=false) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:19020
>     #7  0x00005555644dcc7b in read_structure_type (die=0x7e0ff1f23dd0, cu=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:11239
>     #8  0x000055556451c834 in read_type_die_1 (die=0x7e0ff1f23dd0, cu=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16878
>     #9  0x000055556451c5e0 in read_type_die (die=0x7e0ff1f23dd0, cu=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16861
>     #10 0x0000555564526f3a in get_signatured_type (die=0x7e0ff1f0ffb0, signature=10386129560629316377, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:17998
>     #11 0x000055556451c23b in lookup_die_type (die=0x7e0ff1f0ffb0, attr=0x7e0ff1f10008, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16808
>     #12 0x000055556451b2e9 in die_type (die=0x7e0ff1f0ffb0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16684
>     #13 0x000055556451457f in new_symbol (die=0x7e0ff1f0ffb0, type=0x0, cu=0x7d7ff1e0f480, space=0x0) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16089
>     #14 0x00005555644c52a4 in read_variable (die=0x7e0ff1f0ffb0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:9119
>     #15 0x0000555564494072 in process_die (die=0x7e0ff1f0ffb0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:5197
>     #16 0x000055556449c88e in read_file_scope (die=0x7e0ff1f0fdd0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:6125
>     #17 0x0000555564493671 in process_die (die=0x7e0ff1f0fdd0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:5098
>     #18 0x00005555644912f5 in process_full_comp_unit (cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:4851
>     #19 0x0000555564485e18 in process_queue (per_objfile=0x7d6ff1e71100) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:4161
>     #20 0x000055556446391d in dw2_do_instantiate_symtab (per_cu=0x7ceff1de42d0, per_objfile=0x7d6ff1e71100, skip_partial=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:1650
>     #21 0x0000555564463b3c in dw2_instantiate_symtab (per_cu=0x7ceff1de42d0, per_objfile=0x7d6ff1e71100, skip_partial=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:1671
>     #22 0x00005555644687fd in dwarf2_base_index_functions::expand_all_symtabs (this=0x7c1ff1e04990, objfile=0x7d5ff1e46080) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:1990
>     #23 0x0000555564381050 in cooked_index_functions::expand_all_symtabs (this=0x7c1ff1e04990, objfile=0x7d5ff1e46080) at /home/simark/src/binutils-gdb/gdb/dwarf2/cooked-index.h:237
>     #24 0x0000555565df5b0d in objfile::expand_all_symtabs (this=0x7d5ff1e46080) at /home/simark/src/binutils-gdb/gdb/symfile-debug.c:372
>     #25 0x0000555565eafc4a in maintenance_expand_symtabs (args=0x0, from_tty=1) at /home/simark/src/binutils-gdb/gdb/symmisc.c:914
>
> The main file contains a stub (skeleton) for a compilation unit and a
> stub for a type unit.   The .dwo file contains a compilation unit and a
> type unit matching those stubs.  When doing the initial scan of the main
> file, the DWARF reader parses the CU/TU list from the GDB index
> (.gdb_index), and thus creates a signatured_type object based on that.
> The section field of this signatured_type points to the .debug_types
> section in the main file, the one containing the stub.  And because GDB
> trusts the GDB index, it never needs to look at that .debug_types
> section in the main file.  That section remains not read in.
>
> When expanding the compilation unit, GDB encounters a type unit
> reference (by signature) corresponding to the type in the type unit.  We
> get in lookup_dwo_signatured_type, trying to see if there is a type unit
> matching that signature in the current .dwo file.  We proceed to read
> and expand that type unit, until we eventually get to a
> dwarf2_cu::addr_type() call, which doess:
>
>      int addr_size = this->per_cu->addr_size ();
>
> dwarf2_per_cu::addr_size() tries to read the header from the section
> pointed to by dwarf2_per_cu::section which, if you recall, is the
> .debug_types section in the main file that was never read in.  That
> causes the segfault.
>
> All this was working fine before these patches of mine, that tried to do
> some cleanups:
>
>     a47e2297fc28 ("gdb/dwarf: pass section offset to dwarf2_per_cu_data constructor")
>     c44ab627b021 ("gdb/dwarf: pass section to dwarf2_per_cu_data constructor")
>     39ee8c928551 ("gdb/dwarf: pass unit length to dwarf2_per_cu_data constructor")
>
> Before these patches, the fill_in_sig_entry_from_dwo_entry function
> (called from lookup_dwo_signatured_type, among others) would overwrite
> some dwarf2_per_cu fields (including the section) to point to the .dwo,
> rather than represent what's in the main file.  Therefore, the header
> would have been read from the unit in the .dwo file, and things would
> have been fine.
>
> When doing these changes, I mistakenly assumed that the section written
> by fill_in_sig_entry_from_dwo_entry was the same as the section already
> there, which is why I removed the statements overwriting the section
> field (and the two others).  To my defense, here's the comment on
> dwarf2_per_cu::section:
>
>     /* The section this CU/TU lives in.
>        If the DIE refers to a DWO file, this is always the original die,
>        not the DWO file.  */
>     struct dwarf2_section_info *section = nullptr;
>
> I would prefer to not reintroduce the behavior of overwriting the
> section info in dwarf2_per_cu, because:
>
>  1. I find it confusing, I like the invariant of dwarf2_per_cu::section
>     points to the stub, and dwarf2_cu::section points to where we
>     actually read the debug info from.
>  2. The dwarf2_per_bfd::all_units vector is nowadays sorted by (section,
>     section offset).  If we change the section and section offset of a
>     dwarf2_per_cu, then we can no longer do binary searches in it, we
>     would have to re-sort the vector (not a big deal, but still adds to
>     the confusion).
>
> One possible fix would be to make sure that the section is read in when
> reading the header, probably in dwarf2_per_cu::get_header.  An approach
> like that was proposed by Andrew initially, here:
>
>   https://inbox.sourceware.org/gdb-patches/60ba2b019930fd6164f8e6ab6cb2e396c32c6ac2.1759486109.git.aburgess@redhat.com/
>
> It would work, but there is a more straightforward fix for this
> particular problem, I believe.  In dwarf2_cu, we have access to the
> header read from the unit we're actually reading the DWARF from.  In the
> DWO case, that is the header read from the .dwo file.  We can get the
> address size from there instead of going through the dwarf2_per_cu
> object.  This is what this patch does.

This looks good to me.

I would encourage you to consider extending the existing test to cover
this case without needing to test with the cc-with-gdb-index board.  I
think the reality is most people don't test every patch with every
board.  But I do understand that, if taken to extreme, that would mean
running every test in every mode, which clearly doesn't scale.  But when
we run into a regression my instinct is to ensure that the default test
mode covers this case.  And there's plenty of precedent for this in the
testsuite, e.g. gdb.server/*.exp.  You're welcome to take the test
changes from the patch linked above if that is useful.

Also in the patch linked above is the addition of an assert:

  gdb_assert (this->section ()->readin);

in 'dwarf2_per_cu::get_header ()'.  I haven't checked the later patches
from this series yet, so maybe you already do something similar, but if
not, maybe it would be good to add something like that in this patch,
given that this is dealing with the assert caused by the section not
being read in?

Anyway, these aren't hard requirements, just my thoughts.  I think the
fix looks good.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH 5/6] gdb/dwarf: store addr/offset/ref_addr sizes in dwarf2_per_cu

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> In some specific circumstances, it is possible for GDB to read a type
> unit from a .dwo file without ever reading in the section of the stub in
> the main file.  In that case, calling any of these methods:
>
>   - dwarf2_per_cu::addr_size()
>   - dwarf2_per_cu::offset_size()
>   - dwarf2_per_cu::ref_addr_size()
>
> will cause a crash, because they will try to read the unit header from
> the not-read-in section buffer.  See the previous patch for more
> details.
>
> The remaining calls to these methods are in the loc.c and expr.c
> files.  That is, in the location and expression machinery.  It is
> possible to set things up to cause them to trigger a crash, as shown by
> the new test, when running it with the cc-with-gdb-index board:
>
>     $ make check TESTS="gdb.dwarf2/fission-type-unit-locexpr.exp" RUNTESTFLAGS="--target_board=cc-with-gdb-index"
>     Running /home/simark/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp ...
>     ERROR: GDB process no longer exists
>
> The backtrace at the moment of the crash is:
>
>     #0  0x0000555566968b1f in bfd_getl32 (p=0x78) at /home/simark/src/binutils-gdb/bfd/libbfd.c:846
>     #1  0x00005555642e51b7 in read_initial_length (abfd=0x7d1ff1eb0e40, buf=0x78 <error: Cannot access memory at address 0x78>, bytes_read=0x7bfff09daca0, handle_nonstd=true)
>         at /home/simark/src/binutils-gdb/gdb/dwarf2/leb.c:92
>     #2  0x00005555647ca584 in read_unit_head (header=0x7d0ff1e06c70, info_ptr=0x78 <error: Cannot access memory at address 0x78>, section=0x7c3ff1dea7d0, section_kind=ruh_kind::COMPILE)
>         at /home/simark/src/binutils-gdb/gdb/dwarf2/unit-head.c:44
>     #3  0x000055556452df18 in dwarf2_per_cu::get_header (this=0x7d0ff1e06c40) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18531
>     #4  0x000055556452e10e in dwarf2_per_cu::addr_size (this=0x7d0ff1e06c40) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18544
>     #5  0x0000555564314ac3 in dwarf2_locexpr_baton_eval (dlbaton=0x7bfff0c9a508, frame=..., addr_stack=0x7bfff0b59150, valp=0x7bfff0c9a430, push_values=..., is_reference=0x7bfff0d33030)
>         at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1593
>     #6  0x0000555564315bd2 in dwarf2_evaluate_property (prop=0x7bfff0c9a450, initial_frame=..., addr_stack=0x7bfff0b59150, value=0x7bfff0c9a430, push_values=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1668
>     #7  0x0000555564a14ee1 in resolve_dynamic_field (field=..., addr_stack=0x7bfff0b59150, frame=...) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:2758
>     #8  0x0000555564a15e24 in resolve_dynamic_struct (type=0x7e0ff1f02550, addr_stack=0x7bfff0b59150, frame=...) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:2839
>     #9  0x0000555564a17061 in resolve_dynamic_type_internal (type=0x7e0ff1f02550, addr_stack=0x7bfff0b59150, frame=..., top_level=true) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:2972
>     #10 0x0000555564a17899 in resolve_dynamic_type (type=0x7e0ff1f02550, valaddr=..., addr=0x4010, in_frame=0x7bfff0d32e60) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:3019
>     #11 0x000055556675fb34 in value_from_contents_and_address (type=0x7e0ff1f02550, valaddr=0x0, address=0x4010, frame=...) at /home/simark/src/binutils-gdb/gdb/value.c:3674
>     #12 0x00005555666ce911 in get_value_at (type=0x7e0ff1f02550, addr=0x4010, frame=..., lazy=1) at /home/simark/src/binutils-gdb/gdb/valops.c:992
>     #13 0x00005555666ceb89 in value_at_lazy (type=0x7e0ff1f02550, addr=0x4010, frame=...) at /home/simark/src/binutils-gdb/gdb/valops.c:1039
>     #14 0x000055556491909f in language_defn::read_var_value (this=0x5555725fce40 <minimal_language_defn>, var=0x7e0ff1f02500, var_block=0x7e0ff1f025d0, frame_param=...)
>         at /home/simark/src/binutils-gdb/gdb/findvar.c:504
>     #15 0x000055556491961b in read_var_value (var=0x7e0ff1f02500, var_block=0x7e0ff1f025d0, frame=...) at /home/simark/src/binutils-gdb/gdb/findvar.c:518
>     #16 0x00005555666d1861 in value_of_variable (var=0x7e0ff1f02500, b=0x7e0ff1f025d0) at /home/simark/src/binutils-gdb/gdb/valops.c:1384
>     #17 0x00005555647f7099 in evaluate_var_value (noside=EVAL_NORMAL, blk=0x7e0ff1f025d0, var=0x7e0ff1f02500) at /home/simark/src/binutils-gdb/gdb/eval.c:533
>     #18 0x00005555647f740c in expr::var_value_operation::evaluate (this=0x7c2ff1e3b690, expect_type=0x0, exp=0x7c2ff1e3aa00, noside=EVAL_NORMAL) at /home/simark/src/binutils-gdb/gdb/eval.c:559
>     #19 0x00005555647f3347 in expression::evaluate (this=0x7c2ff1e3aa00, expect_type=0x0, noside=EVAL_NORMAL) at /home/simark/src/binutils-gdb/gdb/eval.c:109
>     #20 0x000055556543ac2f in process_print_command_args (args=0x7fffffffe728 "global_var", print_opts=0x7bfff0be4a30, voidprint=true) at /home/simark/src/binutils-gdb/gdb/printcmd.c:1328
>     #21 0x000055556543ae65 in print_command_1 (args=0x7fffffffe728 "global_var", voidprint=1) at /home/simark/src/binutils-gdb/gdb/printcmd.c:1341
>     #22 0x000055556543b707 in print_command (exp=0x7fffffffe728 "global_var", from_tty=1) at /home/simark/src/binutils-gdb/gdb/printcmd.c:1408
>
> The problem to solve is: in order to evaluate a location expression, we
> need to know some information (the various sizes) found in the unit
> header.  In that context, it's not possible to get it from
> dwarf2_cu::header, like the previous patch did: at the time the
> expression is evaluated, the corresponding dwarf2_cu might have been
> freed.  We don't want to re-build a dwarf2_cu just for that, it would be
> very inefficient.  We could force reading in the dwarf2_per_cu::section
> section (in the main file), but we never needed to read that section
> before, so it would be better to avoid reading it unnecessarily.
>
> My initial attempt was to store this information in baton objects
> (dwarf2_locexpr_baton & co), so that it can be retrieved when the time
> comes to evaluate the expressions.  However, it quickly became obvious
> that storing it there would be redundant and wasteful.
>
> I instead opted to store this information directly inside dwarf2_per_cu,
> making it easily available when evaluating expressions.  These fields
> are initially have the value 0, and are set by cutu_reader whenever the
> unit is parsed.  The various getters (dwarf2_per_cu::addr_size & al) now
> just return these fields.
>
> Doing so allows removing anything related to reading the header from
> dwarf2_per_cu, which I think is a nice simplification.  This means that
> nothing ever needs to read the header from just a dwarf2_per_cu.

Following on from my feedback on the previous patch.  I see why the
assert is not needed now :)  the function is going to be deleted.

>
> It also happens to shrink the dwarf2_per_cu object size a bit, going
> from:
>
>     (top-gdb) p sizeof(dwarf2_per_cu)
>     $1 = 176
>
> to
>
>     (top-gdb) p sizeof(dwarf2_per_cu)
>     $1 = 120
>
> I placed the new fields at this strange location in dwarf2_per_cu
> because there happened to be a nice 3 bytes hole there (on Linux amd64
> at least).
>
> The new test set things up as described previously.  Note that the crash
> only occurs if using the cc-with-gdb-index board.

As with the previous patch, I would prefer to see this case folded into
the default test mode, but this isn't a hard requirement, just
mentioning the correct board to use in the commit message does help a
lot when looking back through the commit history.

This fix looks good.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> Change-Id: I50807a1bbb605f0f92606a9e61c026e3376a4fcf
> ---
>  gdb/dwarf2/read.c                             |  64 ++-----
>  gdb/dwarf2/read.h                             |  56 ++++--
>  .../gdb.dwarf2/fission-type-unit-locexpr.c    |  25 +++
>  .../gdb.dwarf2/fission-type-unit-locexpr.exp  | 161 ++++++++++++++++++
>  4 files changed, 241 insertions(+), 65 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.c
>  create mode 100644 gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index df6e784025dc..a8b9798c4e8e 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -2832,6 +2832,14 @@ cutu_reader::read_cutu_die_from_dwo (dwarf2_cu *cu, dwo_unit *dwo_unit,
>        dwo_unit->length = cu->header.get_length_with_initial ();
>      }
>  
> +  /* Record some information found in the header.  This will be needed when
> +     evaluating DWARF expressions in the context of this unit, for instance.  */
> +  per_cu->set_addr_size (cu->header.addr_size);
> +  per_cu->set_offset_size (cu->header.offset_size);
> +  per_cu->set_ref_addr_size (cu->header.version == 2
> +			     ? cu->header.addr_size
> +			     : cu->header.offset_size);
> +
>    dwo_abbrev_section->read (objfile);
>    m_dwo_abbrev_table
>      = abbrev_table::read (dwo_abbrev_section, cu->header.abbrev_sect_off);
> @@ -3075,6 +3083,15 @@ cutu_reader::cutu_reader (dwarf2_per_cu &this_cu,
>  	  gdb_assert (this_cu.sect_off () == cu->header.sect_off);
>  	  this_cu.set_length (cu->header.get_length_with_initial ());
>  	}
> +
> +	/* Record some information found in the header.  This will be needed
> +	   when evaluating DWARF expressions in the context of this unit, for
> +	   instance.  */
> +	this_cu.set_addr_size (cu->header.addr_size);
> +	this_cu.set_offset_size (cu->header.offset_size);
> +	this_cu.set_ref_addr_size (cu->header.version == 2
> +				   ? cu->header.addr_size
> +				   : cu->header.offset_size);
>      }
>  
>    /* Skip dummy compilation units.  */
> @@ -18520,53 +18537,6 @@ dwarf2_symbol_mark_computed (const struct attribute *attr, struct symbol *sym,
>  
>  /* See read.h.  */
>  
> -const unit_head *
> -dwarf2_per_cu::get_header () const
> -{
> -  if (!m_header_read_in)
> -    {
> -      const gdb_byte *info_ptr
> -	= this->section ()->buffer + to_underlying (this->sect_off ());
> -
> -      read_unit_head (&m_header, info_ptr, this->section (), ruh_kind::COMPILE);
> -
> -      m_header_read_in = true;
> -    }
> -
> -  return &m_header;
> -}
> -
> -/* See read.h.  */
> -
> -int
> -dwarf2_per_cu::addr_size () const
> -{
> -  return this->get_header ()->addr_size;
> -}
> -
> -/* See read.h.  */
> -
> -int
> -dwarf2_per_cu::offset_size () const
> -{
> -  return this->get_header ()->offset_size;
> -}
> -
> -/* See read.h.  */
> -
> -int
> -dwarf2_per_cu::ref_addr_size () const
> -{
> -  const unit_head *header = this->get_header ();
> -
> -  if (header->version == 2)
> -    return header->addr_size;
> -  else
> -    return header->offset_size;
> -}
> -
> -/* See read.h.  */
> -
>  void
>  dwarf2_per_cu::set_lang (enum language lang, dwarf_source_language dw_lang)
>  {
> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
> index 95f00e617c7a..c09536e73412 100644
> --- a/gdb/dwarf2/read.h
> +++ b/gdb/dwarf2/read.h
> @@ -126,7 +126,6 @@ struct dwarf2_per_cu
>        tu_read (false),
>        lto_artificial (false),
>        queued (false),
> -      m_header_read_in (false),
>        files_read (false),
>        scanned (false),
>        m_section (section),
> @@ -189,11 +188,6 @@ struct dwarf2_per_cu
>       any of the current compilation units are processed.  */
>    packed<bool, 1> queued;
>  
> -private:
> -  /* True if HEADER has been read in.  */
> -  mutable packed<bool, 1> m_header_read_in;
> -
> -public:
>    /* True if we've tried to read the file table.  There will be no
>       point in trying to read it again next time.  */
>    packed<bool, 1> files_read;
> @@ -219,6 +213,14 @@ struct dwarf2_per_cu
>       not.  */
>    std::atomic<bool> scanned;
>  
> +private:
> +  /* Sizes for an address, an offset, and a section offset.  These fields are
> +     set by cutu_reader when the unit is read.  */
> +  unsigned char m_addr_size = 0;
> +  unsigned char m_offset_size = 0;
> +  unsigned char m_ref_addr_size = 0;
> +
> +public:
>    /* Our index in the unshared "symtabs" vector.  */
>    unsigned index = 0;
>  
> @@ -231,12 +233,6 @@ struct dwarf2_per_cu
>    /* Backlink to the owner of this.  */
>    dwarf2_per_bfd *m_per_bfd;
>  
> -  /* DWARF header of this unit.  Note that dwarf2_cu reads its own version of
> -     the header, which may differ from this one, since it may pass
> -     rch_kind::TYPE to read_unit_head, whereas for dwarf2_per_cu we always pass
> -     ruh_kind::COMPILE.  */
> -  mutable unit_head m_header;
> -
>  public:
>    /* The file and directory for this CU.  This is cached so that we
>       don't need to re-examine the DWO in some situations.  This may be
> @@ -281,20 +277,44 @@ struct dwarf2_per_cu
>    bool is_dwz () const
>    { return m_is_dwz; }
>  
> -  /* Get the header of this per_cu, reading it if necessary.  */
> -  const unit_head *get_header () const;
> -
>    /* Return the address size given in the compilation unit header for
>       this CU.  */
> -  int addr_size () const;
> +  int addr_size () const
> +  {
> +    gdb_assert (m_addr_size != 0);
> +    return m_addr_size;
> +  }
> +
> +  /* Set the address size given in the compilation unit header for
> +     this CU.  */
> +  void set_addr_size (int addr_size)
> +  { m_addr_size = addr_size; }
>  
>    /* Return the offset size given in the compilation unit header for
>       this CU.  */
> -  int offset_size () const;
> +  int offset_size () const
> +  {
> +    gdb_assert (m_offset_size != 0);
> +    return m_offset_size;
> +  }
> +
> +  /* Set the offset size given in the compilation unit header for
> +     this CU.  */
> +  void set_offset_size (int offset_size)
> +  {  m_offset_size = offset_size; }
>  
>    /* Return the DW_FORM_ref_addr size given in the compilation unit
>       header for this CU.  */
> -  int ref_addr_size () const;
> +  int ref_addr_size () const
> +  {
> +    gdb_assert (m_ref_addr_size != 0);
> +    return m_ref_addr_size;
> +  }
> +
> +  /* Return the DW_FORM_ref_addr size given in the compilation unit
> +     header for this CU.  */
> +  void set_ref_addr_size (int ref_addr_size)
> +  { m_ref_addr_size = ref_addr_size; }
>  
>    /* Return length of this CU.  */
>    unsigned int length () const
> diff --git a/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.c b/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.c
> new file mode 100644
> index 000000000000..1a84f5096334
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.c
> @@ -0,0 +1,25 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2025 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +unsigned char buf[] = { 0x11, 0x22, 0x22, 0x11, };
> +
> +int
> +main (void)
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp b/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp
> new file mode 100644
> index 000000000000..43bbe8f48436
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/fission-type-unit-locexpr.exp
> @@ -0,0 +1,161 @@
> +# Copyright 2025 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This test is meant to reproduce a bug occurring in those specific
> +# circumstances:
> +#
> +#  - Split DWARF (debug info in a .dwo file)
> +#  - A type unit in the .dwo file, with a corresponding stub in the main file
> +#  - A GDB index
> +#  - We evaluate a DWARF expression in the context of that type unit
> +#
> +# The test doesn't create a GDB index, but that can be done by running it
> +# with the cc-with-gdb-index board.
> +
> +load_lib dwarf.exp
> +
> +require dwarf2_support
> +
> +standard_testfile .c -dw.S
> +
> +set asm_file [standard_output_file $srcfile2]
> +
> +Dwarf::assemble $asm_file {
> +    set outdir [standard_output_file ""]
> +    set cu_debug_addr_label "invalid"
> +
> +    # Debug info in the DWO file.
> +
> +    # Definition of "int".
> +    tu {
> +	fission 1
> +	version 4
> +    } 0xCAFE1 the_type_1 {
> +	type_unit {} {
> +	    the_type_1: base_type {
> +		DW_AT_byte_size 4 DW_FORM_sdata
> +		DW_AT_encoding  @DW_ATE_signed
> +		DW_AT_name      int
> +	    }
> +	}
> +    }
> +
> +    # Definition of structure type "struct_type".
> +    tu {
> +	fission 1
> +	version 4
> +    } 0xCAFE2 the_type_2 {
> +	type_unit {} {
> +	    the_type_2: DW_TAG_structure_type {
> +		DW_AT_name struct_type
> +		DW_AT_byte_size 4 DW_FORM_data1
> +	    } {
> +		DW_TAG_member {
> +			DW_AT_name field
> +			DW_AT_type 0xCAFE1 DW_FORM_ref_sig8
> +
> +			# This is the expression that is going to be evaluated
> +			# by printing the variable.
> +			DW_AT_data_member_location {
> +			    # In order to coax GDB to actually run this
> +			    # expression in the evaluator, it needs to contain
> +			    # something that will make decode_locdesc unable
> +			    # to reduce it to a constant.  That's what these 3
> +			    # operations (which are ultimately a no-op) are for.
> +			    # Note that there is an implicit
> +			    # DW_OP_push_object_address done before the
> +			    # expression is evaluated.
> +			    DW_OP_dup
> +			    DW_OP_deref_size 1
> +			    DW_OP_drop
> +			} SPECIAL_expr
> +		}
> +	    }
> +	}
> +    }
> +
> +    # Definition of the CU.
> +    cu {
> +	fission 1
> +	version 4
> +    } {
> +	set cu_debug_addr_label [debug_addr_label]
> +
> +	compile_unit {
> +	    DW_AT_GNU_dwo_id 0xF00D DW_FORM_data8
> +	} {
> +	    DW_TAG_variable {
> +		DW_AT_name global_var
> +		DW_AT_type 0xCAFE2 DW_FORM_ref_sig8
> +		DW_AT_location {
> +		    DW_OP_GNU_addr_index [gdb_target_symbol buf]
> +		} SPECIAL_expr
> +	    }
> +	}
> +    }
> +
> +    # Debug info in the main file.
> +
> +    # Stub for "int" type unit.
> +    tu {
> +	version 4
> +    } 0xCAFE1 "" {
> +	type_unit {
> +	    DW_AT_GNU_dwo_name ${::gdb_test_file_name}.dwo DW_FORM_strp
> +	    DW_AT_comp_dir ${outdir}
> +	} {
> +	}
> +    }
> +
> +    # Stub for "struct_type" type unit.
> +    tu {
> +	version 4
> +    } 0xCAFE2 "" {
> +	type_unit {
> +	    DW_AT_GNU_dwo_name ${::gdb_test_file_name}.dwo DW_FORM_strp
> +	    DW_AT_comp_dir ${outdir}
> +	} {
> +	}
> +    }
> +
> +    # Stub for the CU.
> +    cu {
> +	version 4
> +    } {
> +	compile_unit {
> +	    DW_AT_GNU_dwo_name ${::gdb_test_file_name}.dwo DW_FORM_strp
> +	    DW_AT_comp_dir ${outdir}
> +	    DW_AT_GNU_dwo_id 0xF00D DW_FORM_data8
> +	    DW_AT_GNU_addr_base $cu_debug_addr_label
> +	} {}
> +    }
> +}
> +
> +set object_file [standard_output_file ${testfile}.o]
> +if { [build_executable_and_dwo_files "${testfile}.exp" ${binfile} {nodebug} \
> +	  [list $asm_file {nodebug split-dwo} ${object_file}] \
> +	  [list $srcfile {nodebug}]] } {
> +    return -1
> +}
> +
> +if { [is_remote host] } {
> +    gdb_remote_download host $dwo_file
> +}
> +
> +clean_restart ${testfile}
> +
> +# This print would cause the DW_AT_data_member_location expression to be
> +# evaluated and cause the crash.
> +gdb_test "print/x global_var" " = {field = 0x11222211}"
> -- 
> 2.51.2

Re: [PATCH 6/6] gdb/dwarf: use dwarf2_per_cu::ref_addr_size in one spot

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> I noticed this spot that could use dwarf2_per_cu::ref_addr_size, instead
> of implementing the same logic.

LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> Change-Id: I894ef1dcaa687c659e49bc1eb433e42818038cf2
> ---
>  gdb/dwarf2/read.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index a8b9798c4e8e..c355cc2bb5cb 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -3866,12 +3866,7 @@ cutu_reader::skip_one_attribute (dwarf_form form, const gdb_byte *info_ptr)
>    switch (form)
>      {
>      case DW_FORM_ref_addr:
> -      /* In DWARF 2, DW_FORM_ref_addr is address sized; in DWARF 3
> -	       and later it is offset sized.  */
> -      if (m_cu->header.version == 2)
> -	return info_ptr + m_cu->header.addr_size;
> -      else
> -	return info_ptr + m_cu->header.offset_size;
> +      return info_ptr + m_cu->per_cu->ref_addr_size ();
>  
>      case DW_FORM_GNU_ref_alt:
>        return info_ptr + m_cu->header.offset_size;
> -- 
> 2.51.2

Re: [PATCH 2/6] gdb/testsuite/dwarf: convert _section proc to use parse_options

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

I meant to send this last week but forgot.

Simon> -    proc _section {name {flags ""} {type ""}} {
Simon> -	if {$name == ".debug_str"} {
Simon> -	    # Hard-code this because it's always desirable.
Simon> -	    _emit "        .section $name, \"MS\", %progbits, 1"
Simon> -	} elseif {$flags == "" && $type == ""} {
Simon> -	    _emit "        .section $name"
Simon> -	} elseif {$type == ""} {
Simon> -	    _emit "        .section $name, \"$flags\""
Simon> -	} else {
Simon> -	    _emit "        .section $name, \"$flags\", %$type"

This code ignores flags and type if name is .debug_str, but the
replacement code:

Simon> +	if { $name == ".debug_str" } {
Simon> +	    # Hard-code this because it's always desirable.
Simon> +	    append directive ", \"MS\", %progbits, 1"
Simon> +	}
Simon> +
Simon> +	if { $flags != "" } {
Simon> +	    append directive ",\"$flags\""
Simon> +	}
Simon> +
Simon> +	if { $type != "" } {
Simon> +	    # Can't specify a type without flags.
Simon> + 	    if { $flags == "" } {
Simon> +		append directive ", \"\""
Simon> +	    }
Simon> +
Simon> +	    append directive ", %$type"
Simon> +	}

... does not.

I think the old code was clearer in this regard.  I didn't read forward
in the series to see if this restructuring helps newer code though.
Anyway this change should be addressed.

Tom

Re: [PATCH 1/6] gdb/testsuite/dwarf: use single abbrev table in .dwo files

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> # Must be run on local host due to use of objcopy.
>> if {[is_remote host]} {
>> return -1
>> }

Simon> I don't understand why that wouldn't be possible if debugging on a
Simon> remote host.  Here, we are building target objects, we have a toolchain
Simon> for the target, so we should have an objcopy for the target that we can
Simon> run.  It should be possible, in theory.  Am I missing something?

Most likely just nobody ever wanted to try it.

Simon> Since build_executable_and_dwo_files returns -1 if the host is remote,
Simon> causing the test to exit early an cleanly (although we could perhaps add
Simon> an untested call somewhere), do we really need the require at the top?

No opinion on that but I do agree that build_executable_and_dwo_files is
missing some 'untested' invocations -- some "return -1" paths call it
and some do not.

Tom

Re: [PATCH 4/6] gdb/dwarf: when in dwarf2_cu, read addr_size from dwarf2_cu::header

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> I would prefer to not reintroduce the behavior of overwriting the
Simon> section info in dwarf2_per_cu, because:

Simon>  1. I find it confusing, I like the invariant of dwarf2_per_cu::section
Simon>     points to the stub, and dwarf2_cu::section points to where we
Simon>     actually read the debug info from.

The "overwrite" behavior was super messy and hard to untangle, so +1 to
getting rid of that.

I was wondering if the new invariant is sufficiently documented.  I
somewhat think the comments in dwarf2_per_cu should be updated to
explain it.  Also while dwarf2_cu::section has a comment, I'm left
wondering about dwarf2_cu::header.

Tom

Re: [PATCH 5/6] gdb/dwarf: store addr/offset/ref_addr sizes in dwarf2_per_cu

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> +  /* Sizes for an address, an offset, and a section offset.  These fields are
Simon> +     set by cutu_reader when the unit is read.  */
Simon> +  unsigned char m_addr_size = 0;
Simon> +  unsigned char m_offset_size = 0;
Simon> +  unsigned char m_ref_addr_size = 0;

How about uint8_t instead.

Tom

Re: [PATCH 1/6] gdb/testsuite/dwarf: use single abbrev table in .dwo files

[Simon Marchi]

On 2025-11-19 11:05, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>>> # Must be run on local host due to use of objcopy.
>>> if {[is_remote host]} {
>>> return -1
>>> }
> 
> Simon> I don't understand why that wouldn't be possible if debugging on a
> Simon> remote host.  Here, we are building target objects, we have a toolchain
> Simon> for the target, so we should have an objcopy for the target that we can
> Simon> run.  It should be possible, in theory.  Am I missing something?
> 
> Most likely just nobody ever wanted to try it.
> 
> Simon> Since build_executable_and_dwo_files returns -1 if the host is remote,
> Simon> causing the test to exit early an cleanly (although we could perhaps add
> Simon> an untested call somewhere), do we really need the require at the top?
> 
> No opinion on that but I do agree that build_executable_and_dwo_files is
> missing some 'untested' invocations -- some "return -1" paths call it
> and some do not.

Ok, so I'll make a patch to add untested calls to these code paths.  And
I'll remove the gdb_remote_download call from the test: if someone wants
to make build_executable_and_dwo_files work with remote hosts, they will
have to modify all tests using build_executable_and_dwo_files anyway
(or, build_executable_and_dwo_files will do the gdb_remote_download, in
which case individual tests won't need to).

Simon

Re: [PATCH 2/6] gdb/testsuite/dwarf: convert _section proc to use parse_options

[Simon Marchi]

On 2025-11-19 10:53, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> I meant to send this last week but forgot.
> 
> Simon> -    proc _section {name {flags ""} {type ""}} {
> Simon> -	if {$name == ".debug_str"} {
> Simon> -	    # Hard-code this because it's always desirable.
> Simon> -	    _emit "        .section $name, \"MS\", %progbits, 1"
> Simon> -	} elseif {$flags == "" && $type == ""} {
> Simon> -	    _emit "        .section $name"
> Simon> -	} elseif {$type == ""} {
> Simon> -	    _emit "        .section $name, \"$flags\""
> Simon> -	} else {
> Simon> -	    _emit "        .section $name, \"$flags\", %$type"
> 
> This code ignores flags and type if name is .debug_str, but the
> replacement code:
> 
> Simon> +	if { $name == ".debug_str" } {
> Simon> +	    # Hard-code this because it's always desirable.
> Simon> +	    append directive ", \"MS\", %progbits, 1"
> Simon> +	}
> Simon> +
> Simon> +	if { $flags != "" } {
> Simon> +	    append directive ",\"$flags\""
> Simon> +	}
> Simon> +
> Simon> +	if { $type != "" } {
> Simon> +	    # Can't specify a type without flags.
> Simon> + 	    if { $flags == "" } {
> Simon> +		append directive ", \"\""
> Simon> +	    }
> Simon> +
> Simon> +	    append directive ", %$type"
> Simon> +	}
> 
> ... does not.
> 
> I think the old code was clearer in this regard.  I didn't read forward
> in the series to see if this restructuring helps newer code though.
> Anyway this change should be addressed.

It's true that if one was to call the proc with $name == ".debug_str"
and some flags, the resulting assembler directive would be malformed.  I
can change it to

	if { $name == ".debug_str" } {
	    # Hard-code this because it's always desirable.
	    _emit "        .section .debug_str, \"MS\", %progbits, 1"
	    return
	}

I think that would make it clear again that this is a special case,
disconnected from the rest.

Or something cleaner would be to remove this bit from this proc, and
add a _section_debug_str proc that would call _section with the right
params (might need a _defer_output_debug_str as well).

Simon

Re: [PATCH 4/6] gdb/dwarf: when in dwarf2_cu, read addr_size from dwarf2_cu::header

[Simon Marchi]

On 2025-11-19 09:30, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
> 
>> This patch fixes a crash caused by GDB trying to read from a section not
>> read in.  The bug happens in those specific circumstances:
>>
>>  - reading a type unit from .dwo
>>  - that type unit has a stub in the main file
>>  - there is a GDB index (.gdb_index) present
>>
>> This crash is the cause of the following test failure:
>>
>>     $ Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/fission-reread.exp ...
>>     ERROR: GDB process no longer exists
> 
> It's probably worth mentioning that this failure only occurs when using
> the cc-with-gdb-index board.

Fixed.

> I would encourage you to consider extending the existing test to cover
> this case without needing to test with the cc-with-gdb-index board.  I
> think the reality is most people don't test every patch with every
> board.  But I do understand that, if taken to extreme, that would mean
> running every test in every mode, which clearly doesn't scale.  But when
> we run into a regression my instinct is to ensure that the default test
> mode covers this case.  And there's plenty of precedent for this in the
> testsuite, e.g. gdb.server/*.exp.  You're welcome to take the test
> changes from the patch linked above if that is useful.

Good idea, I'll check that.

> Also in the patch linked above is the addition of an assert:
> 
>   gdb_assert (this->section ()->readin);
> 
> in 'dwarf2_per_cu::get_header ()'.  I haven't checked the later patches
> from this series yet, so maybe you already do something similar, but if
> not, maybe it would be good to add something like that in this patch,
> given that this is dealing with the assert caused by the section not
> being read in?

Haven't checked the review comments on that patch yet, but the following
patch removes dwarf2_per_cu::get_header.

Thanks,

Simon

Re: [PATCH 4/6] gdb/dwarf: when in dwarf2_cu, read addr_size from dwarf2_cu::header

[Simon Marchi]

On 2025-11-19 11:11, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> I would prefer to not reintroduce the behavior of overwriting the
> Simon> section info in dwarf2_per_cu, because:
> 
> Simon>  1. I find it confusing, I like the invariant of dwarf2_per_cu::section
> Simon>     points to the stub, and dwarf2_cu::section points to where we
> Simon>     actually read the debug info from.
> 
> The "overwrite" behavior was super messy and hard to untangle, so +1 to
> getting rid of that.
> 
> I was wondering if the new invariant is sufficiently documented.  I
> somewhat think the comments in dwarf2_per_cu should be updated to
> explain it.  Also while dwarf2_cu::section has a comment, I'm left
> wondering about dwarf2_cu::header.

I have a subsequent patch that updates the comments in dwarf2_per_cu for
the section, offset and length fields.  You can get a preview here:

https://review.lttng.org/c/binutils-gdb/+/15628

I could improve the comment of dwarf2_cu::header to say that it always
comes from where we really read the DIEs from (the .dwo file in case of
split DWARF), if that's what you mean.  I would need to check that this
is really true.

Simon

Re: [PATCH 2/6] gdb/testsuite/dwarf: convert _section proc to use parse_options

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> It's true that if one was to call the proc with $name == ".debug_str"
Simon> and some flags, the resulting assembler directive would be malformed.  I
Simon> can change it to

Simon> 	if { $name == ".debug_str" } {
Simon> 	    # Hard-code this because it's always desirable.
Simon> 	    _emit "        .section .debug_str, \"MS\", %progbits, 1"
Simon> 	    return
Simon> 	}

Simon> I think that would make it clear again that this is a special case,
Simon> disconnected from the rest.

Seems fine to me.  Thanks.

Tom

Re: [PATCH 5/6] gdb/dwarf: store addr/offset/ref_addr sizes in dwarf2_per_cu

[Simon Marchi]

On 2025-11-19 11:14, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> +  /* Sizes for an address, an offset, and a section offset.  These fields are
> Simon> +     set by cutu_reader when the unit is read.  */
> Simon> +  unsigned char m_addr_size = 0;
> Simon> +  unsigned char m_offset_size = 0;
> Simon> +  unsigned char m_ref_addr_size = 0;
> 
> How about uint8_t instead.

I don't mind doing that change, but why do you think it's better than
"unsigned char" here?

Simon

Re: [PATCH 5/6] gdb/dwarf: store addr/offset/ref_addr sizes in dwarf2_per_cu

[Tom Tromey]

>> How about uint8_t instead.

Simon> I don't mind doing that change, but why do you think it's better than
Simon> "unsigned char" here?

A char is conceptually a character.  In the olden days we didn't have an
8-bit integer type, but now we do.

Tom