[v3] gdb: fix "frame function" failure to get frame when call is it's last instruction
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
"frame function" currently fails to retrieve the frame
associated with a function when the last instruction
in the frame is a call. This is because the instruction
pointer points to an address that lies outside the frame.
Using "get_frame_address_in_block" instead of "get_frame_pc"
resolves this issue.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30929
---
Changes v2 -> v3:
* frame-selection.exp: Merge with frame-selection-last-instr-call.exp.
* frame-selection.c: Merge with frame-selection-last-instr-call.c.
* frame-selection-last-instr-call.exp: Remove this.
* frame-selection-last-instr-call.c: Likewise.
gdb/stack.c | 4 +-
gdb/testsuite/gdb.base/frame-selection.c | 14 +++--
gdb/testsuite/gdb.base/frame-selection.exp | 67 +++++++++++++++++++++-
3 files changed, 77 insertions(+), 8 deletions(-)
Comments
On 7/4/24 1:33 PM, Sahil Siddiq wrote:
> "frame function" currently fails to retrieve the frame
> associated with a function when the last instruction
> in the frame is a call. This is because the instruction
> pointer points to an address that lies outside the frame.
>
> Using "get_frame_address_in_block" instead of "get_frame_pc"
> resolves this issue.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30929
> ---
Hi!
Thanks for the quick turnaround on this!
I have tested, it fixes the issue reported and adds no regressions! I
have a minor nit inlined, but with that fixes, feel free to add my git
trailer to the commit message:
Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
I hope a maintainer approves this for pushing soon!
> Changes v2 -> v3:
> * frame-selection.exp: Merge with frame-selection-last-instr-call.exp.
> * frame-selection.c: Merge with frame-selection-last-instr-call.c.
> * frame-selection-last-instr-call.exp: Remove this.
> * frame-selection-last-instr-call.c: Likewise.
>
> gdb/stack.c | 4 +-
> gdb/testsuite/gdb.base/frame-selection.c | 14 +++--
> gdb/testsuite/gdb.base/frame-selection.exp | 67 +++++++++++++++++++++-
> 3 files changed, 77 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/stack.c b/gdb/stack.c
> index b36193be2f..8249866468 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -2860,8 +2860,8 @@ find_frame_for_function (const char *function_name)
> do
> {
> for (size_t i = 0; (i < sals.size () && !found); i++)
> - found = (get_frame_pc (frame) >= func_bounds[i].low
> - && get_frame_pc (frame) < func_bounds[i].high);
> + found = (get_frame_address_in_block (frame) >= func_bounds[i].low
> + && get_frame_address_in_block (frame) < func_bounds[i].high);
> if (!found)
> {
> level = 1;
> diff --git a/gdb/testsuite/gdb.base/frame-selection.c b/gdb/testsuite/gdb.base/frame-selection.c
> index 237e155b8c..f7893388ae 100644
> --- a/gdb/testsuite/gdb.base/frame-selection.c
> +++ b/gdb/testsuite/gdb.base/frame-selection.c
> @@ -40,13 +40,17 @@ recursive (int arg)
> return v;
> }
>
> +void
> +call_abort (void)
> +{
> + __builtin_abort();
> +}
> +
> int
> main (void)
> {
> - int i, j;
> -
> - i = frame_1 ();
> - j = recursive (0);
> + frame_1 ();
> + recursive (0);
>
> - return i + j;
> + call_abort();
> }
> diff --git a/gdb/testsuite/gdb.base/frame-selection.exp b/gdb/testsuite/gdb.base/frame-selection.exp
> index e8d9c87c3a..4333f6a2d1 100644
> --- a/gdb/testsuite/gdb.base/frame-selection.exp
> +++ b/gdb/testsuite/gdb.base/frame-selection.exp
> @@ -63,7 +63,7 @@ proc check_frame { level address function } {
>
> set re [multi_line \
> "Stack level ${level}, frame at ($address):" \
> - ".* = $hex in ${function} \(\[^\r\n\]*\); saved .* = $hex" \
> + ".* = $hex in ${function}( \(\[^\r\n\]*\))*; saved .* = $hex" \
This looks like a spurious change. I removed the change and everything
works.
There's no need to send a new version for just this change though, just
fix it locally and apply my review tag :)
Hi,
Thank you for the review.
On Friday, July 5, 2024 6:36:52 PM GMT+5:30 Guinevere Larsen wrote:
> [...]
> I have tested, it fixes the issue reported and adds no regressions! I
> have a minor nit inlined, but with that fixes, feel free to add my git
> trailer to the commit message:
>
> Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
>
> I hope a maintainer approves this for pushing soon!
>
> [...]
> > diff --git a/gdb/testsuite/gdb.base/frame-selection.exp
> > b/gdb/testsuite/gdb.base/frame-selection.exp index e8d9c87c3a..4333f6a2d1
> > 100644
> > --- a/gdb/testsuite/gdb.base/frame-selection.exp
> > +++ b/gdb/testsuite/gdb.base/frame-selection.exp
> > @@ -63,7 +63,7 @@ proc check_frame { level address function } {
> >
> > set re [multi_line \
> >
> > "Stack level ${level}, frame at ($address):" \
> >
> > - ".* = $hex in ${function} \(\[^\r\n\]*\); saved .* = $hex" \
> > + ".* = $hex in ${function}( \(\[^\r\n\]*\))*; saved .* = $hex" \
>
> This looks like a spurious change. I removed the change and everything
> works.
>
> There's no need to send a new version for just this change though, just
> fix it locally and apply my review tag :)
>
Since this is my first time contributing to gdb, I am a little unsure
about what comes next.
My understanding is that a maintainer will respond to this thread
with an "Approved-By" tag and then I'll be able to push this patch
to the repository with the "Reviewed-By" and "Approved-By" tags
appended in the commit message. Is this correct?
Thanks,
Sahil
On 7/5/24 11:23 AM, Sahil wrote:
> Hi,
>
> Thank you for the review.
>
> On Friday, July 5, 2024 6:36:52 PM GMT+5:30 Guinevere Larsen wrote:
>> [...]
>> I have tested, it fixes the issue reported and adds no regressions! I
>> have a minor nit inlined, but with that fixes, feel free to add my git
>> trailer to the commit message:
>>
>> Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
>>
>> I hope a maintainer approves this for pushing soon!
>>
>> [...]
>>> diff --git a/gdb/testsuite/gdb.base/frame-selection.exp
>>> b/gdb/testsuite/gdb.base/frame-selection.exp index e8d9c87c3a..4333f6a2d1
>>> 100644
>>> --- a/gdb/testsuite/gdb.base/frame-selection.exp
>>> +++ b/gdb/testsuite/gdb.base/frame-selection.exp
>>> @@ -63,7 +63,7 @@ proc check_frame { level address function } {
>>>
>>> set re [multi_line \
>>>
>>> "Stack level ${level}, frame at ($address):" \
>>>
>>> - ".* = $hex in ${function} \(\[^\r\n\]*\); saved .* = $hex" \
>>> + ".* = $hex in ${function}( \(\[^\r\n\]*\))*; saved .* = $hex" \
>> This looks like a spurious change. I removed the change and everything
>> works.
>>
>> There's no need to send a new version for just this change though, just
>> fix it locally and apply my review tag :)
>>
> Since this is my first time contributing to gdb, I am a little unsure
> about what comes next.
>
> My understanding is that a maintainer will respond to this thread
> with an "Approved-By" tag and then I'll be able to push this patch
> to the repository with the "Reviewed-By" and "Approved-By" tags
> appended in the commit message. Is this correct?
>
> Thanks,
> Sahil
>
>
If you have completed the copyright assignment process, yes that's
exactly it. If you haven't, you'll need that finished before being
allowed to push, since I think the changes to the testcase would be
considered legally significant.
I'm not sure if you should start the assignment process yet, since the
patch hasn't been approved. Eli (CC'd because of this question), should
Sahil start the assignment process already, or wait for someone to
approve their patch first?
> Date: Fri, 5 Jul 2024 11:38:11 -0300
> Cc: Sahil Siddiq <sahilcdq@proton.me>,
> "Eli Zaretskii (eliz@gnu.org)" <eliz@gnu.org>
> From: Guinevere Larsen <blarsen@redhat.com>
>
> I'm not sure if you should start the assignment process yet, since the
> patch hasn't been approved. Eli (CC'd because of this question), should
> Sahil start the assignment process already, or wait for someone to
> approve their patch first?
Sahil can start the process regardless of the approval. The copyright
assignment is for this and all the future contributions, so even if
this one eventually doesn't go in, the assignment is good and valid
for future ones.
Thanks.
Hi,
On Friday, July 5, 2024 8:08:11 PM GMT+5:30 Guinevere Larsen wrote:
> [...]
> If you have completed the copyright assignment process, yes that's
> exactly it. If you haven't, you'll need that finished before being
> allowed to push, since I think the changes to the testcase would be
> considered legally significant.
On Friday, July 5, 2024 11:46:47 PM GMT+5:30 Eli Zaretskii wrote:
> > Date: Fri, 5 Jul 2024 11:38:11 -0300
> > Cc: Sahil Siddiq <sahilcdq@proton.me>,
> >
> > "Eli Zaretskii (eliz@gnu.org)" <eliz@gnu.org>
> >
> > From: Guinevere Larsen <blarsen@redhat.com>
> >
> > I'm not sure if you should start the assignment process yet, since the
> > patch hasn't been approved. Eli (CC'd because of this question), should
> > Sahil start the assignment process already, or wait for someone to
> > approve their patch first?
>
> Sahil can start the process regardless of the approval. The copyright
> assignment is for this and all the future contributions, so even if
> this one eventually doesn't go in, the assignment is good and valid
> for future ones.
Thank you for the clarifications. I'll start the process.
Regards,
Sahil
@@ -2860,8 +2860,8 @@ find_frame_for_function (const char *function_name)
do
{
for (size_t i = 0; (i < sals.size () && !found); i++)
- found = (get_frame_pc (frame) >= func_bounds[i].low
- && get_frame_pc (frame) < func_bounds[i].high);
+ found = (get_frame_address_in_block (frame) >= func_bounds[i].low
+ && get_frame_address_in_block (frame) < func_bounds[i].high);
if (!found)
{
level = 1;
@@ -40,13 +40,17 @@ recursive (int arg)
return v;
}
+void
+call_abort (void)
+{
+ __builtin_abort();
+}
+
int
main (void)
{
- int i, j;
-
- i = frame_1 ();
- j = recursive (0);
+ frame_1 ();
+ recursive (0);
- return i + j;
+ call_abort();
}
@@ -63,7 +63,7 @@ proc check_frame { level address function } {
set re [multi_line \
"Stack level ${level}, frame at ($address):" \
- ".* = $hex in ${function} \(\[^\r\n\]*\); saved .* = $hex" \
+ ".* = $hex in ${function}( \(\[^\r\n\]*\))*; saved .* = $hex" \
".*\r\n$gdb_prompt $" ]
set testname "check frame level ${level}"
@@ -186,3 +186,68 @@ with_test_prefix "second frame_2 breakpoint" {
gdb_test "frame function recursive" "#1 $hex in recursive.*" \
"select frame for function recursive, third attempt"
}
+
+with_test_prefix "call_is_last_instr_in_frame" {
+ gdb_breakpoint abort
+ gdb_continue_to_breakpoint abort
+
+ # The last instruction in frame "call_abort" is a call
+ gdb_test "bt" \
+ [multi_line \
+ "#0 .*abort \[^\r\n\]*" \
+ "#1 $hex in call_abort \[^\r\n\]*" \
+ "#2 $hex in main \[^\r\n\]*"] \
+ "backtrace at breakpoint"
+
+
+ # Select frame using level, but relying on this being the default
+ # action, so "frame 0" performs "frame level 0".
+ gdb_test "frame 1" "#1 $hex in call_abort.*"
+ set frame_1_address [ get_frame_address "frame 1" ]
+ gdb_test "frame 2" "#2 $hex in main.*"
+ set frame_2_address [ get_frame_address "frame 2" ]
+
+ # Select frame using 'level' specification.
+ gdb_test "frame level 1" "#1 $hex in call_abort.*"
+ gdb_test "frame level 2" "#2 $hex in main.*"
+
+ # Select frame by address.
+ gdb_test "frame address ${frame_1_address}" "#1 $hex in call_abort.*" \
+ "select frame 1 by address"
+ gdb_test "frame address ${frame_2_address}" "#2 $hex in main.*" \
+ "select frame 2 by address"
+
+ # Select frame by function.
+ gdb_test "frame function call_abort" "#1 $hex in call_abort.*"
+ gdb_test "frame function main" "#2 $hex in main.*"
+
+ with_test_prefix "select-frame, no keyword" {
+ gdb_test_no_output "select-frame 1"
+ check_frame "1" "${frame_1_address}" "call_abort"
+ gdb_test_no_output "select-frame 2"
+ check_frame "2" "${frame_2_address}" "main"
+ }
+
+ with_test_prefix "select-frame, keyword=level" {
+ gdb_test_no_output "select-frame level 1"
+ check_frame "1" "${frame_1_address}" "call_abort"
+ gdb_test_no_output "select-frame level 2"
+ check_frame "2" "${frame_2_address}" "main"
+ }
+
+ with_test_prefix "select-frame, keyword=address" {
+ gdb_test_no_output "select-frame address ${frame_1_address}" \
+ "select frame 1 by address"
+ check_frame "1" "${frame_1_address}" "call_abort"
+ gdb_test_no_output "select-frame address ${frame_2_address}" \
+ "select frame 2 by address"
+ check_frame "2" "${frame_2_address}" "main"
+ }
+
+ with_test_prefix "select-frame, keyword=function" {
+ gdb_test_no_output "select-frame function call_abort"
+ check_frame "1" "${frame_1_address}" "call_abort"
+ gdb_test_no_output "select-frame function main"
+ check_frame "2" "${frame_2_address}" "main"
+ }
+}