[v2] 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 |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Test passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
Commit Message
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, after which the assert triggers
because:
- buffer.size () == 8 (the size of r11), and
- value->type ()->length () == 16 (the size of the type of v0).
Fix this in frame_unwind_got_register, by returning a value with the type of
r11 instead.
Tested on s390x-linux and x86_64-linux.
---
gdb/frame-unwind.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
base-commit: baac6c221e9d69335bf41366a1c7d87d8ab2f893
Comments
Tom de Vries <tdevries@suse.de> writes:
> 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, after which the assert triggers
> because:
> - buffer.size () == 8 (the size of r11), and
> - value->type ()->length () == 16 (the size of the type of v0).
>
> Fix this in frame_unwind_got_register, by returning a value with the type of
> r11 instead.
>
> Tested on s390x-linux and x86_64-linux.
> ---
> gdb/frame-unwind.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index 6c6ca46be43..39e84c45ac3 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -278,8 +278,9 @@ struct value *
> frame_unwind_got_register (const frame_info_ptr &frame,
> int regnum, int new_regnum)
> {
> - return value_of_register_lazy (get_next_frame_sentinel_okay (frame),
> - new_regnum);
> + struct gdbarch *gdbarch = frame_unwind_arch (frame);
Just FYI my first impulse was that this should be:
struct gdbarch *gdbarch = get_frame_arch (frame);
but:
1. The other frame_unwind_got_* functions also use
frame_unwind_arch (frame).
2. Those functions originally used get_frame_arch (frame) but Ulrich
Weigand changed to frame_unwind_arch in 2009.
So this is correct.
> + struct type *type = register_type (gdbarch, regnum);
> + return value_from_register (type, new_regnum, frame);
> }
>
> /* Return a value which indicates that FRAME saved REGNUM in memory at
>
> base-commit: baac6c221e9d69335bf41366a1c7d87d8ab2f893
Looks good to me. Thanks!
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
--
Thiago
On 2025-01-15 19:41, Thiago Jung Bauermann wrote:
> Tom de Vries <tdevries@suse.de> writes:
>
>> 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, after which the assert triggers
>> because:
>> - buffer.size () == 8 (the size of r11), and
>> - value->type ()->length () == 16 (the size of the type of v0).
>>
>> Fix this in frame_unwind_got_register, by returning a value with the type of
>> r11 instead.
>>
>> Tested on s390x-linux and x86_64-linux.
>> ---
>> gdb/frame-unwind.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
>> index 6c6ca46be43..39e84c45ac3 100644
>> --- a/gdb/frame-unwind.c
>> +++ b/gdb/frame-unwind.c
>> @@ -278,8 +278,9 @@ struct value *
>> frame_unwind_got_register (const frame_info_ptr &frame,
>> int regnum, int new_regnum)
>> {
>> - return value_of_register_lazy (get_next_frame_sentinel_okay (frame),
>> - new_regnum);
>> + struct gdbarch *gdbarch = frame_unwind_arch (frame);
>
> Just FYI my first impulse was that this should be:
>
> struct gdbarch *gdbarch = get_frame_arch (frame);
>
> but:
>
> 1. The other frame_unwind_got_* functions also use
> frame_unwind_arch (frame).
>
> 2. Those functions originally used get_frame_arch (frame) but Ulrich
> Weigand changed to frame_unwind_arch in 2009.
>
> So this is correct.
Yes, I think it is correct. In this function, FRAME is really the "next
frame", the frame that has saved the previous frame's value of register
REGNUM. We're trying to figure out the value of register REGNUM in
FRAME's previous frame. So it makes sense to use the arch of FRAME's
previous frame to get the type of register REGNUM.
>> + struct type *type = register_type (gdbarch, regnum);
Other `frame_unwind_got_*` functions create a value with type
`register_type (gdbarch, regnum)`, so it gives me more confidence that
this is correct.
>> + return value_from_register (type, new_regnum, frame);
There's a change in behavior, we used to create a lazy value at this
point, and now the register contents will be fetched here. Is this a
problem?
Simon
On 1/16/25 16:15, Simon Marchi wrote:
>
>
> On 2025-01-15 19:41, Thiago Jung Bauermann wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> 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, after which the assert triggers
>>> because:
>>> - buffer.size () == 8 (the size of r11), and
>>> - value->type ()->length () == 16 (the size of the type of v0).
>>>
>>> Fix this in frame_unwind_got_register, by returning a value with the type of
>>> r11 instead.
>>>
>>> Tested on s390x-linux and x86_64-linux.
>>> ---
>>> gdb/frame-unwind.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
>>> index 6c6ca46be43..39e84c45ac3 100644
>>> --- a/gdb/frame-unwind.c
>>> +++ b/gdb/frame-unwind.c
>>> @@ -278,8 +278,9 @@ struct value *
>>> frame_unwind_got_register (const frame_info_ptr &frame,
>>> int regnum, int new_regnum)
>>> {
>>> - return value_of_register_lazy (get_next_frame_sentinel_okay (frame),
>>> - new_regnum);
>>> + struct gdbarch *gdbarch = frame_unwind_arch (frame);
>>
>> Just FYI my first impulse was that this should be:
>>
>> struct gdbarch *gdbarch = get_frame_arch (frame);
>>
>> but:
>>
>> 1. The other frame_unwind_got_* functions also use
>> frame_unwind_arch (frame).
>>
>> 2. Those functions originally used get_frame_arch (frame) but Ulrich
>> Weigand changed to frame_unwind_arch in 2009.
>>
>> So this is correct.
>
> Yes, I think it is correct. In this function, FRAME is really the "next
> frame", the frame that has saved the previous frame's value of register
> REGNUM. We're trying to figure out the value of register REGNUM in
> FRAME's previous frame. So it makes sense to use the arch of FRAME's
> previous frame to get the type of register REGNUM.
>
>>> + struct type *type = register_type (gdbarch, regnum);
>
> Other `frame_unwind_got_*` functions create a value with type
> `register_type (gdbarch, regnum)`, so it gives me more confidence that
> this is correct.
>
>>> + return value_from_register (type, new_regnum, frame);
>
> There's a change in behavior, we used to create a lazy value at this
> point, and now the register contents will be fetched here. Is this a
> problem?
>
Hi Simon,
thanks for the review.
It doesn't seem to be a problem on the two architectures that I tested.
If it turns out that there's a problem, we can try to add a "bool
lazy_p" parameter to value_from_register.
But I'm not aware that we exploit this laziness in any way.
Thanks,
- Tom
On 1/16/25 16:59, Tom de Vries wrote:
> On 1/16/25 16:15, Simon Marchi wrote:
>>
>>
>> On 2025-01-15 19:41, Thiago Jung Bauermann wrote:
>>> Tom de Vries <tdevries@suse.de> writes:
>>>
>>>> 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, after which the assert
>>>> triggers
>>>> because:
>>>> - buffer.size () == 8 (the size of r11), and
>>>> - value->type ()->length () == 16 (the size of the type of v0).
>>>>
>>>> Fix this in frame_unwind_got_register, by returning a value with the
>>>> type of
>>>> r11 instead.
>>>>
>>>> Tested on s390x-linux and x86_64-linux.
>>>> ---
>>>> gdb/frame-unwind.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
>>>> index 6c6ca46be43..39e84c45ac3 100644
>>>> --- a/gdb/frame-unwind.c
>>>> +++ b/gdb/frame-unwind.c
>>>> @@ -278,8 +278,9 @@ struct value *
>>>> frame_unwind_got_register (const frame_info_ptr &frame,
>>>> int regnum, int new_regnum)
>>>> {
>>>> - return value_of_register_lazy (get_next_frame_sentinel_okay (frame),
>>>> - new_regnum);
>>>> + struct gdbarch *gdbarch = frame_unwind_arch (frame);
>>>
>>> Just FYI my first impulse was that this should be:
>>>
>>> struct gdbarch *gdbarch = get_frame_arch (frame);
>>>
>>> but:
>>>
>>> 1. The other frame_unwind_got_* functions also use
>>> frame_unwind_arch (frame).
>>>
>>> 2. Those functions originally used get_frame_arch (frame) but Ulrich
>>> Weigand changed to frame_unwind_arch in 2009.
>>>
>>> So this is correct.
>>
>> Yes, I think it is correct. In this function, FRAME is really the "next
>> frame", the frame that has saved the previous frame's value of register
>> REGNUM. We're trying to figure out the value of register REGNUM in
>> FRAME's previous frame. So it makes sense to use the arch of FRAME's
>> previous frame to get the type of register REGNUM.
>>
>>>> + struct type *type = register_type (gdbarch, regnum);
>>
>> Other `frame_unwind_got_*` functions create a value with type
>> `register_type (gdbarch, regnum)`, so it gives me more confidence that
>> this is correct.
>>
>>>> + return value_from_register (type, new_regnum, frame);
>>
>> There's a change in behavior, we used to create a lazy value at this
>> point, and now the register contents will be fetched here. Is this a
>> problem?
>>
>
> Hi Simon,
>
> thanks for the review.
>
> It doesn't seem to be a problem on the two architectures that I tested.
>
> If it turns out that there's a problem, we can try to add a "bool
> lazy_p" parameter to value_from_register.
>
> But I'm not aware that we exploit this laziness in any way.
Hi,
Is this approach ok, or should I submit a version that keeps the value lazy?
Thanks,
- Tom
On 1/23/25 2:59 AM, Tom de Vries wrote:
> On 1/16/25 16:59, Tom de Vries wrote:
>> On 1/16/25 16:15, Simon Marchi wrote:
>>>
>>>
>>> On 2025-01-15 19:41, Thiago Jung Bauermann wrote:
>>>> Tom de Vries <tdevries@suse.de> writes:
>>>>
>>>>> 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, after which the assert triggers
>>>>> because:
>>>>> - buffer.size () == 8 (the size of r11), and
>>>>> - value->type ()->length () == 16 (the size of the type of v0).
>>>>>
>>>>> Fix this in frame_unwind_got_register, by returning a value with the type of
>>>>> r11 instead.
>>>>>
>>>>> Tested on s390x-linux and x86_64-linux.
>>>>> ---
>>>>> gdb/frame-unwind.c | 5 +++--
>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
>>>>> index 6c6ca46be43..39e84c45ac3 100644
>>>>> --- a/gdb/frame-unwind.c
>>>>> +++ b/gdb/frame-unwind.c
>>>>> @@ -278,8 +278,9 @@ struct value *
>>>>> frame_unwind_got_register (const frame_info_ptr &frame,
>>>>> int regnum, int new_regnum)
>>>>> {
>>>>> - return value_of_register_lazy (get_next_frame_sentinel_okay (frame),
>>>>> - new_regnum);
>>>>> + struct gdbarch *gdbarch = frame_unwind_arch (frame);
>>>>
>>>> Just FYI my first impulse was that this should be:
>>>>
>>>> struct gdbarch *gdbarch = get_frame_arch (frame);
>>>>
>>>> but:
>>>>
>>>> 1. The other frame_unwind_got_* functions also use
>>>> frame_unwind_arch (frame).
>>>>
>>>> 2. Those functions originally used get_frame_arch (frame) but Ulrich
>>>> Weigand changed to frame_unwind_arch in 2009.
>>>>
>>>> So this is correct.
>>>
>>> Yes, I think it is correct. In this function, FRAME is really the "next
>>> frame", the frame that has saved the previous frame's value of register
>>> REGNUM. We're trying to figure out the value of register REGNUM in
>>> FRAME's previous frame. So it makes sense to use the arch of FRAME's
>>> previous frame to get the type of register REGNUM.
>>>
>>>>> + struct type *type = register_type (gdbarch, regnum);
>>>
>>> Other `frame_unwind_got_*` functions create a value with type
>>> `register_type (gdbarch, regnum)`, so it gives me more confidence that
>>> this is correct.
>>>
>>>>> + return value_from_register (type, new_regnum, frame);
>>>
>>> There's a change in behavior, we used to create a lazy value at this
>>> point, and now the register contents will be fetched here. Is this a
>>> problem?
>>>
>>
>> Hi Simon,
>>
>> thanks for the review.
>>
>> It doesn't seem to be a problem on the two architectures that I tested.
>>
>> If it turns out that there's a problem, we can try to add a "bool lazy_p" parameter to value_from_register.
>>
>> But I'm not aware that we exploit this laziness in any way.
>
> Hi,
>
> Is this approach ok, or should I submit a version that keeps the value lazy?
>
> Thanks,
> - Tom
>
I guess it's not such a big deal, but it still feels like a regression
in terms of efficiency. It seems nice if values are created lazy by
default, and their contents only fetched when needed.
The only spot I am aware where the content of the resulting value isn't
needed is the "info frame" command (function info_frame_command_core).
So depending on how difficult it is to implement, I think it would be
nice to add the laziness back.
Simon
@@ -278,8 +278,9 @@ struct value *
frame_unwind_got_register (const frame_info_ptr &frame,
int regnum, int new_regnum)
{
- return value_of_register_lazy (get_next_frame_sentinel_okay (frame),
- new_regnum);
+ struct gdbarch *gdbarch = frame_unwind_arch (frame);
+ struct type *type = register_type (gdbarch, regnum);
+ return value_from_register (type, new_regnum, frame);
}
/* Return a value which indicates that FRAME saved REGNUM in memory at