[PATCH] gdb: fixes for DW_OP_entry_value when inferior is at entry point

[PATCH] gdb: fixes for DW_OP_entry_value when inferior is at entry point

[Andrew Burgess]

This fixes some issues with DW_OP_entry_value which are discussed in
PR gdb/34571.

The bug identifies a case where a variable has a DW_AT_location value
of (on s390):

  DW_OP_entry_value: (DW_OP_reg2 (r2)); DW_OP_stack_value

But GDB is not able to correctly figure out the variable's value.

To understand the fix for this bug we need to first revisit the
earlier commit that introduced the bug:

  commit 1bafda2c4595f0f936a5845caf9667b70b198091
  Date:   Wed Apr 9 12:02:18 2025 +0200

      [gdb/symtab] Handle DW_OP_entry_value at function entry

I believe there is a misunderstanding in this commit about how
different DWARF attributes are handled, and GDB was updated inline
with this misunderstanding.

The commit message for 1bafda2c4595f0f9 includes this example DWARF:

        <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)

We need to notice two things here:

  1. This does indeed use DW_OP_entry_value, and

  2. It does not end with DW_OP_stack_value, the final value
     calculated by this expression is the value of the
     DW_AT_upper_bound attribute.

The test then adds a test that uses the DWARF assembler to assemble
this:

               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 }
               }

On my x86-64 machine this results in the following DWARF:

  <2><4d>: Abbrev Number: 4 (DW_TAG_variable)
     <4e>   DW_AT_name        : argc
     <53>   DW_AT_type        : <0x2c>
     <57>   DW_AT_location    : 4 byte block: a3 2 90 5
                                (DW_OP_entry_value: (DW_OP_regx: 5 (rdi)))

Notice here that:

  1. This also uses DW_OP_entry_value, and

  2. As with the DW_AT_upper_bound case, this does not end with
     DW_OP_stack_value.

However, I believe this is a misunderstanding of the DWARF.
DW_AT_upper_bound and DW_AT_location handle their DWARF expressions in
two different ways.

Here's part of what DWARF-5 says about DW_OP_entry_value:

  The DW_OP_entry_value operation pushes the value that the described
  location held upon entering the current subprogram.

So when the DW_AT_location expression is evaluated the DWARF stack
will contain the entry value for register %rdi.  However, that is the
value of the register, it is not a register name, given the above
DWARF, GDB should be treating the value on the stack (the contents of
%rdi) as the address at which the variable can be found, which is not
what the test expects.

But, we can clearly see why the test was written this way.  It was
trying to represent the original problematic DWARF, which used
DW_OP_entry_value without a trailing DW_OP_stack_value.  However, the
original case was for DW_AT_upper_bound, which I think is covered by
2.19 "Static and Dynamic Values of Attributes" in the DWARF-5 spec.
In this section we see:

  "Some attributes that apply to types specify a property (such as the
   lower bound of an array) that is an integer value, where the value
   may be known during compilation or may be computed dynamically
   during execution."

and later in the same section:

  "For an exprloc, the value is interpreted as a DWARF expression;
   evaluation of the expression yields the value of the attribute."

So I believe this is telling us that the DWARF expression for
DW_AT_upper_bound should be handled differently than the expression
for DW_AT_location.  For DW_AT_location the result on the DWARF stack
is going to be one of the location descriptions listed in section
2.6.1.1 "Simple Location Descriptions", but for DW_AT_upper_bound the
result will be the value itself.

If we go back to the original PR gdb/34571 bug we see that it's
DW_AT_location expression was:

  DW_OP_entry_value: (DW_OP_reg2 (r2)); DW_OP_stack_value

with a trailing DW_OP_stack_value.  The test case didn't have that
trailing DW_OP_stack_value.  The problem is that currently, when GDB sees:

  DW_OP_entry_value: (DW_OP_reg2 (r2));

It pushes the register name $r2 to the DWARF stack, and then marks the
stack as being a register location description.  This works for the
test where there is no DW_OP_stack_value.  But when we add the
trailing DW_OP_stack_value GDB marks the stack as being an "Implicit
Location Description" (meaning the value on the stack is the result).
This causes us to then interpret the register number as the result,
rather than fetching the register value.

I worried that I was going to somehow have to try and support both
cases, but the more I looked into it, the more I convinced myself that
commit 1bafda2c4595f0f9 wasn't fully correct, and that we should just
update the test that was added in that commit.

So that's what this commit does.

When we see DW_OP_entry_value for a plain register name, and we're at
the very start of the function, instead of pushing the register name
to the stack and marking the stack as being a "Register Location
Description", we instead read the register value, push that to the
stack, and leave the m_location variable unchanged.

Then in the test I've added a DW_OP_stack_value to the DW_AT_location
attribute.

