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

[Andrew Burgess <aburgess@redhat.com>]

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