[v2,gdb/symtab] Handle DW_OP_entry_value at function entry

  On riscv64-linux, with test-case gdb.base/vla-optimized-out.exp I ran into:
...
(gdb) p sizeof (a)^M
$2 = <optimized out>^M
(gdb) FAIL: $exp: o1: printed size of optimized out vla
...

The variable a has type 0xbf:
...
 <1><bf>: Abbrev Number: 12 (DW_TAG_array_type)
    <c0>   DW_AT_type        : <0xe3>
    <c4>   DW_AT_sibling     : <0xdc>
 <2><c8>: Abbrev Number: 13 (DW_TAG_subrange_type)
    <c9>   DW_AT_type        : <0xdc>
    <cd>   DW_AT_upper_bound : 13 byte block:
                               a3 1 5a 23 1 8 20 24 8 20 26 31 1c
			       (DW_OP_entry_value: (DW_OP_reg10 (a0));
			        DW_OP_plus_uconst: 1; DW_OP_const1u: 32;
				DW_OP_shl; DW_OP_const1u: 32; DW_OP_shra;
				DW_OP_lit1; DW_OP_minus)
...
which has an upper bound using an DW_OP_entry_value, and since the
corresponding call site contains no information to resolve the value of a0 at
function entry:
...
 <2><6b>: Abbrev Number: 6 (DW_TAG_call_site)
    <6c>   DW_AT_call_return_pc: 0x638
    <74>   DW_AT_call_origin : <0x85>
...
evaluting the dwarf expression fails, and we get <optimized out>.

My first thought was to try breaking at *f1 instead f1 to see if that would
help, but actually the breakpoint resolved to the same address.

In other words, the inferior is stopped at function entry.

Fix this by resolving DW_OP_entry_value when stopped at function entry by
simply evaluating the expression.

This handles these two cases (x86_64, using reg rdi):
- DW_OP_entry_value: (DW_OP_regx: 5 (rdi))
- DW_OP_entry_value: (DW_OP_bregx: 5 (rdi) 0; DW_OP_deref_size: 4)

Tested on x86_64-linux.

Tested gdb.base/vla-optimized-out.exp on riscv64-linux.

Tested an earlier version of gdb.dwarf2/dw2-entry-value-2.exp on
riscv64-linux, but atm I'm running into trouble on that machine (cfarm92) so
I haven't tested the current version there.
---
 gdb/dwarf2/expr.c                             |  69 +++++++---
 gdb/dwarf2/expr.h                             |   4 +
 gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c  |  32 +++++
 .../gdb.dwarf2/dw2-entry-value-2.exp          | 125 ++++++++++++++++++
 gdb/testsuite/lib/dwarf.exp                   |  23 +++-
 5 files changed, 235 insertions(+), 18 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.exp


base-commit: 48984d3da79f340b814e43b6576993ea1a927f5a
  

On 1/17/25 16:12, Tom de Vries wrote:
> On riscv64-linux, with test-case gdb.base/vla-optimized-out.exp I ran into:
> ...
> (gdb) p sizeof (a)^M
> $2 = <optimized out>^M
> (gdb) FAIL: $exp: o1: printed size of optimized out vla
> ...
> 
> The variable a has type 0xbf:
> ...
>   <1><bf>: Abbrev Number: 12 (DW_TAG_array_type)
>      <c0>   DW_AT_type        : <0xe3>
>      <c4>   DW_AT_sibling     : <0xdc>
>   <2><c8>: Abbrev Number: 13 (DW_TAG_subrange_type)
>      <c9>   DW_AT_type        : <0xdc>
>      <cd>   DW_AT_upper_bound : 13 byte block:
>                                 a3 1 5a 23 1 8 20 24 8 20 26 31 1c
> 			       (DW_OP_entry_value: (DW_OP_reg10 (a0));
> 			        DW_OP_plus_uconst: 1; DW_OP_const1u: 32;
> 				DW_OP_shl; DW_OP_const1u: 32; DW_OP_shra;
> 				DW_OP_lit1; DW_OP_minus)
> ...
> which has an upper bound using an DW_OP_entry_value, and since the
> corresponding call site contains no information to resolve the value of a0 at
> function entry:
> ...
>   <2><6b>: Abbrev Number: 6 (DW_TAG_call_site)
>      <6c>   DW_AT_call_return_pc: 0x638
>      <74>   DW_AT_call_origin : <0x85>
> ...
> evaluting the dwarf expression fails, and we get <optimized out>.
> 
> My first thought was to try breaking at *f1 instead f1 to see if that would
> help, but actually the breakpoint resolved to the same address.
> 
> In other words, the inferior is stopped at function entry.
> 
> Fix this by resolving DW_OP_entry_value when stopped at function entry by
> simply evaluating the expression.
> 
> This handles these two cases (x86_64, using reg rdi):
> - DW_OP_entry_value: (DW_OP_regx: 5 (rdi))
> - DW_OP_entry_value: (DW_OP_bregx: 5 (rdi) 0; DW_OP_deref_size: 4)
> 