I've also added some additional variables with more complex
DW_AT_location expressions.  These all make use of DW_OP_entry_value,
but manipulate the value in some way to compute the final result.
These reflect examples that I saw when compiling the example code from
PR gdb/34571 at different optimisation levels.

One thing that did puzzle me is that commit 1bafda2c4595f0f9 talks
about the problem having been discovered when looking at the text
gdb.base/vla-optimized-out.exp on risc-v, and the claim is that the
issues in that test were fixed by 1bafda2c4595f0f9.  I didn't
understand how that could be possible given the bug I claim exists.
But if we look at the original DW_AT_upper_bound DWARF we see what
happened:

  DW_OP_entry_value: (DW_OP_reg10 (a0));	<- Reg value on stack.
  DW_OP_plus_uconst: 1;				<- Add 1.
  DW_OP_const1u: 32;				\
  DW_OP_shl;					| Sign extend via
  DW_OP_const1u: 32;				| left/right shift.
  DW_OP_shra;					/
  DW_OP_lit1;					\ Subtract 1.
  DW_OP_minus					/

I don't understand why there's the +1/-1 logic in there, I wonder if
this is a compiler artefact, but clearly this is supposed to take the
value from register $a10 and sign extend it from 32 to 64 bits.

However, what it actually does is take the register NUMBER, and sign
extend that.  Luckily though, for small register numbers, the whole
expression leaves the register number unchanged on the DWARF
expression stack.

Because DW_OP_entry_value also incorrectly marks the DWARF expression
stack as being a "Register Location Description", then after all this
is complete GDB reads the value from the register.

So long at the value it reads didn't actually need sign extending then
we're fine.  In this test the value in the register is '5', which
doesn't require the sign extension, so despite the bug in GDB, the
test does the right thing.

That at least explains why the original commit appeared to fix the
problem with gdb.base/vla-optimized-out.exp.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34571
---
 gdb/dwarf2/expr.c                             | 11 +++--
 gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c  |  1 +
 .../gdb.dwarf2/dw2-entry-value-2.exp          | 46 +++++++++++++++++++
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index 3a6b8f58199..934fda67ca6 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -2365,10 +2365,15 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 		if (trivial_entry_value (this->m_frame))
 		  {
 		    /* We can assume that DW_OP_entry_value (expr) == expr.
-		       Handle as DW_OP_regx.  */
+		       Handle DW_OP_regx, place register value on the
+		       stack.  */
+		    gdbarch *f_arch = get_frame_arch (this->m_frame);
+		    int dwarf_regnum = kind_u.dwarf_reg;
+		    int gdb_regnum
+		      = dwarf_reg_to_regnum_or_error (f_arch, dwarf_regnum);
 		    result_val
-		      = value_from_ulongest (address_type, kind_u.dwarf_reg);
-		    this->m_location = DWARF_VALUE_REGISTER;
+		      = value_from_register (address_type, gdb_regnum,
+					     this->m_frame);
 		    break;
 		  }
 
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c b/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c
index 45fa86bdf2f..af3199abcee 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c
+++ b/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.c
@@ -16,6 +16,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 int var = 2;
+unsigned long long fake_data[3] = { 1, 2, 3 };
 
 static
 void bar (int *p)
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.exp b/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.exp
index 3b48846fb71..6d441dc53f8 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-entry-value-2.exp
@@ -67,6 +67,33 @@ Dwarf::assemble $asm_file {
 			DW_OP_entry_value {
 			    DW_OP_regx $::dwarf_regnum
 			}
+			DW_OP_stack_value
+		     } SPECIAL_expr
+		}
+
+		DW_TAG_variable {
+		     DW_AT_name argc2
+		     DW_AT_type :$integer
+		     DW_AT_location {
+			DW_OP_entry_value {
+			    DW_OP_regx $::dwarf_regnum
+			}
+			DW_OP_plus_uconst 3
+			DW_OP_stack_value
+		     } SPECIAL_expr
+		}
+
+		DW_TAG_variable {
+		     DW_AT_name argc3
+		     DW_AT_type :$integer
+		     DW_AT_location {
+			DW_OP_addr [gdb_target_symbol fake_data]
+			DW_OP_entry_value {
+			    DW_OP_regx $::dwarf_regnum
+			}
+			DW_OP_const1u 3
+			DW_OP_shl
+			DW_OP_plus
 		     } SPECIAL_expr
 		}
 	    }
@@ -87,6 +114,19 @@ Dwarf::assemble $asm_file {
 			DW_OP_stack_value
 		     } SPECIAL_expr
 		}
+
+		DW_TAG_variable {
+		     DW_AT_name foo2
+		     DW_AT_type :$integer
+		     DW_AT_location {
+			DW_OP_entry_value {
+			    DW_OP_bregx $::dwarf_regnum 0
+			    DW_OP_deref_size 4
+			}
+			DW_OP_plus_uconst 1
+			DW_OP_stack_value
+		     } SPECIAL_expr
+		}
 	    }
 	}
     }
