From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x133.google.com (mail-lf1-x133.google.com [IPv6:2a00:1450:4864:20::133]) by sourceware.org (Postfix) with ESMTPS id 09E3E385380D for ; Mon, 16 May 2022 14:10:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 09E3E385380D Received: by mail-lf1-x133.google.com with SMTP id t25so26017705lfg.7 for ; Mon, 16 May 2022 07:10:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=49nx8e/NiJEZmogl57MT+ZUtV9J/7sNEEk/J9voe3tw=; b=aMfC+0/hiwPt/kktunoQirv0zKGBinSSfwJrUbtO9dltfJ+guXUZ0nDHozMaDt+wxR 55d2GkvHi04ZSoaNQWx2PbEVqOT64Ak7YVVTNYJAr5QWhBuEB7PuOmKhS64TK/P8jS8W BfAK47eubqp0csjnN9jDRurltxevNQ7MJviG282/3nCEkrl4MFxU/A9ouns26x1ikK99 8scZ1G6968+LChGC+moMefykmoRdX2+C9NJrrtPokQdVnfoiBPyuyLA0Pz/btlzPbkr/ BW5N5Vx4tw2kUDDNKTvSSWWKCjvRfoA9R+lMlWNk6GpzFEqDCZSK4YI3gGd+K+cgR8p8 7Bvw== X-Gm-Message-State: AOAM5329EbMWF6IA1A5KZ3+7A3PvzMnpmnMEbAepsh8pd4vsZQ+6QjAd mCMrL6FR9GueKHNGZqyis4EmOZIh70X+mbDJAp8= X-Google-Smtp-Source: ABdhPJxfYgxItjjSGcFHUmN131X0kSHN0+cyTrnRSuCh5r/17HMkp730Or8MkV3aKVr9viBPM4Pe1lw/vP8D8ltHHl8= X-Received: by 2002:a19:6b19:0:b0:471:f39c:7d1d with SMTP id d25-20020a196b19000000b00471f39c7d1dmr13273579lfa.443.1652710251107; Mon, 16 May 2022 07:10:51 -0700 (PDT) MIME-Version: 1.0 References: <20220513122737.2159359-1-yyc1992@gmail.com> <0a776760-d3b9-4007-8383-c45f1520e6ba@arm.com> In-Reply-To: From: Yichao Yu Date: Mon, 16 May 2022 10:10:39 -0400 Message-ID: Subject: Re: [PATCH] Return the regnum for PC (32) on aarch64 To: Luis Machado Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, GIT_PATCH_0, KAM_NUMSUBJECT, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 May 2022 14:10:55 -0000 On Mon, May 16, 2022 at 9:29 AM Luis Machado wrote: > > On 5/16/22 14:17, Yichao Yu wrote: > > On Mon, May 16, 2022 at 5:05 AM Luis Machado 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 . */ > >>> + > >>> + .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 . > >>> + > >>> +# 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.