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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello Tom,

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

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

Thank you for debugging the issue! So memcpy was overflowing the
buffer. Nice to see the assert in action. :)

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

It should be '>=' above.

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

Thank you for the patch! With the fix in the assert comparison:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

--
Thiago