From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25688 invoked by alias); 26 Sep 2018 15:11:20 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 25670 invoked by uid 89); 26 Sep 2018 15:11:19 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm1-f65.google.com Received: from mail-wm1-f65.google.com (HELO mail-wm1-f65.google.com) (209.85.128.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 26 Sep 2018 15:11:17 +0000 Received: by mail-wm1-f65.google.com with SMTP id y3-v6so1240820wma.1 for ; Wed, 26 Sep 2018 08:11:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Le37BGAfdHixYJvlk4gZ/yxsgVRrhaHNbSk4iWSl5kQ=; b=YQiXPx28Gc0yBtKHLV9l7E7clHaX8+ABvtVGopfkLPscGRMMVgldX1CLv+8vIOJvqr zUXZGm8JOC9rVChy4RxEQVks8uMlWOEYVpNn5y8VDqul27V6QgvZ8VBcEqeUPAj6PUPq JXnrCbAvHO1SE9C0ZzkyrZQWYyJ/9XDJ2TIScH5hzuPDo0h6s7deN0uEYlaqaCavWXDg qXzk4PUVuE9rgDIlI+pr1WzIPJV6JCsn60T/tPH/SsuoyvcdXfJCYXC9NI2yEkVpG+W6 9U1gGR/BpEuxHhFFtbwo11X1jpqmxZFcMo3ddLn7m0v6HzRRAf2e/TXxF0+jyKq+svhz SXGg== Return-Path: Received: from localhost (host81-154-73-63.range81-154.btcentralplus.com. [81.154.73.63]) by smtp.gmail.com with ESMTPSA id p11-v6sm7504571wrd.74.2018.09.26.08.11.13 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 26 Sep 2018 08:11:13 -0700 (PDT) Date: Wed, 26 Sep 2018 15:11:00 -0000 From: Andrew Burgess To: Joel Brobecker Cc: gdb-patches@sourceware.org, jimw@sifive.com, palmer@sifive.com Subject: Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding Message-ID: <20180926151112.GL5952@embecosm.com> References: <20180919164138.16480-1-andrew.burgess@embecosm.com> <20180926134916.GB8955@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180926134916.GB8955@adacore.com> X-Fortune: Fortune favors the lucky. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg00854.txt.bz2 * Joel Brobecker [2018-09-26 06:49:16 -0700]: > Hi Andrew, > > > This commit improves the prologue scanning stack unwinder, to better > > support AUIPC, LUI, and more variants of ADD and ADDI. > > > > This allows unwinding over frames containing large local variables, > > where the frame size does not fit into a single instruction immediate, > > and is first loaded into a temporary register, before being added to > > the stack pointer. > > > > A new test is added that tests this behaviour. As there's nothing > > truely RiscV specific about this test I've added it into gdb.base, but > > as this depends on target specific code to perform the unwind it is > > possible that some targets might fail this new test. > > > > gdb/ChangeLog: > > > > * riscv-tdep.c (riscv_insn::decode): Decode c.lui. > > (riscv_scan_prologue): Split handling of AUIPC, LUI, ADD, ADDI, > > and NOP. > > > > gdb/testsuite/ChangeLog: > > > > * gdb.base/large-frame-1.c: New file. > > * gdb.base/large-frame-2.c: New file. > > * gdb.base/large-frame.exp: New file. > > * gdb.base/large-frame.h: New file. > > I am OK with the patch. > > Are you sure you want to assert on the register number? What if > the instruction was malformed and the register number ended up > being larger than RISCV_NUM_INTEGER_REGS; is that possible? Well, it shouldn't happen in the sense that, for a given instruction, if it operates on integer registers then the space in the encoding should only allow for up to RISCV_NUM_INTEGER_REGS registers (some encodings allow for less registers). Right now I can't imagine a situation where one of those asserts could fire and it's not either a bug in GDB, or a bug in the libopcode field extraction code - hence an assert rather than an error. Thanks, Andrew > > > > --- > > gdb/ChangeLog | 6 ++++ > > gdb/riscv-tdep.c | 59 ++++++++++++++++++++++------------ > > gdb/testsuite/ChangeLog | 7 ++++ > > gdb/testsuite/gdb.base/large-frame-1.c | 32 ++++++++++++++++++ > > gdb/testsuite/gdb.base/large-frame-2.c | 25 ++++++++++++++ > > gdb/testsuite/gdb.base/large-frame.exp | 57 ++++++++++++++++++++++++++++++++ > > gdb/testsuite/gdb.base/large-frame.h | 24 ++++++++++++++ > > 7 files changed, 189 insertions(+), 21 deletions(-) > > create mode 100644 gdb/testsuite/gdb.base/large-frame-1.c > > create mode 100644 gdb/testsuite/gdb.base/large-frame-2.c > > create mode 100644 gdb/testsuite/gdb.base/large-frame.exp > > create mode 100644 gdb/testsuite/gdb.base/large-frame.h > > > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > > index 254914c9c7e..163c8ec231d 100644 > > --- a/gdb/riscv-tdep.c > > +++ b/gdb/riscv-tdep.c > > @@ -1227,7 +1227,11 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc) > > m_imm.s = EXTRACT_RVC_ADDI4SPN_IMM (ival); > > } > > else if (is_c_lui_insn (ival)) > > - m_opcode = OTHER; > > + { > > + m_opcode = LUI; > > + m_rd = decode_register_index (ival, OP_SH_CRS1S); > > + m_imm.s = EXTRACT_RVC_LUI_IMM (ival); > > + } > > /* C_SD and C_FSW have the same opcode. C_SD is RV64 and RV128 only, > > and C_FSW is RV32 only. */ > > else if (xlen != 4 && is_c_sd_insn (ival)) > > @@ -1359,28 +1363,41 @@ riscv_scan_prologue (struct gdbarch *gdbarch, > > gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS); > > regs[insn.rd ()] = pv_add_constant (regs[insn.rs1 ()], 0); > > } > > - else if ((insn.rd () == RISCV_GP_REGNUM > > - && (insn.opcode () == riscv_insn::AUIPC > > - || insn.opcode () == riscv_insn::LUI > > - || (insn.opcode () == riscv_insn::ADDI > > - && insn.rs1 () == RISCV_GP_REGNUM) > > - || (insn.opcode () == riscv_insn::ADD > > - && (insn.rs1 () == RISCV_GP_REGNUM > > - || insn.rs2 () == RISCV_GP_REGNUM)))) > > - || (insn.opcode () == riscv_insn::ADDI > > - && insn.rd () == RISCV_ZERO_REGNUM > > - && insn.rs1 () == RISCV_ZERO_REGNUM > > - && insn.imm_signed () == 0)) > > + else if ((insn.opcode () == riscv_insn::ADDI > > + && insn.rd () == RISCV_ZERO_REGNUM > > + && insn.rs1 () == RISCV_ZERO_REGNUM > > + && insn.imm_signed () == 0)) > > { > > - /* Handle: auipc gp, n > > - or: addi gp, gp, n > > - or: add gp, gp, reg > > - or: add gp, reg, gp > > - or: lui gp, n > > - or: add x0, x0, 0 (NOP) */ > > - /* These instructions are part of the prologue, but we don't need > > - to do anything special to handle them. */ > > + /* Handle: add x0, x0, 0 (NOP) */ > > } > > + else if (insn.opcode () == riscv_insn::AUIPC) > > + { > > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS); > > + regs[insn.rd ()] = pv_constant (cur_pc + insn.imm_signed ()); > > + } > > + else if (insn.opcode () == riscv_insn::LUI) > > + { > > + /* Handle: lui REG, n > > + Where REG is not gp register. */ > > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS); > > + regs[insn.rd ()] = pv_constant (insn.imm_signed ()); > > + } > > + else if (insn.opcode () == riscv_insn::ADDI) > > + { > > + /* Handle: addi REG1, REG2, IMM */ > > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS); > > + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS); > > + regs[insn.rd ()] > > + = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ()); > > + } > > + else if (insn.opcode () == riscv_insn::ADD) > > + { > > + /* Handle: addi REG1, REG2, IMM */ > > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS); > > + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS); > > + gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS); > > + regs[insn.rd ()] = pv_add (regs[insn.rs1 ()], regs[insn.rs2 ()]); > > + } > > else > > { > > end_prologue_addr = cur_pc; > > diff --git a/gdb/testsuite/gdb.base/large-frame-1.c b/gdb/testsuite/gdb.base/large-frame-1.c > > new file mode 100644 > > index 00000000000..cd9c5ccdd0a > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/large-frame-1.c > > @@ -0,0 +1,32 @@ > > +/* This file is part of GDB, the GNU debugger. > > + > > + Copyright 2018 Free Software Foundation, Inc. > > + > > + 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 . */ > > + > > +#include "large-frame.h" > > + > > +__attribute__ ((noinline)) void > > +blah (int *a) > > +{ > > + asm ("" :: "m" (a) : "memory"); > > +} > > + > > +int > > +main () > > +{ > > + func (); > > + > > + return 0; > > +} > > diff --git a/gdb/testsuite/gdb.base/large-frame-2.c b/gdb/testsuite/gdb.base/large-frame-2.c > > new file mode 100644 > > index 00000000000..2491c347292 > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/large-frame-2.c > > @@ -0,0 +1,25 @@ > > +/* This file is part of GDB, the GNU debugger. > > + > > + Copyright 2018 Free Software Foundation, Inc. > > + > > + 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 . */ > > + > > +#include "large-frame.h" > > + > > +__attribute__ ((noinline)) int > > +func (void) > > +{ > > + int a[4096]; > > + blah (a); > > +} > > diff --git a/gdb/testsuite/gdb.base/large-frame.exp b/gdb/testsuite/gdb.base/large-frame.exp > > new file mode 100644 > > index 00000000000..00090da795e > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/large-frame.exp > > @@ -0,0 +1,57 @@ > > +# Copyright 2018 Free Software Foundation, Inc. > > +# > > +# 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. > > + > > +# This test was added to test GDB's ability to backtrace over a large > > +# stack frame for which there is no debug information. This should > > +# test the non-DWARF stack unwinder. > > +# > > +# The test was originally added for Risc-V where this case caused a > > +# problem at one point, however, there's nothing Risc-V specific in > > +# the test. > > + > > +proc run_test { opt_level } { > > + global srcfile srcfile2 binfile hex > > + > > + standard_testfile large-frame-1.c large-frame-2.c > > + > > + if {[prepare_for_testing_full "failed to prepare" \ > > + [list ${binfile}-${opt_level} debug \ > > + $srcfile [list debug] \ > > + $srcfile2 [list nodebug optimize=-$opt_level]]]} { > > + return > > + } > > + > > + if { ![runto_main] } then { > > + unsupported "runto main" > > + return > > + } > > + > > + gdb_breakpoint "blah" > > + gdb_continue_to_breakpoint "blah" > > + > > + gdb_test "backtrace" [multi_line \ > > + "#0 blah \[^\n\r\]+" \ > > + "#1 $hex in func \[^\n\r\]+" \ > > + "#2 $hex in main \[^\n\r\]+"] > > +} > > + > > +foreach opt { O0 O1 O2 } { > > + with_test_prefix "optimize=-$opt" { > > + run_test $opt > > + } > > +} > > + > > diff --git a/gdb/testsuite/gdb.base/large-frame.h b/gdb/testsuite/gdb.base/large-frame.h > > new file mode 100644 > > index 00000000000..17058a5efd2 > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/large-frame.h > > @@ -0,0 +1,24 @@ > > +/* This file is part of GDB, the GNU debugger. > > + > > + Copyright 2018 Free Software Foundation, Inc. > > + > > + 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 . */ > > + > > +#ifndef LARGE_FRAME_H > > +#define LARGE_FRAME_H > > + > > +extern void blah (int *); > > +extern int func (void); > > + > > +#endif /* LARGE_FRAME_H */ > > -- > > 2.14.4 > > -- > Joel