From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83719 invoked by alias); 29 Aug 2018 23:36:31 -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 83694 invoked by uid 89); 29 Aug 2018 23:36:30 -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,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-pf1-f177.google.com Received: from mail-pf1-f177.google.com (HELO mail-pf1-f177.google.com) (209.85.210.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 29 Aug 2018 23:36:27 +0000 Received: by mail-pf1-f177.google.com with SMTP id k21-v6so2967773pff.11 for ; Wed, 29 Aug 2018 16:36:27 -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=RC+uXNh0SFbb3ogY9KHlkAGC+6vd8IV2Z/hg036e6Z4=; b=E/zcSB81KbjOFMZwP9QZBEI/AD9qdp2BvodTz+dCLrakq+lNxBE3mM5MCHI9bW3A+2 VsJiF8vmJ8l1WnLNRcUIGGm6xKAlvDkI2DWdysUG9fuQ4XER4OrtxPENrVZwuzYLI7aU nhysn5nj7mJSyBUB7vuo4wWZOF1Qr2GcdEQ7bIDkJH0BooHinsj+qZC0lzcQB+54jH3t 9s76/oPz1lTAiYXr+iZ/oD6mz8Xx/Bo6ulYqTHoQs3758uS/Imsa0lk4x8/IebqQFMlU suuhD/KRwsDglxNnUC2xpAlyLpT/iEiPyJAH6vBclrDee4c8UwXeDJUVkfcuVWOd/+Q+ G+9Q== Return-Path: Received: from localhost ([12.206.222.5]) by smtp.gmail.com with ESMTPSA id o10-v6sm19089164pfk.76.2018.08.29.16.36.23 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 29 Aug 2018 16:36:24 -0700 (PDT) Date: Wed, 29 Aug 2018 23:36:00 -0000 X-Google-Original-Date: Wed, 29 Aug 2018 16:35:17 PDT (-0700) Subject: Re: [PATCH 4/4] gdb/riscv: Provide non-DWARF stack unwinder In-Reply-To: CC: gdb-patches@sourceware.org, Jim Wilson , andrew.burgess@embecosm.com 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-08/txt/msg00789.txt.bz2 On Wed, 29 Aug 2018 09:40:54 PDT (-0700), andrew.burgess@embecosm.com wrote: > Collects information during the prologue scan and uses this to unwind > registers when no DWARF information is available. > > This patch has been tested by disabling the DWARF stack unwinders, and > running the complete GDB testsuite against a range of RISC-V targets. > The results are comparable to running with the DWARF unwinders in > place. > > gdb/ChangeLog: > > * riscv-tdep.c: Add 'prologue-value.h' include. > (struct riscv_unwind_cache): New struct. > (riscv_debug_unwinder): New global. > (riscv_scan_prologue): Update arguments, capture register details > from prologue scan. > (riscv_skip_prologue): Reformat arguments line, move end of > prologue calculation into riscv_scan_prologue. > (riscv_frame_cache): Update return type, create > riscv_unwind_cache, scan the prologue, and fill in remaining cache > details. > (riscv_frame_this_id): Use frame id computed in riscv_frame_cache. > (riscv_frame_prev_register): Use the trad_frame within the > riscv_unwind_cache. > (_initialize_riscv_tdep): Add 'set/show debug riscv unwinder' > flag. > --- > gdb/ChangeLog | 18 +++++ > gdb/riscv-tdep.c | 227 +++++++++++++++++++++++++++++++++++++++++++------------ > gdb/riscv-tdep.h | 2 + > 3 files changed, 200 insertions(+), 47 deletions(-) > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > index b4ac83a6dd9..35ff27f2bcb 100644 > --- a/gdb/riscv-tdep.c > +++ b/gdb/riscv-tdep.c > @@ -54,6 +54,7 @@ > #include "opcode/riscv-opc.h" > #include "cli/cli-decode.h" > #include "observable.h" > +#include "prologue-value.h" > > /* The stack must be 16-byte aligned. */ > #define SP_ALIGNMENT 16 > @@ -71,6 +72,29 @@ static inline bool is_ ## INSN_NAME ## _insn (long insn) \ > #include "opcode/riscv-opc.h" > #undef DECLARE_INSN > > +/* Cached information about a frame. */ > + > +struct riscv_unwind_cache > +{ > + /* The register from which we can calculate the frame base. This is > + usually $sp or $fp. */ > + int frame_base_reg; > + > + /* The offset from the current value in register FRAME_BASE_REG to the > + actual frame base address. */ > + int frame_base_offset; > + > + /* Information about previous register values. */ > + struct trad_frame_saved_reg *regs; > + > + /* The id for this frame. */ > + struct frame_id this_id; > + > + /* The base (stack) address for this frame. This is the stack pointer > + value on entry to this frame before any adjustments are made. */ > + CORE_ADDR frame_base; > +}; > + > /* Architectural name for core registers. */ > > static const char * const riscv_gdb_reg_names[RISCV_LAST_FP_REGNUM + 1] = > @@ -265,6 +289,11 @@ show_riscv_debug_variable (struct ui_file *file, int from_tty, > > static unsigned int riscv_debug_infcall = 0; > > +/* When this is set to non-zero debugging information about stack unwinding > + will be printed. */ > + > +static unsigned int riscv_debug_unwinder = 0; > + > /* Read the MISA register from the target. The register will only be read > once, and the value read will be cached. If the register can't be read > from the target then a default value (0) will be returned. If the > @@ -1240,16 +1269,34 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc) > > static CORE_ADDR > riscv_scan_prologue (struct gdbarch *gdbarch, > - CORE_ADDR start_pc, CORE_ADDR limit_pc) > + CORE_ADDR start_pc, CORE_ADDR end_pc, > + struct riscv_unwind_cache *cache) > { > - CORE_ADDR cur_pc, next_pc; > - long frame_offset = 0; > + CORE_ADDR cur_pc, next_pc, after_prologue_pc; > CORE_ADDR end_prologue_addr = 0; > > - if (limit_pc > start_pc + 200) > - limit_pc = start_pc + 200; > - > - for (next_pc = cur_pc = start_pc; cur_pc < limit_pc; cur_pc = next_pc) > + /* Find an upper limit on the function prologue using the debug > + information. If the debug information could not be used to provide > + that bound, then use an arbitrary large number as the upper bound. */ > + after_prologue_pc = skip_prologue_using_sal (gdbarch, start_pc); > + if (after_prologue_pc == 0) > + after_prologue_pc = start_pc + 100; /* Arbitrary large number. */ > + if (after_prologue_pc < end_pc) > + end_pc = after_prologue_pc; > + > + pv_t regs[RISCV_NUM_INTEGER_REGS]; /* Number of GPR. */ > + for (int regno = 0; regno < RISCV_NUM_INTEGER_REGS; regno++) > + regs[regno] = pv_register (regno, 0); > + pv_area stack (RISCV_SP_REGNUM, gdbarch_addr_bit (gdbarch)); > + > + if (riscv_debug_unwinder) > + fprintf_unfiltered > + (gdb_stdlog, > + "Prologue scan for function starting at %s (limit %s)\n", > + core_addr_to_string (start_pc), > + core_addr_to_string (end_pc)); > + > + for (next_pc = cur_pc = start_pc; cur_pc < end_pc; cur_pc = next_pc) > { > struct riscv_insn insn; > > @@ -1267,10 +1314,10 @@ riscv_scan_prologue (struct gdbarch *gdbarch, > { > /* Handle: addi sp, sp, -i > or: addiw sp, sp, -i */ > - if (insn.imm_signed () < 0) > - frame_offset += insn.imm_signed (); > - else > - break; > + 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::SW > || insn.opcode () == riscv_insn::SD) > @@ -1282,6 +1329,11 @@ riscv_scan_prologue (struct gdbarch *gdbarch, > or: sw reg, offset(s0) > or: sd reg, offset(s0) */ > /* Instruction storing a register onto the stack. */ > + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS); > + gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS); > + stack.store (pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ()), > + (insn.opcode () == riscv_insn::SW ? 4 : 8), > + regs[insn.rs2 ()]); > } > else if (insn.opcode () == riscv_insn::ADDI > && insn.rd () == RISCV_FP_REGNUM > @@ -1289,6 +1341,10 @@ riscv_scan_prologue (struct gdbarch *gdbarch, > { > /* Handle: addi s0, sp, size */ > /* Instructions setting up the frame pointer. */ > + 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 > || insn.opcode () == riscv_insn::ADDW) > @@ -1299,6 +1355,9 @@ riscv_scan_prologue (struct gdbarch *gdbarch, > /* Handle: add s0, sp, 0 > or: addw s0, sp, 0 */ > /* Instructions setting up the frame pointer. */ > + 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 ()], 0); > } > else if ((insn.rd () == RISCV_GP_REGNUM > && (insn.opcode () == riscv_insn::AUIPC > @@ -1324,24 +1383,63 @@ riscv_scan_prologue (struct gdbarch *gdbarch, > } > else > { > - if (end_prologue_addr == 0) > - end_prologue_addr = cur_pc; > + end_prologue_addr = cur_pc; > + break; > } > } > > if (end_prologue_addr == 0) > end_prologue_addr = cur_pc; > > + if (riscv_debug_unwinder) > + fprintf_unfiltered (gdb_stdlog, "End of prologue at %s\n", > + core_addr_to_string (end_prologue_addr)); > + > + if (cache != NULL) > + { > + /* Figure out if it is a frame pointer or just a stack pointer. Also > + the offset held in the pv_t is from the original register value to > + the current value, which for a grows down stack means a negative > + value. The FRAME_BASE_OFFSET is the negation of this, how to get > + from the current value to the original value. */ > + if (pv_is_register (regs[RISCV_FP_REGNUM], RISCV_SP_REGNUM)) > + { > + cache->frame_base_reg = RISCV_FP_REGNUM; > + cache->frame_base_offset = -regs[RISCV_FP_REGNUM].k; > + } > + else > + { > + cache->frame_base_reg = RISCV_SP_REGNUM; > + cache->frame_base_offset = -regs[RISCV_SP_REGNUM].k; > + } > + > + /* Assign offset from old SP to all saved registers. As we don't > + have the previous value for the frame base register at this > + point, we store the offset as the address in the trad_frame, and > + then convert this to an actual address later. */ > + for (int i = 0; i <= RISCV_NUM_INTEGER_REGS; i++) > + { > + CORE_ADDR offset; > + if (stack.find_reg (gdbarch, i, &offset)) > + { > + if (riscv_debug_unwinder) > + fprintf_unfiltered (gdb_stdlog, > + "Register $%s at stack offset %ld\n", > + gdbarch_register_name (gdbarch, i), > + offset); > + trad_frame_set_addr (cache->regs, i, offset); > + } > + } > + } > + > return end_prologue_addr; > } > > /* Implement the riscv_skip_prologue gdbarch method. */ > > static CORE_ADDR > -riscv_skip_prologue (struct gdbarch *gdbarch, > - CORE_ADDR pc) > +riscv_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc) > { > - CORE_ADDR limit_pc; > CORE_ADDR func_addr; > > /* See if we can determine the end of the prologue via the symbol > @@ -1357,16 +1455,9 @@ riscv_skip_prologue (struct gdbarch *gdbarch, > } > > /* Can't determine prologue from the symbol table, need to examine > - instructions. */ > - > - /* Find an upper limit on the function prologue using the debug > - information. If the debug information could not be used to provide > - that bound, then use an arbitrary large number as the upper bound. */ > - limit_pc = skip_prologue_using_sal (gdbarch, pc); > - if (limit_pc == 0) > - limit_pc = pc + 100; /* MAGIC! */ > - > - return riscv_scan_prologue (gdbarch, pc, limit_pc); > + instructions. Pass -1 for the end address to indicate the prologue > + scanner can scan as far as it needs to find the end of the prologue. */ > + return riscv_scan_prologue (gdbarch, pc, ((CORE_ADDR) -1), NULL); > } > > /* Implement the gdbarch push dummy code callback. */ > @@ -2444,31 +2535,63 @@ riscv_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame) > /* Generate, or return the cached frame cache for the RiscV frame > unwinder. */ > > -static struct trad_frame_cache * > +static struct riscv_unwind_cache * > riscv_frame_cache (struct frame_info *this_frame, void **this_cache) > { > - CORE_ADDR pc; > - CORE_ADDR start_addr; > - CORE_ADDR stack_addr; > - struct trad_frame_cache *this_trad_cache; > + CORE_ADDR pc, start_addr; > + struct riscv_unwind_cache *cache; > struct gdbarch *gdbarch = get_frame_arch (this_frame); > + int numregs, regno; > > if ((*this_cache) != NULL) > - return (struct trad_frame_cache *) *this_cache; > - this_trad_cache = trad_frame_cache_zalloc (this_frame); > - (*this_cache) = this_trad_cache; > + return (struct riscv_unwind_cache *) *this_cache; > > - trad_frame_set_reg_realreg (this_trad_cache, gdbarch_pc_regnum (gdbarch), > - RISCV_RA_REGNUM); > + cache = FRAME_OBSTACK_ZALLOC (struct riscv_unwind_cache); > + cache->regs = trad_frame_alloc_saved_regs (this_frame); > + (*this_cache) = cache; > > + /* Scan the prologue, filling in the cache. */ > + start_addr = get_frame_func (this_frame); > pc = get_frame_pc (this_frame); > - find_pc_partial_function (pc, NULL, &start_addr, NULL); > - stack_addr = get_frame_register_signed (this_frame, RISCV_SP_REGNUM); > - trad_frame_set_id (this_trad_cache, frame_id_build (stack_addr, start_addr)); > + riscv_scan_prologue (gdbarch, start_addr, pc, cache); > + > + /* We can now calculate the frame base address. */ > + cache->frame_base > + = (get_frame_register_signed (this_frame, cache->frame_base_reg) + > + cache->frame_base_offset); > + if (riscv_debug_unwinder) > + fprintf_unfiltered (gdb_stdlog, "Frame base is %s ($%s + 0x%x)\n", > + core_addr_to_string (cache->frame_base), > + gdbarch_register_name (gdbarch, > + cache->frame_base_reg), > + cache->frame_base_offset); > + > + /* The prologue scanner sets the address of registers stored to the stack > + as the offset of that register from the frame base. The prologue > + scanner doesn't know the actual frame base value, and so is unable to > + compute the exact address. We do now know the frame base value, so > + update the address of registers stored to the stack. */ > + numregs = gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch); > + for (regno = 0; regno < numregs; ++regno) > + { > + if (trad_frame_addr_p (cache->regs, regno)) > + cache->regs[regno].addr += cache->frame_base; > + } > + > + /* The previous $pc can be found wherever the $ra value can be found. > + The previous $ra value is gone, this would have been stored be the > + previous frame if required. */ > + cache->regs[gdbarch_pc_regnum (gdbarch)] = cache->regs[RISCV_RA_REGNUM]; > + trad_frame_set_unknown (cache->regs, RISCV_RA_REGNUM); > + > + /* Build the frame id. */ > + cache->this_id = frame_id_build (cache->frame_base, start_addr); > > - trad_frame_set_this_base (this_trad_cache, stack_addr); > + /* The previous $sp value is the frame base value. */ > + trad_frame_set_value (cache->regs, gdbarch_sp_regnum (gdbarch), > + cache->frame_base); > > - return this_trad_cache; > + return cache; > } > > /* Implement the this_id callback for RiscV frame unwinder. */ > @@ -2478,10 +2601,10 @@ riscv_frame_this_id (struct frame_info *this_frame, > void **prologue_cache, > struct frame_id *this_id) > { > - struct trad_frame_cache *info; > + struct riscv_unwind_cache *cache; > > - info = riscv_frame_cache (this_frame, prologue_cache); > - trad_frame_get_id (info, this_id); > + cache = riscv_frame_cache (this_frame, prologue_cache); > + *this_id = cache->this_id; > } > > /* Implement the prev_register callback for RiscV frame unwinder. */ > @@ -2491,10 +2614,10 @@ riscv_frame_prev_register (struct frame_info *this_frame, > void **prologue_cache, > int regnum) > { > - struct trad_frame_cache *info; > + struct riscv_unwind_cache *cache; > > - info = riscv_frame_cache (this_frame, prologue_cache); > - return trad_frame_get_register (info, this_frame, regnum); > + cache = riscv_frame_cache (this_frame, prologue_cache); > + return trad_frame_get_prev_register (this_frame, cache->regs, regnum); > } > > /* Structure defining the RiscV normal frame unwind functions. Since we > @@ -2823,6 +2946,16 @@ of the inferior call mechanism."), > show_riscv_debug_variable, > &setdebugriscvcmdlist, &showdebugriscvcmdlist); > > + add_setshow_zuinteger_cmd ("unwinder", class_maintenance, > + &riscv_debug_unwinder, _("\ > +Set riscv stack unwinding debugging."), _("\ > +Show riscv stack unwinding debugging."), _("\ > +When non-zero, print debugging information for the riscv specific parts\n\ > +of the stack unwinding mechanism."), > + NULL, > + show_riscv_debug_variable, > + &setdebugriscvcmdlist, &showdebugriscvcmdlist); > + > /* Add root prefix command for all "set riscv" and "show riscv" commands. */ > add_prefix_cmd ("riscv", no_class, set_riscv_command, > _("RISC-V specific commands."), > diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h > index 8358d4e00ba..8a2454eb668 100644 > --- a/gdb/riscv-tdep.h > +++ b/gdb/riscv-tdep.h > @@ -34,6 +34,8 @@ enum > RISCV_A1_REGNUM = 11, /* Second argument. */ > RISCV_PC_REGNUM = 32, /* Program Counter. */ > > + RISCV_NUM_INTEGER_REGS = 32, > + > RISCV_FIRST_FP_REGNUM = 33, /* First Floating Point Register */ > RISCV_FA0_REGNUM = 43, > RISCV_FA1_REGNUM = RISCV_FA0_REGNUM + 1, There only thing I can think of here is that this won't handle large stack frames where we have to use some sort of lui-based sequence to increment the stack pointer. For example, if I do something like extern int func(int *a); int _start(void) { int a[4096]; return func(a); } $ riscv64-sifive-elf-gcc test.c -O3 -S -o- .file "test.c" .option nopic .text .align 1 .globl _start .type _start, @function _start: li t1,-16384 addi sp,sp,-32 addi t1,t1,16 sd ra,24(sp) li a5,16384 add sp,sp,t1 add a5,a5,sp li a0,-16384 add a0,a5,a0 call func li t1,16384 addi t1,t1,-16 add sp,sp,t1 ld ra,24(sp) addi sp,sp,32 jr ra .size _start, .-_start .ident "GCC: (GNU) 8.1.0" I don't see why this would block merging the patches, though. I'm also not sure why you'd end up with something like "auipc gp, IMM" or "addi gp, gp, IMM" in a prologue, is that a holdover from MIPS where they have a GP per shared object? If we've actually got any of those it seems suspicious at best.