Ping.

Thanks,
- Tom

> Tested on x86_64-linux.
> 
> Tested gdb.base/vla-optimized-out.exp on riscv64-linux.
> 
> Tested an earlier version of gdb.dwarf2/dw2-entry-value-2.exp on
> riscv64-linux, but atm I'm running into trouble on that machine (cfarm92) so
> I haven't tested the current version there.
> ---
>   gdb/dwarf2/expr.c                             |  69 +++++++---
>   gdb/dwarf2/expr.h                             |   4 +
>   gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c  |  32 +++++
>   .../gdb.dwarf2/dw2-entry-value-2.exp          | 125 ++++++++++++++++++
>   gdb/testsuite/lib/dwarf.exp                   |  23 +++-
>   5 files changed, 235 insertions(+), 18 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c
>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.exp
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index ee1522b7437..695b30e71b4 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -881,6 +881,34 @@ dwarf_expr_context::read_mem (gdb_byte *buf, CORE_ADDR addr,
>   
>   /* See expr.h.  */
>   
> +value *
> +dwarf_expr_context::deref (CORE_ADDR addr, int size, struct type *type)
> +{
> +  gdb_byte *buf = (gdb_byte *) alloca (size);
> +  this->read_mem (buf, addr, size);
> +
> +  if (type == nullptr)
> +    type = this->address_type ();
> +
> +  /* If the size of the object read from memory is different
> +     from the type length, we need to zero-extend it.  */
> +  if (type->length () != size)
> +    {
> +      gdbarch *arch = this->m_per_objfile->objfile->arch ();
> +      bfd_endian byte_order = gdbarch_byte_order (arch);
> +      ULONGEST datum
> +	= extract_unsigned_integer (buf, size, byte_order);
> +
> +      buf = (gdb_byte *) alloca (type->length ());
> +      store_unsigned_integer (buf, type->length (),
> +			      byte_order, datum);
> +    }
> +
> +  return value_from_contents_and_address (type, buf, addr);
> +}
> +
> +/* See expr.h.  */
> +
>   void
>   dwarf_expr_context::push_dwarf_reg_entry_value (call_site_parameter_kind kind,
>   						call_site_parameter_u kind_u,
> @@ -1916,7 +1944,6 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>   	case DW_OP_GNU_deref_type:
>   	  {
>   	    int addr_size = (op == DW_OP_deref ? this->m_addr_size : *op_ptr++);
> -	    gdb_byte *buf = (gdb_byte *) alloca (addr_size);
>   	    CORE_ADDR addr = fetch_address (0);
>   	    struct type *type;
>   
> @@ -1931,21 +1958,7 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>   	    else
>   	      type = address_type;
>   
> -	    this->read_mem (buf, addr, addr_size);
> -
> -	    /* If the size of the object read from memory is different
> -	       from the type length, we need to zero-extend it.  */
> -	    if (type->length () != addr_size)
> -	      {
> -		ULONGEST datum =
> -		  extract_unsigned_integer (buf, addr_size, byte_order);
> -
> -		buf = (gdb_byte *) alloca (type->length ());
> -		store_unsigned_integer (buf, type->length (),
> -					byte_order, datum);
> -	      }
> -
> -	    result_val = value_from_contents_and_address (type, buf, addr);
> +	    result_val = this->deref (addr, addr_size, type);
>   	    break;
>   	  }
>   
> @@ -2274,6 +2287,18 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>   	    if (kind_u.dwarf_reg != -1)
>   	      {
>   		op_ptr += len;
> +
> +		if (m_frame == get_current_frame ()
> +		    && find_function_type (get_frame_pc (m_frame)) != nullptr)
> +		  {
> +		    /* We're stopped at the start of the function.
> +		       Handle as DW_OP_regx.  */
> +		    result_val
> +		      = value_from_ulongest (address_type, kind_u.dwarf_reg);
> +		    this->m_location = DWARF_VALUE_REGISTER;
> +		    break;
> +		  }
> +
>   		this->push_dwarf_reg_entry_value (CALL_SITE_PARAMETER_DWARF_REG,
>   						  kind_u,
>   						  -1 /* deref_size */);
> @@ -2288,6 +2313,18 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>   		if (deref_size == -1)
>   		  deref_size = this->m_addr_size;
>   		op_ptr += len;
> +
> +		if (m_frame == get_current_frame ()
> +		    && find_function_type (get_frame_pc (m_frame)) != nullptr)
> +		  {
> +		    /* We're stopped at the start of the function.
> +		       Handle as DW_OP_bregx;DW_OP_deref_size.  */
> +		    CORE_ADDR addr
> +		      = read_addr_from_reg (this->m_frame, kind_u.dwarf_reg);
> +		    result_val = this->deref (addr, deref_size);
> +		    break;
> +		  }
> +
>   		this->push_dwarf_reg_entry_value (CALL_SITE_PARAMETER_DWARF_REG,
>   						  kind_u, deref_size);
>   		goto no_push;
> diff --git a/gdb/dwarf2/expr.h b/gdb/dwarf2/expr.h
> index 1b1653e0768..58ae5ccd8d4 100644
> --- a/gdb/dwarf2/expr.h
> +++ b/gdb/dwarf2/expr.h
> @@ -256,6 +256,10 @@ struct dwarf_expr_context
>        but with the address being 0.  In this situation, we arrange for
>        memory reads to come from the passed-in buffer.  */
>     void read_mem (gdb_byte *buf, CORE_ADDR addr, size_t length);
> +
> +  /* Deref ADDR with size SIZE and return a value of type TYPE.
> +     If TYPE == nullptr, defaults to this->address_type ().  */
> +  value *deref (CORE_ADDR addr, int size, struct type *type = nullptr);
>   };
>   
>   /* Return the value of register number REG (a DWARF register number),
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c b/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c
> new file mode 100644
> index 00000000000..57025e0bc73
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c
> @@ -0,0 +1,32 @@
> +/* 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/>.  */
> +
> +int var = 2;
> +
> +static
> +void bar (int *p)
> +{
> +  asm ("bar_label: .globl bar_label");
> +}
> +
> +int
> +main()
> +{
> +  asm ("main_label: .globl main_label");
> +  bar (&var);
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.exp b/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.exp
> new file mode 100644
> index 00000000000..55ecf9cece7
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.exp
> @@ -0,0 +1,125 @@
> +# 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/>.
> +
> +# Check that we can get DW_OP_entry_value at function entry.
> +
> +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 -debug.S
> +
> +set test "get dwarf regnum for first argument register"
> +if { [is_x86_64_m64_target] } {
> +    set dwarf_regnum 5
> +} elseif { [istarget riscv64*] } {
> +    set dwarf_regnum 10
> +} else {
> +    set reason "architecture-specific setting missing"
> +    unsupported "$test ($reason)"
> +    return
> +}
> +pass $test
> +
> +# Set up the DWARF for the test.
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile
> +
> +    get_func_info main
> +    get_func_info bar
> +
> +    cu {} {
> +	DW_TAG_compile_unit {
> +		{DW_AT_name $srcfile}
> +	} {
> +	    declare_labels integer
> +
> +	    integer: DW_TAG_base_type {
> +		{DW_AT_byte_size 8 DW_FORM_sdata}
> +		{DW_AT_encoding @DW_ATE_signed}
> +		{DW_AT_name integer}
> +	    }
> +
> +	    DW_TAG_subprogram {
> +		{ DW_AT_name main }
> +		{ DW_AT_low_pc $main_start DW_FORM_addr }
> +		{ DW_AT_high_pc $main_end DW_FORM_addr }
> +	    } {
> +		DW_TAG_variable {
> +		    { DW_AT_name argc }
> +		    { DW_AT_type :$integer }
> +		    { DW_AT_location {
> +			DW_OP_entry_value {
> +			    DW_OP_regx $::dwarf_regnum
> +			}
> +		    } SPECIAL_expr }
> +		}
> +	    }
> +
> +	    DW_TAG_subprogram {
> +		{ DW_AT_name bar }
> +		{ DW_AT_low_pc $bar_start DW_FORM_addr }
> +		{ DW_AT_high_pc $bar_end DW_FORM_addr }
> +	    } {
> +		DW_TAG_variable {
> +		    { DW_AT_name foo }
> +		    { DW_AT_type :$integer }
> +		    { DW_AT_location {
> +			DW_OP_entry_value {
> +			    DW_OP_bregx $::dwarf_regnum 0
> +			    DW_OP_deref_size 4
> +			}
> +			DW_OP_stack_value
> +		    } SPECIAL_expr }
> +		}
> +	    }
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" $testfile \
> +	  [list $srcfile $asm_file] {nodebug}] } {
> +    return -1
> +}
> +
> +if { ![runto *main] } {
> +    return
> +}
> +
> +with_test_prefix "at main+0" {
> +    gdb_test "p argc" " = 1"
> +
> +    gdb_test "stepi"
> +}
> +
> +with_test_prefix "at main+1" {
> +    gdb_test "p argc" " = <optimized out>"
> +}
> +
> +gdb_breakpoint "*bar"
> +gdb_continue_to_breakpoint "bar"
> +
> +with_test_prefix "at bar+0" {
> +    gdb_test "p foo" " = 2"
> +
> +    gdb_test "stepi"
> +}
> +
> +with_test_prefix "at bar+1" {
> +    gdb_test "p foo" " = <optimized out>"
> +}
> diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
> index f6348972968..81435ea4118 100644
> --- a/gdb/testsuite/lib/dwarf.exp
> +++ b/gdb/testsuite/lib/dwarf.exp
> @@ -1244,7 +1244,6 @@ namespace eval Dwarf {
>       # used, as indicated in the header of the section where the location
>       # description is found.
>       #
> -    # (FIXME should use 'info complete' here.)
>       # Each list's first element is the opcode, either short or long
>       # forms are accepted.
>       # FIXME argument handling
> @@ -1252,9 +1251,18 @@ namespace eval Dwarf {
>       proc _location { body dwarf_version addr_size offset_size } {
>   	variable _constants
>   
> +	set collected_lines ""
>   	foreach line [split $body \n] {
>   	    # Ignore blank lines, and allow embedded comments.
> -	    if {[lindex $line 0] == "" || [regexp -- {^[ \t]*#} $line]} {
> +	    if {  [regexp -- {^[ \t]*$} $line] || [regexp -- {^[ \t]*#} $line] } {
> +		continue
> +	    }
> +	    if { $collected_lines != "" } {
> +		set line "$collected_lines\n$line"
> +		set collected_lines ""
> +	    }
> +	    if { ! [info complete $line] } {
> +		set collected_lines $line
>   		continue
>   	    }
>   	    set opcode [_map_name [lindex $line 0] _OP]
> @@ -1340,6 +1348,17 @@ namespace eval Dwarf {
>   		    _op .2byte $argvec(label)
>   		}
>   
> +		DW_OP_entry_value {
> +		    _get_args $line $opcode body
> +		    set l1 [new_label "expr_start"]
> +		    set l2 [new_label "expr_end"]
> +		    _op .uleb128 "$l2 - $l1" "expression"
> +		    define_label $l1
> +		    _location $argvec(body) $dwarf_version $addr_size \
> +			$offset_size
> +		    define_label $l2
> +		}
> +
>   		DW_OP_implicit_value {
>   		    set l1 [new_label "value_start"]
>   		    set l2 [new_label "value_end"]
> 
> base-commit: 48984d3da79f340b814e43b6576993ea1a927f5a
  

Hey Tom,

This all looks reasonable to me, I did have one question, which is more
for my own education than anything ...

Tom de Vries <tdevries@suse.de> writes:

> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index ee1522b7437..695b30e71b4 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -881,6 +881,34 @@ dwarf_expr_context::read_mem (gdb_byte *buf, CORE_ADDR addr,
>  
>  /* See expr.h.  */
>  
> +value *
> +dwarf_expr_context::deref (CORE_ADDR addr, int size, struct type *type)
> +{
> +  gdb_byte *buf = (gdb_byte *) alloca (size);
> +  this->read_mem (buf, addr, size);
> +
> +  if (type == nullptr)
> +    type = this->address_type ();
> +
> +  /* If the size of the object read from memory is different
> +     from the type length, we need to zero-extend it.  */
> +  if (type->length () != size)
> +    {
> +      gdbarch *arch = this->m_per_objfile->objfile->arch ();
> +      bfd_endian byte_order = gdbarch_byte_order (arch);
> +      ULONGEST datum
> +	= extract_unsigned_integer (buf, size, byte_order);
> +
> +      buf = (gdb_byte *) alloca (type->length ());
> +      store_unsigned_integer (buf, type->length (),
> +			      byte_order, datum);
> +    }
> +
> +  return value_from_contents_and_address (type, buf, addr);
> +}
> +
> +/* See expr.h.  */
> +
>  void
>  dwarf_expr_context::push_dwarf_reg_entry_value (call_site_parameter_kind kind,
>  						call_site_parameter_u kind_u,
> @@ -1916,7 +1944,6 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>  	case DW_OP_GNU_deref_type:
>  	  {
>  	    int addr_size = (op == DW_OP_deref ? this->m_addr_size : *op_ptr++);
> -	    gdb_byte *buf = (gdb_byte *) alloca (addr_size);
>  	    CORE_ADDR addr = fetch_address (0);
>  	    struct type *type;
>  
> @@ -1931,21 +1958,7 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>  	    else
>  	      type = address_type;
>  
> -	    this->read_mem (buf, addr, addr_size);
> -
> -	    /* If the size of the object read from memory is different
> -	       from the type length, we need to zero-extend it.  */
> -	    if (type->length () != addr_size)
> -	      {
> -		ULONGEST datum =
> -		  extract_unsigned_integer (buf, addr_size, byte_order);
> -
> -		buf = (gdb_byte *) alloca (type->length ());
> -		store_unsigned_integer (buf, type->length (),
> -					byte_order, datum);
> -	      }
> -
> -	    result_val = value_from_contents_and_address (type, buf, addr);
> +	    result_val = this->deref (addr, addr_size, type);
>  	    break;
>  	  }
>  
> @@ -2274,6 +2287,18 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>  	    if (kind_u.dwarf_reg != -1)
>  	      {
>  		op_ptr += len;
> +
> +		if (m_frame == get_current_frame ()
> +		    && find_function_type (get_frame_pc (m_frame)) != nullptr)

Given that this construct is duplicated, and then you have a comment to
explain what the `if` condition is checking for, I wonder if moving this
check into a nicely named helper function would make things clearer?

But, my real question is, why the `m_frame == get_current_frame ()`
check?  I believe that `m_frame` is the frame in which the expression is
being evaluated, so why isn't it enough to check if we are at the very
start of m_frame?  Why does m_frame have to be the current frame?

I just couldn't figure this out.

Thanks,
Andrew
  

On 3/20/25 16:51, Andrew Burgess wrote:
> 
> Hey Tom,
> 
> This all looks reasonable to me, I did have one question, which is more
> for my own education than anything ...
> 
> Tom de Vries <tdevries@suse.de> writes:
> 
>> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
>> index ee1522b7437..695b30e71b4 100644
>> --- a/gdb/dwarf2/expr.c
>> +++ b/gdb/dwarf2/expr.c
>> @@ -881,6 +881,34 @@ dwarf_expr_context::read_mem (gdb_byte *buf, CORE_ADDR addr,
>>   
>>   /* See expr.h.  */
>>   
>> +value *
>> +dwarf_expr_context::deref (CORE_ADDR addr, int size, struct type *type)
>> +{
>> +  gdb_byte *buf = (gdb_byte *) alloca (size);
>> +  this->read_mem (buf, addr, size);
>> +
>> +  if (type == nullptr)
>> +    type = this->address_type ();
>> +
>> +  /* If the size of the object read from memory is different
>> +     from the type length, we need to zero-extend it.  */
>> +  if (type->length () != size)
>> +    {
>> +      gdbarch *arch = this->m_per_objfile->objfile->arch ();
>> +      bfd_endian byte_order = gdbarch_byte_order (arch);
>> +      ULONGEST datum
>> +	= extract_unsigned_integer (buf, size, byte_order);
>> +
>> +      buf = (gdb_byte *) alloca (type->length ());
>> +      store_unsigned_integer (buf, type->length (),
>> +			      byte_order, datum);
>> +    }
>> +
>> +  return value_from_contents_and_address (type, buf, addr);
>> +}
>> +
>> +/* See expr.h.  */
>> +
>>   void
>>   dwarf_expr_context::push_dwarf_reg_entry_value (call_site_parameter_kind kind,
>>   						call_site_parameter_u kind_u,
>> @@ -1916,7 +1944,6 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>>   	case DW_OP_GNU_deref_type:
>>   	  {
>>   	    int addr_size = (op == DW_OP_deref ? this->m_addr_size : *op_ptr++);
>> -	    gdb_byte *buf = (gdb_byte *) alloca (addr_size);
>>   	    CORE_ADDR addr = fetch_address (0);
>>   	    struct type *type;
>>   
>> @@ -1931,21 +1958,7 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>>   	    else
>>   	      type = address_type;
>>   
>> -	    this->read_mem (buf, addr, addr_size);
>> -
>> -	    /* If the size of the object read from memory is different
>> -	       from the type length, we need to zero-extend it.  */
>> -	    if (type->length () != addr_size)
>> -	      {
>> -		ULONGEST datum =
>> -		  extract_unsigned_integer (buf, addr_size, byte_order);
>> -
>> -		buf = (gdb_byte *) alloca (type->length ());
>> -		store_unsigned_integer (buf, type->length (),
>> -					byte_order, datum);
>> -	      }
>> -
>> -	    result_val = value_from_contents_and_address (type, buf, addr);
>> +	    result_val = this->deref (addr, addr_size, type);
>>   	    break;
>>   	  }
>>   
>> @@ -2274,6 +2287,18 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>>   	    if (kind_u.dwarf_reg != -1)
>>   	      {
>>   		op_ptr += len;
>> +
>> +		if (m_frame == get_current_frame ()
>> +		    && find_function_type (get_frame_pc (m_frame)) != nullptr)
> 
> Given that this construct is duplicated, and then you have a comment to
> explain what the `if` condition is checking for, I wonder if moving this
> check into a nicely named helper function would make things clearer?
> 

Hi Andrew,

thanks for the review.

> But, my real question is, why the `m_frame == get_current_frame ()`
> check?  I believe that `m_frame` is the frame in which the expression is
> being evaluated, so why isn't it enough to check if we are at the very
> start of m_frame?  Why does m_frame have to be the current frame?
> 
> I just couldn't figure this out.

If not, we run into:
...
34        e (v, v);^M
(gdb) PASS: gdb.arch/amd64-entry-value.exp: continue to breakpoint: 
self: breakhere
bt^M
#0  d (i=<optimized out>, j=<optimized out>) at 
gdb.arch/amd64-entry-value.cc:34^M
#1  0x0000000000400825 in self (i=<optimized out>) at 
gdb.arch/amd64-entry-value.cc:120^M
#2  0x0000000000400832 in self2 (i=<optimized out>) at 
gdb.arch/amd64-entry-value.cc:106^M
#3  0x0000000000400830 in self (i=5) at gdb.arch/amd64-entry-value.cc:115^M
#4  0x00000000004005c8 in main () at gdb.arch/amd64-entry-value.cc:231^M
(gdb) FAIL: gdb.arch/amd64-entry-value.exp: self: bt
...
where we should have:
...
#3  0x0000000000400830 in self (i=<optimized out>) at 
gdb.arch/amd64-entry-value.cc:115^M
...

Function self has parameter i:
...
  <2><70e>: Abbrev Number: 6 (DW_TAG_formal_parameter)
     <70f>   DW_AT_name        : i
     <713>   DW_AT_type        : <0x303>
     <717>   DW_AT_location    : 0x4c5 (location list)
...
with location list:
...
     000004c5 00000000004007f0 0000000000400807 (DW_OP_reg5 (rdi))
     000004d8 0000000000400807 0000000000400818 (DW_OP_reg3 (rbx))
     000004eb 0000000000400818 0000000000400824 (DW_OP_breg5 (rdi): -2; 
DW_OP_stack_value)
     00000500 0000000000400824 0000000000400825 (DW_OP_GNU_entry_value: 
(DW_OP_reg5 (rdi)); DW_OP_stack_value)
     00000516 0000000000400825 0000000000400829 (DW_OP_reg3 (rbx))
     00000529 0000000000400829 000000000040082e (DW_OP_reg5 (rdi))
     0000053c 000000000040082e 0000000000400830 (DW_OP_GNU_entry_value: 
(DW_OP_reg5 (rdi)); DW_OP_stack_value)
     00000552 <End of list>
...

Given "#3  0x0000000000400830" we end up with in the "000000000040082e - 
0000000000400830" range (though the end-of-range is exclusive, but we 
use a $pc-1 trick in get_frame_address_in_block), and try to evaluate 
"(DW_OP_GNU_entry_value: (DW_OP_reg5 (rdi)); DW_OP_stack_value)".

The "find_function_type (get_frame_pc (m_frame)) != nullptr" test 
succeeds because 0x0000000000400830 is the start of self2, which is I 
added the "m_frame == get_current_frame ()" bit.

So, perhaps we should use get_frame_address_in_block here as well:
...
   if (find_function_type (get_frame_address_in_block (m_frame))
       != nullptr)
...
?

I'll test this solution.

Thanks,
- Tom
  

On 3/22/25 09:06, Tom de Vries wrote:
> On 3/20/25 16:51, Andrew Burgess wrote:
>>
>> Hey Tom,
>>
>> This all looks reasonable to me, I did have one question, which is more
>> for my own education than anything ...
>>
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
>>> index ee1522b7437..695b30e71b4 100644
>>> --- a/gdb/dwarf2/expr.c
>>> +++ b/gdb/dwarf2/expr.c
>>> @@ -881,6 +881,34 @@ dwarf_expr_context::read_mem (gdb_byte *buf, 
>>> CORE_ADDR addr,
>>>   /* See expr.h.  */
>>> +value *
>>> +dwarf_expr_context::deref (CORE_ADDR addr, int size, struct type *type)
>>> +{
>>> +  gdb_byte *buf = (gdb_byte *) alloca (size);
>>> +  this->read_mem (buf, addr, size);
>>> +
>>> +  if (type == nullptr)
>>> +    type = this->address_type ();
>>> +
>>> +  /* If the size of the object read from memory is different
>>> +     from the type length, we need to zero-extend it.  */
>>> +  if (type->length () != size)
>>> +    {
>>> +      gdbarch *arch = this->m_per_objfile->objfile->arch ();
>>> +      bfd_endian byte_order = gdbarch_byte_order (arch);
>>> +      ULONGEST datum
>>> +    = extract_unsigned_integer (buf, size, byte_order);
>>> +
>>> +      buf = (gdb_byte *) alloca (type->length ());
>>> +      store_unsigned_integer (buf, type->length (),
>>> +                  byte_order, datum);
>>> +    }
>>> +
>>> +  return value_from_contents_and_address (type, buf, addr);
>>> +}
>>> +
>>> +/* See expr.h.  */
>>> +
>>>   void
>>>   dwarf_expr_context::push_dwarf_reg_entry_value 
>>> (call_site_parameter_kind kind,
>>>                           call_site_parameter_u kind_u,
>>> @@ -1916,7 +1944,6 @@ dwarf_expr_context::execute_stack_op (const 
>>> gdb_byte *op_ptr,
>>>       case DW_OP_GNU_deref_type:
>>>         {
>>>           int addr_size = (op == DW_OP_deref ? this->m_addr_size : 
>>> *op_ptr++);
>>> -        gdb_byte *buf = (gdb_byte *) alloca (addr_size);
>>>           CORE_ADDR addr = fetch_address (0);
>>>           struct type *type;
>>> @@ -1931,21 +1958,7 @@ dwarf_expr_context::execute_stack_op (const 
>>> gdb_byte *op_ptr,
>>>           else
>>>             type = address_type;
>>> -        this->read_mem (buf, addr, addr_size);
>>> -
>>> -        /* If the size of the object read from memory is different
>>> -           from the type length, we need to zero-extend it.  */
>>> -        if (type->length () != addr_size)
>>> -          {
>>> -        ULONGEST datum =
>>> -          extract_unsigned_integer (buf, addr_size, byte_order);
>>> -
>>> -        buf = (gdb_byte *) alloca (type->length ());
>>> -        store_unsigned_integer (buf, type->length (),
>>> -                    byte_order, datum);
>>> -          }
>>> -
>>> -        result_val = value_from_contents_and_address (type, buf, addr);
>>> +        result_val = this->deref (addr, addr_size, type);
>>>           break;
>>>         }
>>> @@ -2274,6 +2287,18 @@ dwarf_expr_context::execute_stack_op (const 
>>> gdb_byte *op_ptr,
>>>           if (kind_u.dwarf_reg != -1)
>>>             {
>>>           op_ptr += len;
>>> +
>>> +        if (m_frame == get_current_frame ()
>>> +            && find_function_type (get_frame_pc (m_frame)) != nullptr)
>>
>> Given that this construct is duplicated, and then you have a comment to
>> explain what the `if` condition is checking for, I wonder if moving this
>> check into a nicely named helper function would make things clearer?
>>
> 
> Hi Andrew,
> 
> thanks for the review.
> 
>> But, my real question is, why the `m_frame == get_current_frame ()`
>> check?  I believe that `m_frame` is the frame in which the expression is
>> being evaluated, so why isn't it enough to check if we are at the very
>> start of m_frame?  Why does m_frame have to be the current frame?
>>
>> I just couldn't figure this out.
> 
> If not, we run into:
> ...
> 34        e (v, v);^M
> (gdb) PASS: gdb.arch/amd64-entry-value.exp: continue to breakpoint: 
> self: breakhere
> bt^M
> #0  d (i=<optimized out>, j=<optimized out>) at gdb.arch/amd64-entry- 
> value.cc:34^M
> #1  0x0000000000400825 in self (i=<optimized out>) at gdb.arch/amd64- 
> entry-value.cc:120^M
> #2  0x0000000000400832 in self2 (i=<optimized out>) at gdb.arch/amd64- 
> entry-value.cc:106^M
> #3  0x0000000000400830 in self (i=5) at gdb.arch/amd64-entry-value.cc:115^M
> #4  0x00000000004005c8 in main () at gdb.arch/amd64-entry-value.cc:231^M
> (gdb) FAIL: gdb.arch/amd64-entry-value.exp: self: bt
> ...
> where we should have:
> ...
> #3  0x0000000000400830 in self (i=<optimized out>) at gdb.arch/amd64- 
> entry-value.cc:115^M
> ...
> 
> Function self has parameter i:
> ...
>   <2><70e>: Abbrev Number: 6 (DW_TAG_formal_parameter)
>      <70f>   DW_AT_name        : i
>      <713>   DW_AT_type        : <0x303>
>      <717>   DW_AT_location    : 0x4c5 (location list)
> ...
> with location list:
> ...
>      000004c5 00000000004007f0 0000000000400807 (DW_OP_reg5 (rdi))
>      000004d8 0000000000400807 0000000000400818 (DW_OP_reg3 (rbx))
>      000004eb 0000000000400818 0000000000400824 (DW_OP_breg5 (rdi): -2; 
> DW_OP_stack_value)
>      00000500 0000000000400824 0000000000400825 (DW_OP_GNU_entry_value: 
> (DW_OP_reg5 (rdi)); DW_OP_stack_value)
>      00000516 0000000000400825 0000000000400829 (DW_OP_reg3 (rbx))
>      00000529 0000000000400829 000000000040082e (DW_OP_reg5 (rdi))
>      0000053c 000000000040082e 0000000000400830 (DW_OP_GNU_entry_value: 
> (DW_OP_reg5 (rdi)); DW_OP_stack_value)
>      00000552 <End of list>
> ...
> 
> Given "#3  0x0000000000400830" we end up with in the "000000000040082e - 
> 0000000000400830" range (though the end-of-range is exclusive, but we 
> use a $pc-1 trick in get_frame_address_in_block), and try to evaluate 
> "(DW_OP_GNU_entry_value: (DW_OP_reg5 (rdi)); DW_OP_stack_value)".
> 
> The "find_function_type (get_frame_pc (m_frame)) != nullptr" test 
> succeeds because 0x0000000000400830 is the start of self2, which is I 
> added the "m_frame == get_current_frame ()" bit.
> 
> So, perhaps we should use get_frame_address_in_block here as well:
> ...
>    if (find_function_type (get_frame_address_in_block (m_frame))
>        != nullptr)
> ...
> ?
> 
> I'll test this solution.

I did so and found no problems, but thinking about it some more, I 
realized that it could be possible that the first instruction of a 
function is also a tail call, so I've added the test back again, only 
now in the form of frame_relative_level (frame) == 0.

I've split off the duplicated construct into a separate function as you 
suggested:
...
+/* Return true if, for an expr evaluated in the context of FRAME, we
     can
+   assume that DW_OP_entry_value (expr) == expr.
+
+   We can assume this right after executing a call, when stopped at the
+   start of the called function, in other words, when:
+   - FRAME is the innermost frame, and
+   - FRAME->pc is the first insn in a function.  */
+
+static bool
+trivial_entry_value (frame_info_ptr frame)
+{
+  bool innermost_frame = frame_relative_level (frame) == 0;
+
+  /* Get pc corresponding to frame.  Use get_frame_address_in_block to
       make
+     sure we get a pc in the correct function in the case of tail
       calls.  */
+  CORE_ADDR pc = get_frame_address_in_block (frame);
+  bool at_first_insn = find_function_type (pc) != nullptr;
+
+  return innermost_frame && at_first_insn;
+}
+
...

Submitted as v3 here ( 
https://sourceware.org/pipermail/gdb-patches/2025-March/216625.html ).

Thanks,
- Tom

> Thanks,
> - Tom