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

Andrew Burgess
2025-11-19T14:43:04Z

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