From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com [IPv6:2a00:1450:4864:20::12a]) by sourceware.org (Postfix) with ESMTPS id E7E5B3857363 for ; Mon, 16 May 2022 13:17:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E7E5B3857363 Received: by mail-lf1-x12a.google.com with SMTP id b18so25704445lfv.9 for ; Mon, 16 May 2022 06:17:13 -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=ynlrBRddbzXhZdPwaV0+mFvBdu0Ti789YpGLGh4jGe0=; b=VXjqRiPN+STacT4V2k9lDdGKcXLj8X+r8Ly0kle6g/lVfgugsx/zEbpHR2TuN55A3/ KN/lLI0AYeTHVWu7YFOgutpo1wLTJe5B9OaNwS0dj2FvvwCs6XfnwhYUkbypHV01aMhb 1AEkJOu+bSeEeFqGQnXAiXc3ZB5WJPfbtMnRNC46Mqt9UE/irXmyGus+NBc2/Wbk7aYD Ged92D1AjkEGmJeVHzeZ/tjonHuknNmc/dhv6OaFexeagl5RkGcxrEXquaWmO4NVCmDS vznDCuDabdiLVKO91k666wD2AJx0N+G2c5SYosWt2QLSMc4ZpL3mvLIOTa+cVa9Ziwal vJJw== X-Gm-Message-State: AOAM530zPF+elm/ENucuMn+tm6rSiY3wQgqNbgJmRI3XkFjQ0JNusaDX ZiriUA6zxQjHc73i98i3MfyNu20bFjbpBpy0GdTanvNg X-Google-Smtp-Source: ABdhPJzRTNBNbPVaFOMtzjtpNGjNH0vmz9p/rR3qaZeeY/35WDISFnVOZyY+88Kd/FdUSUyNx95qFcXzp+qT/mrigKM= X-Received: by 2002:a05:6512:2311:b0:473:a659:878b with SMTP id o17-20020a056512231100b00473a659878bmr13482411lfu.352.1652707032133; Mon, 16 May 2022 06:17:12 -0700 (PDT) MIME-Version: 1.0 References: <20220513122737.2159359-1-yyc1992@gmail.com> <0a776760-d3b9-4007-8383-c45f1520e6ba@arm.com> In-Reply-To: <0a776760-d3b9-4007-8383-c45f1520e6ba@arm.com> From: Yichao Yu Date: Mon, 16 May 2022 09:17:00 -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.5 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 13:17:16 -0000 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? > > + 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)