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

[Guinevere Larsen <blarsen@redhat.com>]

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"
> +    }
> +}