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

[Yichao Yu <yyc1992@gmail.com>]

On Mon, May 16, 2022 at 9:29 AM Luis Machado <luis.machado@arm.com> wrote:
>
> On 5/16/22 14:17, Yichao Yu wrote:
> > On Mon, May 16, 2022 at 5:05 AM Luis Machado <luis.machado@arm.com> wrote:
> >>
> >> 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:
> >
> > Ooops...
> >
> >> verbose "Skipping ${gdb_test_file_name}."
> >
> > OK. I'll do that.
> >
> >>> +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?
> >
> > This is actually the main reason I included the frame number in the
> > backtrace test following this. I may be able to do something with the
> > one stepping out of the function but here each step only print the
> > instruction out so there's nothing common to test AFAICT.
> > Maybe I can pass in the expected current instruction?
> >
>
> Passing the expected pattern containing the instruction would work I think. That way we actually test that we stepped it correctly.
>
> Otherwise this particular test will PASS regardless of the outcome.

Did that in v2.

>
> >>> +    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?
> >
> > I was just following what the other tests did here. Is this
> > unnecessary? (also for one earlier version of the assembly code
> > continuing would crash since I trashed x30 and didn't restore it, not
> > sure if this line would have caught that though since I didn't
> > actually run that version though)
>
> If there is a reason we should exercise continuing until we finish the program, then it makes sense to keep this. But given this is
> just some custom assembly to exercise CFI/unwinding, I think the test should stop after we've verified unwinding works.

I kept this in v2, I can get rid of it in v3 if necessary.