[PATCH v3] gdb: fix "frame function" failure to get frame when call is it's last instruction

[PATCH v3] gdb: fix "frame function" failure to get frame when call is it's last instruction

[Sahil Siddiq]

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

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" \
 		".*\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"
+    }
+}
-- 
2.45.2

Re: [PATCH v3] gdb: fix "frame function" failure to get frame when call is it's last instruction

[Guinevere Larsen]

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 :)

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>   		".*\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"
> +    }
> +}

Re: [PATCH v3] gdb: fix "frame function" failure to get frame when call is it's last instruction

[Sahil]

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

Re: [PATCH v3] gdb: fix "frame function" failure to get frame when call is it's last instruction

[Guinevere Larsen]

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?

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

Re: [PATCH v3] gdb: fix "frame function" failure to get frame when call is it's last instruction

[Eli Zaretskii]

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

Re: [PATCH v3] gdb: fix "frame function" failure to get frame when call is it's last instruction

[Sahil]

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

Re: [PATCH v3] gdb: fix "frame function" failure to get frame when call is it's last instruction

[Tom Tromey]

>>>>> Sahil Siddiq <icegambit91@gmail.com> writes:

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

I ran across this patch... whatever happened here?
Did you get a copyright assignment sorted out?

thanks,
Tom