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

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

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.

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