Re: [PATCH] Return the regnum for PC (32) on aarch64

[Luis Machado <luis.machado@arm.com>]

Hi,

Thanks for the patch. It looks good to me. There are just some 
adjustments/suggestions on the testcase itself.

> This will allow the unwind info to explicitly specify a different value
> for the return address than the link register. Such usage, e.g. for signal frames,
> is mentioned in aadwarf64 from ARM.
> 
> Ref https://sourceware.org/pipermail/gdb/2022-May/050091.html
> ---
>  gdb/aarch64-tdep.c                           |  3 +
>  gdb/aarch64-tdep.h                           |  1 +
>  gdb/testsuite/gdb.arch/aarch64-unwind-pc.S   | 48 +++++++++++++++
>  gdb/testsuite/gdb.arch/aarch64-unwind-pc.exp | 63 ++++++++++++++++++++
>  4 files changed, 115 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.arch/aarch64-unwind-pc.S
>  create mode 100644 gdb/testsuite/gdb.arch/aarch64-unwind-pc.exp
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 9d06ebfe27c..f9eb455232f 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -2219,6 +2219,9 @@ aarch64_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>    if (reg == AARCH64_DWARF_SP)
>      return AARCH64_SP_REGNUM;
>  
> +  if (reg == AARCH64_DWARF_PC)
> +    return AARCH64_PC_REGNUM;
> +
>    if (reg >= AARCH64_DWARF_V0 && reg <= AARCH64_DWARF_V0 + 31)
>      return AARCH64_V0_REGNUM + reg - AARCH64_DWARF_V0;
>  
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index e4cdebb6311..aa1bedbdac6 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -34,6 +34,7 @@ struct regset;
>  /* AArch64 Dwarf register numbering.  */
>  #define AARCH64_DWARF_X0   0
>  #define AARCH64_DWARF_SP  31
> +#define AARCH64_DWARF_PC  32
>  #define AARCH64_DWARF_PAUTH_RA_STATE  34
>  #define AARCH64_DWARF_PAUTH_DMASK  35
>  #define AARCH64_DWARF_PAUTH_CMASK  36
> diff --git a/gdb/testsuite/gdb.arch/aarch64-unwind-pc.S b/gdb/testsuite/gdb.arch/aarch64-unwind-pc.S
> new file mode 100644
> index 00000000000..8589a64fd56
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-unwind-pc.S
> @@ -0,0 +1,48 @@
> +/* Copyright 2022-2022 Free Software Foundation, Inc.

2022-2022 -> 2022

> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +        .text
> +        .globl main
> +        .type main,#function
> +main:
> +        .cfi_startproc
> +        stp     x29, x30, [sp, -16]!
> +        .cfi_def_cfa sp, 16
> +        .cfi_offset x29, 0
> +        .cfi_offset x30, 8
> +        bl test_func
> +        ldp     x29, x30, [sp], 16
> +        .cfi_restore x29
> +        .cfi_restore x30
> +        .cfi_def_cfa sp, 0
> +        mov     x0, 0
> +        ret
> +        .cfi_endproc
> +
> +        .globl test_func
> +test_func:
> +        .cfi_startproc
> +        // Unwind x30 to a different value
> +        // CFA_val_expression x30 const2u 0x1234
> +        .cfi_escape 0x16, 30, 0x03, 0x0a, 0x34, 0x12
> +        // CFA_val_expression pc breg30 0
> +        .cfi_escape 0x16, 32, 0x02, 0x8e, 0x00
> +        mov     x0, x30
> +        .cfi_register 32, x0
> +        mov     x30, 0x1234
> +        ret     x0
> +        .cfi_endproc
> diff --git a/gdb/testsuite/gdb.arch/aarch64-unwind-pc.exp b/gdb/testsuite/gdb.arch/aarch64-unwind-pc.exp
> new file mode 100644
> index 00000000000..c047c7d2f1a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-unwind-pc.exp
> @@ -0,0 +1,63 @@
> +# Copyright 2022-2022 Free Software Foundation, Inc.

2022-2022 -> 2022

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This file is part of the gdb testsuite.
> +
> +# Test explicitly unwinding PC on aarch64

How about...

"Test explicitly unwinding the PC DWARF register on aarch64"?

> +
> +if {![is_aarch64_target]} then {
> +    verbose "Skipping arm displaced stepping tests."
> +    return
> +}
> +

Copy/paste problem possibly? The usual way to report this is:

verbose "Skipping ${gdb_test_file_name}."

> +standard_testfile .S
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +proc test_reg_vals { i } {
> +    gdb_test "p \$pc - &main" "= 8" "p \$pc, $i"
> +    gdb_test "p \$x30" "= 4660" "p \$x30, $i"
> +}

I'd drop the argument to the proc. We can surround the proc call in a 
prefix instead. See below.

> +
> +proc test_unwind_pc { i } {
> +    gdb_test "si" "" "single step, $i"

Instead of passing regardless of the output, can we validate that we 
really stepped things correctly?

> +    gdb_test "backtrace" \
> +	".*#1.*in main ().*" \
> +	"backtrace, $i"
> +    gdb_test "up" "" "parent frame, $i"
> +    test_reg_vals "$i"
> +}

Drop the proc argument as well and use a prefix instead. See below.

> +
> +# Ready to enter the function
> +gdb_test "si" "bl test_func" "call site"
> +# Step through the 3 instructions in the function to make sure that
> +# we have the same unwind info throughout.
> +test_unwind_pc 1
> +test_unwind_pc 2
> +test_unwind_pc 3

You can make these like so, for example...

with_test_prefix "1st stepi" {
     test_unwind_pc
}

with_test_prefix "2nd stepi" {
     test_unwind_pc
}

with_test_prefix "3rd stepi" {
     test_unwind_pc
}


> +# Check again after we returned
> +gdb_test "si" "" "single step out"

Same as before. Instead of passing regardless of the output, can we 
validate that we really stepped out? This would make the test more robust.

> +gdb_test "backtrace" \
> +    "#0\[\t \]+main ().*" \
> +    "backtrace, 4"
> +test_reg_vals 4
> +

Given this is the final check, do we need to number it? We could just...

+with_test_prefix "final" {
+    gdb_test "backtrace" \
+        "#0\[\t \]+main ().*" \
+        "backtrace"
+    test_reg_vals
+}


> +gdb_continue_to_end "aarch64-unwind-pc"

At this point we've already exercised what we wanted, right? Is the 
above test needed?