From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 51562 invoked by alias); 26 Sep 2018 17:13:37 -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 51548 invoked by uid 89); 26 Sep 2018 17:13:36 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.7 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-pf1-f193.google.com Received: from mail-pf1-f193.google.com (HELO mail-pf1-f193.google.com) (209.85.210.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 26 Sep 2018 17:13:34 +0000 Received: by mail-pf1-f193.google.com with SMTP id b7-v6so6331749pfo.3 for ; Wed, 26 Sep 2018 10:13:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=date:subject:in-reply-to:cc:from:to:message-id:mime-version :content-transfer-encoding; bh=mcOK1y8wl7alIuVDwOKAtYUnUMl+hcPoutt0/T8T0c0=; b=EHRlTmbdJqmRMFZ6gBepaW0HRvLeu0CpxqlDGAMnq+OyGJOEox6FXseoJyOV4oKUBj iRpnHCXTl347I8tWdC9oxA22+82AM1xD8EW/KHBYnaRwVUqYmR6Rxnh9O3AdFvnuGBDV E0GxzV3DY6CK/eqlOm400mUW9cWKuYjyXbIh9mAgCdSA7fwkdfPU4ziCXbfwCa2UxeWd V2k7Nl67a6RQG+OSHLPHSqDWFT5FrlT3Zx37EuwURM95gxKVr1zQ2TmPmgzN7EPnlszB LVxkKi0sPY5NPAblvUtDg4vVax03sst7FWKGT/MVLEKjEDLu1AOpOcfIXrAWXsgq/zwf FLjA== Return-Path: Received: from localhost ([12.206.222.5]) by smtp.gmail.com with ESMTPSA id d19-v6sm11496187pfe.42.2018.09.26.10.13.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 26 Sep 2018 10:13:29 -0700 (PDT) Date: Wed, 26 Sep 2018 17:13:00 -0000 X-Google-Original-Date: Wed, 26 Sep 2018 10:13:24 PDT (-0700) Subject: Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding In-Reply-To: <20180926151112.GL5952@embecosm.com> CC: brobecker@adacore.com, gdb-patches@sourceware.org, Jim Wilson From: Palmer Dabbelt To: andrew.burgess@embecosm.com Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2018-09/txt/msg00857.txt.bz2 On Wed, 26 Sep 2018 08:11:12 PDT (-0700), andrew.burgess@embecosm.com wrote: > * 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. While I agree this is true for I-based ISAs, I think this might be able to fire for E-based ISAs because those can actually encode invalid register indices. That said, these should be decoded as invalid instructions so I think we're safe here. I'm OK either way (ie, abort or warn). > > 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