Fix gdb.base/return.exp on s390x-linux

Message ID 20250113155656.11978-1-tdevries@suse.de
State Superseded
Headers
Series Fix gdb.base/return.exp on s390x-linux |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Build failed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom de Vries Jan. 13, 2025, 3:56 p.m. UTC
  Starting commit 7fcdec025c0 ("GDB: Use gdb::array_view for buffers used in
register reading and unwinding"), test-case gdb.base/return.exp fails on
s390x-linux like this:
...
(gdb) PASS: gdb.base/return.exp: continue to return of -5
return 5^M
Make func2 return now? (y or n) y^M
gdb/frame.c:1207: internal-error: frame_register_unwind: \
  Assertion `buffer.size () >= value->type ()->length ()' failed.^M
...

This happens as follows.

Function frame_register_unwind is called with regnum 13 (r11).

The value of r11 was saved in register f0 in the prologue:
...
       ldgr    %f0,%r11
...
and the CFI tells us so:
...
       DW_CFA_register: r11 in r16 (f0)
...
or more precisely, it tells us that r11 was saved in DWARF register 16.

The s390x ABI specifies that DWARF register 16 maps onto either 8-byte
register f0 or 16-byte register v0 (where f0 is part of v0), and
s390_dwarf_reg_to_regnum maps DWARF register 16 to v0, if available.

So this bit in frame_register_unwind:
...
  value = frame_unwind_register_value (next_frame, regnum);
...
ends up returning the 16-byte value of v0.

Before the aforementioned commit, this memcpy would be triggered:
...
       memcpy (bufferp, value->contents_all ().data (),
               value->type ()->length ());
...
which would:
- copy the correct info into the 8 bytes buffer (sort of by accident because
  it assumes that value->offset () == 0, which happens to be the case), but
  also
- write past the end of the buffer.

The commit adds an assert that prevents writing past the end of the buffer,
and consequently it triggers.

Fix this by only considering the portion of the value required to fill the
buffer.

Tested on s390x-linux.
---
 gdb/frame.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 7 deletions(-)


base-commit: 127f733f88717bdb52f03c12c0c2d240bbc892e3
  

Comments

Simon Marchi Jan. 13, 2025, 8:11 p.m. UTC | #1
On 2025-01-13 10:56, Tom de Vries wrote:
> Starting commit 7fcdec025c0 ("GDB: Use gdb::array_view for buffers used in
> register reading and unwinding"), test-case gdb.base/return.exp fails on
> s390x-linux like this:
> ...
> (gdb) PASS: gdb.base/return.exp: continue to return of -5
> return 5^M
> Make func2 return now? (y or n) y^M
> gdb/frame.c:1207: internal-error: frame_register_unwind: \
>   Assertion `buffer.size () >= value->type ()->length ()' failed.^M
> ...
> 
> This happens as follows.
> 
> Function frame_register_unwind is called with regnum 13 (r11).
> 
> The value of r11 was saved in register f0 in the prologue:
> ...
>        ldgr    %f0,%r11
> ...
> and the CFI tells us so:
> ...
>        DW_CFA_register: r11 in r16 (f0)
> ...
> or more precisely, it tells us that r11 was saved in DWARF register 16.
> 
> The s390x ABI specifies that DWARF register 16 maps onto either 8-byte
> register f0 or 16-byte register v0 (where f0 is part of v0), and
> s390_dwarf_reg_to_regnum maps DWARF register 16 to v0, if available.
> 
> So this bit in frame_register_unwind:
> ...
>   value = frame_unwind_register_value (next_frame, regnum);
> ...
> ends up returning the 16-byte value of v0.
> 
> Before the aforementioned commit, this memcpy would be triggered:
> ...
>        memcpy (bufferp, value->contents_all ().data (),
>                value->type ()->length ());
> ...
> which would:
> - copy the correct info into the 8 bytes buffer (sort of by accident because
>   it assumes that value->offset () == 0, which happens to be the case), but
>   also
> - write past the end of the buffer.

I think it's conceptually wrong that the value returned by
frame_unwind_register_value for r11 (8 bytes) has the type of v0 (16
bytes).  Or, if the type of the value returned by
frame_unwind_register_value can't be changed, I think it's irrelevant
here and we should ignore it.

The value returned by frame_unwind_register_value represents where the
original 8 bytes r11 value was saved.  Even if it was saved in the 16
bytes v0, it's still an 8 byte value.  It just happens to be located
at some offset of register v0.  But the type of v0 is irrelevant here.

Imagine if the offset was non-zero (let's say it's 4).  Having the value
with the type of v0 (16 bytes) would not make sense.  It would mean that
we have a 16 bytes value that starts at offset 4 of a 16 byte register,
which doesn't work.

Instead if the return value had the type of r11, it would mean: starting
at this offset (0) of this register (v0), we have a value with a length
of 8 bytes, which is the reality.

Concretely, what I'm saying is that I think that the value returned by
frame_unwind_register_value should have the type of register REGNUM (the
parameter).  We would then use the length of that type in the memcpy,
which would fix the problem at hand.

If the change above is not possible, we should at least take the number
of bytes to memcpy from the type of register REGNUM (the one we try to
get the value of) and ignore the type of "value", since we know it's not
relevant.

> @@ -1204,13 +1235,15 @@ frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
>  
>    if (!buffer.empty ())
>      {
> -      gdb_assert (buffer.size () >= value->type ()->length ());

I think having this assert is relevant, but it should use the length of
the type of register REGNUM.  If we're asking to unwind (read) the value
of register REGNUM, then the destination buffer should certainly be
large enough to hold the value of register REGNUM.

Simon
  
Tom de Vries Jan. 15, 2025, 12:16 p.m. UTC | #2
On 1/13/25 21:11, Simon Marchi wrote:
> 
> 
> On 2025-01-13 10:56, Tom de Vries wrote:
>> Starting commit 7fcdec025c0 ("GDB: Use gdb::array_view for buffers used in
>> register reading and unwinding"), test-case gdb.base/return.exp fails on
>> s390x-linux like this:
>> ...
>> (gdb) PASS: gdb.base/return.exp: continue to return of -5
>> return 5^M
>> Make func2 return now? (y or n) y^M
>> gdb/frame.c:1207: internal-error: frame_register_unwind: \
>>    Assertion `buffer.size () >= value->type ()->length ()' failed.^M
>> ...
>>
>> This happens as follows.
>>
>> Function frame_register_unwind is called with regnum 13 (r11).
>>
>> The value of r11 was saved in register f0 in the prologue:
>> ...
>>         ldgr    %f0,%r11
>> ...
>> and the CFI tells us so:
>> ...
>>         DW_CFA_register: r11 in r16 (f0)
>> ...
>> or more precisely, it tells us that r11 was saved in DWARF register 16.
>>
>> The s390x ABI specifies that DWARF register 16 maps onto either 8-byte
>> register f0 or 16-byte register v0 (where f0 is part of v0), and
>> s390_dwarf_reg_to_regnum maps DWARF register 16 to v0, if available.
>>
>> So this bit in frame_register_unwind:
>> ...
>>    value = frame_unwind_register_value (next_frame, regnum);
>> ...
>> ends up returning the 16-byte value of v0.
>>
>> Before the aforementioned commit, this memcpy would be triggered:
>> ...
>>         memcpy (bufferp, value->contents_all ().data (),
>>                 value->type ()->length ());
>> ...
>> which would:
>> - copy the correct info into the 8 bytes buffer (sort of by accident because
>>    it assumes that value->offset () == 0, which happens to be the case), but
>>    also
>> - write past the end of the buffer.
> 
> I think it's conceptually wrong that the value returned by
> frame_unwind_register_value for r11 (8 bytes) has the type of v0 (16
> bytes).

Hi Simon,

thanks for the review.

AFAIU also Thiago mentioned this point, and I tried to address this, by 
adding a size parameter to dwarf2_reg_to_regnum, but gave up after a 
while.  Also I'm not sure anymore whether that's a good idea in the 
first place.

Anyway, since you mentioned it here, I tried again, and this time I 
found a way to get frame_unwind_register_value to return an 8-byte type.

I've submitted it as a v2 ( 
https://sourceware.org/pipermail/gdb-patches/2025-January/214765.html )

> Or, if the type of the value returned by
> frame_unwind_register_value can't be changed, I think it's irrelevant
> here and we should ignore it.
> 
> The value returned by frame_unwind_register_value represents where the
> original 8 bytes r11 value was saved.  Even if it was saved in the 16
> bytes v0, it's still an 8 byte value.  It just happens to be located
> at some offset of register v0.  But the type of v0 is irrelevant here.
> 
> Imagine if the offset was non-zero (let's say it's 4).  Having the value
> with the type of v0 (16 bytes) would not make sense.  It would mean that
> we have a 16 bytes value that starts at offset 4 of a 16 byte register,
> which doesn't work.
> 
> Instead if the return value had the type of r11, it would mean: starting
> at this offset (0) of this register (v0), we have a value with a length
> of 8 bytes, which is the reality.
> 
> Concretely, what I'm saying is that I think that the value returned by
> frame_unwind_register_value should have the type of register REGNUM (the
> parameter).  We would then use the length of that type in the memcpy,
> which would fix the problem at hand.
> 
> If the change above is not possible, we should at least take the number
> of bytes to memcpy from the type of register REGNUM (the one we try to
> get the value of) and ignore the type of "value", since we know it's not
> relevant.
> 
>> @@ -1204,13 +1235,15 @@ frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
>>   
>>     if (!buffer.empty ())
>>       {
>> -      gdb_assert (buffer.size () >= value->type ()->length ());
> 
> I think having this assert is relevant, but it should use the length of
> the type of register REGNUM.  If we're asking to unwind (read) the value
> of register REGNUM, then the destination buffer should certainly be
> large enough to hold the value of register REGNUM.

FWIW, in the v2, I've left this assert alone.

Thanks,
- Tom
  

Patch

diff --git a/gdb/frame.c b/gdb/frame.c
index 10a32dcd896..da00cbd5d1d 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1193,8 +1193,39 @@  frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
 
   gdb_assert (value != NULL);
 
-  *optimizedp = value->optimized_out ();
-  *unavailablep = !value->entirely_available ();
+  /* The s390x ABI specifies that DWARF register 16 maps onto either
+     8-byte register f0 or 16-byte register v0 (where f0 is part of v0), and
+     s390_dwarf_reg_to_regnum maps DWARF register 16 to v0, if available.
+
+     So, if a function saves register r11 in the prologue to register f0:
+
+       ldgr    %f0,%r11
+
+     then it's represented like this in the CFI:
+
+       DW_CFA_register: r11 in r16 (f0)
+
+     and unwinding 8-byte register r11 gets us the value of 16-byte register
+     v0.
+
+     Handle this by not considering more bytes than fit in the buffer.
+
+     Then there's i386_unwind_pc which passes an 8-byte buffer, while the
+     register may be either 8-byte (for x86_64) or 4-byte (for i386).
+
+     Handle this by not considering more bytes than are provided in the
+     value.  */
+  size_t copy_len = std::min (buffer.size (),
+			      value->type ()->length () - value->offset ());
+
+  if (value->lazy ())
+    value->fetch_lazy ();
+  *optimizedp
+    = value->bits_any_optimized_out (value->offset () * 8,
+				     copy_len * 8);
+  *unavailablep
+    = !value->bytes_available (value->offset (), copy_len);
+
   *lvalp = value->lval ();
   *addrp = value->address ();
   if (*lvalp == lval_register)
@@ -1204,13 +1235,15 @@  frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
 
   if (!buffer.empty ())
     {
-      gdb_assert (buffer.size () >= value->type ()->length ());
-
       if (!*optimizedp && !*unavailablep)
-	memcpy (buffer.data (), value->contents_all ().data (),
-		value->type ()->length ());
+	{
+	  auto value_part
+	    = value->contents_all ().slice (value->offset (), copy_len);
+
+	  memcpy (buffer.data (), value_part.data (), copy_len);
+	}
       else
-	memset (buffer.data (), 0, value->type ()->length ());
+	memset (buffer.data (), 0, copy_len);
     }
 
   /* Dispose of the new value.  This prevents watchpoints from