[PATCH] Return the regnum for PC (32) on aarch64
Luis Machado
luis.machado@arm.com
Mon May 16 13:29:16 GMT 2022
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.
More information about the Gdb-patches
mailing list