@@ -103,12 +143,16 @@ if { ![runto *main] } {
 
 with_test_prefix "at main+0" {
     gdb_test "p argc" " = 1"
+    gdb_test "p argc2" " = 4"
+    gdb_test "p argc3" " = 2"
 
     gdb_test "stepi"
 }
 
 with_test_prefix "at main+1" {
     gdb_test "p argc" " = <optimized out>"
+    gdb_test "p argc2" " = <optimized out>"
+    gdb_test "p argc3" " = <optimized out>"
 }
 
 gdb_breakpoint "*bar"
@@ -116,10 +160,12 @@ gdb_continue_to_breakpoint "bar"
 
 with_test_prefix "at bar+0" {
     gdb_test "p foo" " = 2"
+    gdb_test "p foo2" " = 3"
 
     gdb_test "stepi"
 }
 
 with_test_prefix "at bar+1" {
     gdb_test "p foo" " = <optimized out>"
+    gdb_test "p foo2" " = <optimized out>"
 }

base-commit: 6e3ecea0e3ca191e81e82ee0194c49eea1ffb101
-- 
2.25.4

Re: [PATCH] gdb: fixes for DW_OP_entry_value when inferior is at entry point

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> This fixes some issues with DW_OP_entry_value which are discussed in
Andrew> PR gdb/34571.

Thanks for doing this.

Andrew> To understand the fix for this bug we need to first revisit the
Andrew> earlier commit that introduced the bug:

Andrew>   commit 1bafda2c4595f0f936a5845caf9667b70b198091
Andrew>   Date:   Wed Apr 9 12:02:18 2025 +0200

Andrew>       [gdb/symtab] Handle DW_OP_entry_value at function entry

I appreciate the research and commentary as well.

Looking at this code, I don't really follow why there's even such a
large special case for trivial_entry_value.  It seems like the code
could be much simpler: when seeing a DW_OP_entry_value, evaluate the
entry value expression in the target frame -- either the parent frame,
or for the trivial_entry_value case, the current frame.

That is, these special checks and calls to dwarf_block_to_dwarf_reg and
dwarf_block_to_dwarf_reg_deref (which to be clear predate the
trivial_entry_value patch) also seem totally unnecessary, as does this
restriction:

	    error (_("DWARF-2 expression error: DW_OP_entry_value is "
		     "supported only for single DW_OP_reg* "
		     "or for DW_OP_breg*(0)+DW_OP_deref*"));

That said, I don't think there's anything wrong with an improvement that
doesn't rewrite it all.  And anyway maybe there's some subtlety I don't
understand.

Approved-By: Tom Tromey <tom@tromey.com>

thanks,
Tom

Re: [PATCH] gdb: fixes for DW_OP_entry_value when inferior is at entry point

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> This fixes some issues with DW_OP_entry_value which are discussed in
> Andrew> PR gdb/34571.
>
> Thanks for doing this.
>
> Andrew> To understand the fix for this bug we need to first revisit the
> Andrew> earlier commit that introduced the bug:
>
> Andrew>   commit 1bafda2c4595f0f936a5845caf9667b70b198091
> Andrew>   Date:   Wed Apr 9 12:02:18 2025 +0200
>
> Andrew>       [gdb/symtab] Handle DW_OP_entry_value at function entry
>
> I appreciate the research and commentary as well.
>
> Looking at this code, I don't really follow why there's even such a
> large special case for trivial_entry_value.  It seems like the code
> could be much simpler: when seeing a DW_OP_entry_value, evaluate the
> entry value expression in the target frame -- either the parent frame,
> or for the trivial_entry_value case, the current frame.
>
> That is, these special checks and calls to dwarf_block_to_dwarf_reg and
> dwarf_block_to_dwarf_reg_deref (which to be clear predate the
> trivial_entry_value patch) also seem totally unnecessary, as does this
> restriction:
>
> 	    error (_("DWARF-2 expression error: DW_OP_entry_value is "
> 		     "supported only for single DW_OP_reg* "
> 		     "or for DW_OP_breg*(0)+DW_OP_deref*"));
>
> That said, I don't think there's anything wrong with an improvement that
> doesn't rewrite it all.  And anyway maybe there's some subtlety I don't
> understand.
>
> Approved-By: Tom Tromey <tom@tromey.com>

Thanks Tom.

I pushed this patch to master and gdb-18-branch.

I also have a follow on patch which is in testing now, that refactors
this code inline with your suggestion above.

I decided to push the first patch as the change here is smaller, and
especially for the gdb-18-branch, it seemed lower risk.  The refactor,
once I post it, would only be for master.

Thanks,
Andrew