Re: [PATCH 2/2] GDB: Use gdb::array_view for buffers used in register reading and unwinding

[Tom de Vries <tdevries@suse.de>]

On 1/10/25 23:28, Thiago Jung Bauermann wrote:
> +      gdb_assert (buffer.size () >= value->type ()->length ());
> +

This causes a regression on s390x-linux for test-case gdb.base/return.exp:
...
(gdb) PASS: gdb.base/return.exp: continue to return of -5
return 5^M
Make func2 return now? (y or n) y^M
/home/vries/gdb/src/gdb/frame.c:1207: internal-error: 
frame_register_unwind: Assertion `buffer.size () >= value->type 
()->length ()' failed.^M
A problem internal to GDB has been detected,^M
further debugging may prove unreliable.^M
----- Backtrace -----^M
FAIL: gdb.base/return.exp: return value 5 (GDB internal error)
...

Before the commit, the test-case produces a fail, but doesn't assert:
...
(gdb) PASS: gdb.base/return.exp: continue to return of -5
return 5^M
Make func2 return now? (y or n) y^M
value has been optimized out^M
(gdb) FAIL: gdb.base/return.exp: return value 5
...

Concretely, we're trying to read machine register r11, which according 
to the CFI is saved in dwarf register 16:
...
   DW_CFA_register: r11 in r16 (f0)
...

Dwarf register 16 corresponds to f0 / v0 according to the ABI, and since 
v0 is available, v0 is picked by s390_dwarf_reg_to_regnum.

The assert then fails because the buffer that should hold the value of 8 
byte register r11:
...
(gdb) p buffer.size ()
$1 = 8
...
is smaller than the size of register v0:
...
(gdb) p value->type ()->length ()
$2 = 16
...

Removing the assert reverts back to previous behaviour.

Properly fixing this requires us to only look at the part that is 
relevant, copying the value from there, and checking for optimized out 
and unavailable only there.

This worked for s390x-linux:
...
diff --git a/gdb/frame.c b/gdb/frame.c
index 10a32dcd896..02583857019 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1193,8 +1193,14 @@ frame_register_unwind (const frame_info_ptr 
&next_frame, int regnum,

    gdb_assert (value != NULL);

-  *optimizedp = value->optimized_out ();
-  *unavailablep = !value->entirely_available ();
+  if (value->lazy ())
+    value->fetch_lazy ();
+
+  *optimizedp
+    = value->bits_any_optimized_out (value->offset () * 8,
+				     buffer.size () * 8);
+  *unavailablep
+    = !value->bytes_available (value->offset (), buffer.size ());
    *lvalp = value->lval ();
    *addrp = value->address ();
    if (*lvalp == lval_register)
@@ -1204,13 +1210,17 @@ frame_register_unwind (const frame_info_ptr 
&next_frame, int regnum,

    if (!buffer.empty ())
      {
-      gdb_assert (buffer.size () >= value->type ()->length ());
+      gdb_assert (buffer.size ()
+		  <= value->type ()->length () - value->offset ());

        if (!*optimizedp && !*unavailablep)
-	memcpy (buffer.data (), value->contents_all ().data (),
-		value->type ()->length ());
+	{
+	  auto value_part
+	    = value->contents_all ().slice (value->offset (), buffer.size ());
+	  memcpy (buffer.data (), value_part.data (), buffer.size ());
+	}
        else
-	memset (buffer.data (), 0, value->type ()->length ());
+	memset (buffer.data (), 0, buffer.size ());
      }

    /* Dispose of the new value.  This prevents watchpoints from
...

Thanks,
- Tom