Re: [PATCH] Fix PR30369 regression on aarch64/arm (PR30506)

[Tom de Vries <tdevries@suse.de>]

On 6/7/23 11:40, Luis Machado wrote:
> From: Tom de Vries <tdevries@suse.de>
> 
> The gdb.dwarf2/dw2-prologue-end-2.exp test was failing for both AArch64 and
> Arm.
> 
> As Tom pointed out here (https://inbox.sourceware.org/gdb-patches/6663707c-4297-c2f2-a0bd-f3e84fc62aad@suse.de/),
> there are issues with both the prologue skipper for AArch64 and Arm and an
> incorrect assumption by the testcase.
> 
> This patch fixes both of AArch64's and Arm's prologue skippers to not skip past
> the end of a function.  It also incorporates a fix to the testcase so it
> doesn't assume the prologue skipper will stop at the first instruction of the
> functions/labels.
> 
> Regression-tested on aarch64-linux/arm-linux Ubuntu 20.04/22.04 and
> x86_64-linux Ubuntu 20.04.
> 

Hi Luis,

thanks for picking this up.

LGTM.

Thanks,
- Tom

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30506
> 
> Co-Authored-By: Tom de Vries <tdevries@suse.de>
> Co-Authored-By: Luis Machado <luis.machado@arm.com>
> ---
>   gdb/aarch64-tdep.c                            | 10 +++++++--
>   gdb/arm-tdep.c                                | 21 ++++++++++++++++---
>   .../gdb.dwarf2/dw2-prologue-end-2.exp         | 10 ++++-----
>   3 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index d8603c45fd3..84a90b63b55 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -917,12 +917,15 @@ aarch64_analyze_prologue_test (void)
>   static CORE_ADDR
>   aarch64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>   {
> -  CORE_ADDR func_addr, limit_pc;
> +  CORE_ADDR func_addr, func_end_addr, limit_pc;
>   
>     /* See if we can determine the end of the prologue via the symbol
>        table.  If so, then return either PC, or the PC after the
>        prologue, whichever is greater.  */
> -  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
> +  bool func_addr_found
> +    = find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr);
> +
> +  if (func_addr_found)
>       {
>         CORE_ADDR post_prologue_pc
>   	= skip_prologue_using_sal (gdbarch, func_addr);
> @@ -942,6 +945,9 @@ aarch64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>     if (limit_pc == 0)
>       limit_pc = pc + 128;	/* Magic.  */
>   
> +  limit_pc
> +    = func_end_addr == 0? limit_pc : std::min (limit_pc, func_end_addr - 4);
> +
>     /* Try disassembling prologue.  */
>     return aarch64_analyze_prologue (gdbarch, pc, limit_pc, NULL);
>   }
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index d5128754f02..a0f59557072 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -1769,12 +1769,18 @@ arm_skip_stack_protector(CORE_ADDR pc, struct gdbarch *gdbarch)
>   static CORE_ADDR
>   arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>   {
> -  CORE_ADDR func_addr, limit_pc;
> +  CORE_ADDR func_addr, func_end_addr, limit_pc;
>   
>     /* See if we can determine the end of the prologue via the symbol table.
>        If so, then return either PC, or the PC after the prologue, whichever
>        is greater.  */
> -  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
> +  bool func_addr_found
> +    = find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr);
> +
> +  /* Whether the function is thumb mode or not.  */
> +  bool func_is_thumb = false;
> +
> +  if (func_addr_found)
>       {
>         CORE_ADDR post_prologue_pc
>   	= skip_prologue_using_sal (gdbarch, func_addr);
> @@ -1811,7 +1817,8 @@ arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>   	     associate prologue code with the opening brace; so this
>   	     lets us skip the first line if we think it is the opening
>   	     brace.  */
> -	  if (arm_pc_is_thumb (gdbarch, func_addr))
> +	  func_is_thumb = arm_pc_is_thumb (gdbarch, func_addr);
> +	  if (func_is_thumb)
>   	    analyzed_limit = thumb_analyze_prologue (gdbarch, func_addr,
>   						     post_prologue_pc, NULL);
>   	  else
> @@ -1837,6 +1844,14 @@ arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>     if (limit_pc == 0)
>       limit_pc = pc + 64;          /* Magic.  */
>   
> +  /* Set the correct adjustment based on whether the function is thumb mode or
> +     not.  We use it to get the address of the last instruction in the
> +     function (as opposed to the first address of the next function).  */
> +  CORE_ADDR adjustment = func_is_thumb? 2 : 4;
> +
> +  limit_pc
> +    = func_end_addr == 0? limit_pc : std::min (limit_pc,
> +					       func_end_addr - adjustment);
>   
>     /* Check if this is Thumb code.  */
>     if (arm_pc_is_thumb (gdbarch, pc))
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
> index 488f85f9674..c506cfd55cc 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
> @@ -95,15 +95,15 @@ if { $break_addr == "" } {
>   
>   # Get the "foo_label" address.
>   
> -set foo_label_addr ""
> -gdb_test_multiple "print /x &foo_label" "" {
> +set bar_label_addr ""
> +gdb_test_multiple "print /x &bar_label" "" {
>       -re -wrap "= ($hex)" {
> -	set foo_label_addr $expect_out(1,string)
> +	set bar_label_addr $expect_out(1,string)
>   	pass $gdb_test_name
>       }
>   }
>   
> -if { $foo_label_addr == "" } {
> +if { $bar_label_addr == "" } {
>       return
>   }
>   
> @@ -115,4 +115,4 @@ gdb_test "print &foo_end == &bar_label" " = 1"
>   # Check that the breakpoint is set at the expected address.  Regression test
>   # for PR30369.
>   
> -gdb_assert { $break_addr == $foo_label_addr }
> +gdb_assert { $break_addr < $bar_label_addr }