From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 62022 invoked by alias); 9 May 2019 07:52:15 -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 62013 invoked by uid 89); 9 May 2019 07:52:15 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.3 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.1 spammy=sk:gdb_ass X-HELO: mail-wm1-f68.google.com Received: from mail-wm1-f68.google.com (HELO mail-wm1-f68.google.com) (209.85.128.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 09 May 2019 07:52:12 +0000 Received: by mail-wm1-f68.google.com with SMTP id h11so1814615wmb.5 for ; Thu, 09 May 2019 00:52:11 -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=Ae8zC37Sv+XgbmhPl1E1JNExASXon/6UmqDo8nKTcTc=; b=aSLD5ovJRVpPuZ5T0RSuxsISyGriBtZfFLKFWj/Q/UHR3zbZfLy7rALbQg6Qj+y+Rf ywuQB6KWsuiPQY4AtyxmbrUxxvT1qa1eCLq3+PVPF9NQT9Cg9ZV+uFFfsgfP5xkdfCnR hYoCpx1W1whcM1+YfsocRr6uelaOvzLZ77BAZ8Y+Bi4GZjZiawT+zFfqXayPmKJuEOa/ YBCfX1LNqCAjoJ3jfoMZwKU5bTGmWamKzbIqiEAiFQiNw7vaM4mNAhZ/9fpklz2uclSx g6ByquXdIi5P3DQdYs0EDEesfQNArbt4e0WGR0iJDspe39xABtgt2Ld1BArwx5O/mYDJ iuoQ== Return-Path: Received: from localhost (cust64-dsl91-135-5.idnet.net. [91.135.5.64]) by smtp.gmail.com with ESMTPSA id o3sm1163151wrw.76.2019.05.09.00.52.08 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 09 May 2019 00:52:08 -0700 (PDT) Date: Thu, 09 May 2019 07:52:00 -0000 From: Andrew Burgess To: John Darrington Cc: gdb-patches@sourceware.org Subject: Re: [PING] Re: [PATCH] GDB (s12z): Improve reliability of the stack unwinder. Message-ID: <20190509075207.GC2568@embecosm.com> References: <20190425134515.8202-1-john@darrington.wattle.id.au> <20190509045919.7bkcjg2xil7co4gq@jocasta.intra> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190509045919.7bkcjg2xil7co4gq@jocasta.intra> X-Fortune: Great minds run in great circles. 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: 2019-05/txt/msg00229.txt.bz2 * John Darrington [2019-05-09 06:59:19 +0200]: > On Thu, Apr 25, 2019 at 03:45:15PM +0200, John Darrington suggested: > Previously, the stack unwinder searched through consecutive bytes for values > which it thought might be the start of a stack mutating operation. > This was error prone, because such bytes could also be the operands of other > instructions. This change uses the opcodes api to interpret the code in each > frame. This sounds like a good change, and the core of the patch looks reasonable, there's a few style issues that need to be addressed though. > > gdb/ChangeLog: > * s12z-tdep.c (push_pull_get_stack_adjustment): New function. > (s12z_frame_cache): Use opcodes API to interpret stack frame > code. It looks like most of the changes in this patch are missing from the ChangeLog. You need to document each new struct and function. > --- > gdb/s12z-tdep.c | 227 ++++++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 164 insertions(+), 63 deletions(-) > > diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c > index cef92d8774..b80509f33e 100644 > --- a/gdb/s12z-tdep.c > +++ b/gdb/s12z-tdep.c > @@ -30,6 +30,7 @@ > #include "opcode/s12z.h" > #include "trad-frame.h" > #include "remote.h" > +#include "opcodes/s12z-opc.h" > > /* Two of the registers included in S12Z_N_REGISTERS are > the CCH and CCL "registers" which are just views into > @@ -162,6 +163,97 @@ s12z_disassemble_info (struct gdbarch *gdbarch) > return di; > } > > + > +struct mem_read_abstraction > +{ > + struct mem_read_abstraction_base base; > + bfd_vma memaddr; > + struct disassemble_info* info; > +}; The structure needs a descriptive comment at the top, and ideally each field documented with a comment. > + > +static void > +advance (struct mem_read_abstraction_base *b) > +{ > + struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b; > + mra->memaddr++; > +} All functions should have a header comment. > + > +static bfd_vma > +posn (struct mem_read_abstraction_base *b) > +{ > + struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b; > + return mra->memaddr; > +} > + > +static int > +abstract_read_memory (struct mem_read_abstraction_base *b, > + int offset, > + size_t n, bfd_byte *bytes) > +{ > + struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b; > + > + int status = > + (*mra->info->read_memory_func) (mra->memaddr + offset, > + bytes, n, mra->info); > + > + if (status != 0) > + { > + (*mra->info->memory_error_func) (status, mra->memaddr, mra->info); > + return -1; > + } > + > + return 0; > +} > + > + > +/* Return the stack adjustment caused by > + a push or pull instruction. */ > +static int > +push_pull_get_stack_adjustment (int n_operands, > + struct operand *const *operands) > +{ > + int stack_adjustment = 0; > + gdb_assert (n_operands > 0); > + if (operands[0]->cl == OPND_CL_REGISTER_ALL) > + stack_adjustment = 26; /* All the regs */ Missing a '.' at the end of this comment, and in a few other comments too. > + else if (operands[0]->cl == OPND_CL_REGISTER_ALL16) > + stack_adjustment = 4 * 2; /* All 4 16 bit regs */ > + else > + for (int i = 0; i < n_operands; ++i) > + { > + if (operands[i]->cl != OPND_CL_REGISTER) > + continue; /* I don't think this can ever happen. */ > + const struct register_operand *op > + = (const struct register_operand *) operands[i]; > + switch (op->reg) > + { > + case REG_X: > + case REG_Y: > + stack_adjustment += 3; > + break; > + case REG_D7: > + case REG_D6: > + stack_adjustment += 4; > + break; > + case REG_D2: > + case REG_D3: > + case REG_D4: > + case REG_D5: > + stack_adjustment += 2; > + break; > + case REG_D0: > + case REG_D1: > + case REG_CCL: > + case REG_CCH: > + stack_adjustment += 1; > + break; > + default: > + gdb_assert (0); /* This cannot happen. */ You can use gdb_assert_not_reached here. > + } > + } > + return stack_adjustment; > +} > + > /* Initialize a prologue cache. */ > > static struct trad_frame_cache * > @@ -234,73 +326,82 @@ s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache) > CORE_ADDR addr = start_addr; /* Where we have got to? */ > int frame_size = 0; > int saved_frame_size = 0; > - while (this_pc > addr) > - { > - struct disassemble_info di = s12z_disassemble_info (gdbarch); > - > - /* No instruction can be more than 11 bytes long, I think. */ > - gdb_byte buf[11]; > > - int nb = print_insn_s12z (addr, &di); > - gdb_assert (nb <= 11); > + struct disassemble_info di = s12z_disassemble_info (gdbarch); > > - if (0 != target_read_code (addr, buf, nb)) > - memory_error (TARGET_XFER_E_IO, addr); > > - if (buf[0] == 0x05) /* RTS */ > - { > - frame_size = saved_frame_size; > - } > - /* Conditional Branches. If any of these are encountered, then > - it is likely that a RTS will terminate it. So we need to save > - the frame size so it can be restored. */ > - else if ( (buf[0] == 0x02) /* BRSET */ > - || (buf[0] == 0x0B) /* DBcc / TBcc */ > - || (buf[0] == 0x03)) /* BRCLR */ > - { > - saved_frame_size = frame_size; > - } > - else if (buf[0] == 0x04) /* PUL/ PSH .. */ > - { > - bool pull = buf[1] & 0x80; > - int stack_adjustment = 0; > - if (buf[1] & 0x40) > - { > - if (buf[1] & 0x01) stack_adjustment += 3; /* Y */ > - if (buf[1] & 0x02) stack_adjustment += 3; /* X */ > - if (buf[1] & 0x04) stack_adjustment += 4; /* D7 */ > - if (buf[1] & 0x08) stack_adjustment += 4; /* D6 */ > - if (buf[1] & 0x10) stack_adjustment += 2; /* D5 */ > - if (buf[1] & 0x20) stack_adjustment += 2; /* D4 */ > - } > - else > - { > - if (buf[1] & 0x01) stack_adjustment += 2; /* D3 */ > - if (buf[1] & 0x02) stack_adjustment += 2; /* D2 */ > - if (buf[1] & 0x04) stack_adjustment += 1; /* D1 */ > - if (buf[1] & 0x08) stack_adjustment += 1; /* D0 */ > - if (buf[1] & 0x10) stack_adjustment += 1; /* CCL */ > - if (buf[1] & 0x20) stack_adjustment += 1; /* CCH */ > - } > + struct mem_read_abstraction mra; > + mra.base.read = (int (*)(mem_read_abstraction_base*, > + int, size_t, bfd_byte*)) abstract_read_memory; > + mra.base.advance = advance ; > + mra.base.posn = posn; > + mra.info = &di; > > - if (!pull) > - stack_adjustment = -stack_adjustment; > - frame_size -= stack_adjustment; > - } > - else if (buf[0] == 0x0a) /* LEA S, (xxx, S) */ > - { > - if (0x06 == (buf[1] >> 4)) > - { > - int simm = (signed char) (buf[1] & 0x0F); > - frame_size -= simm; > - } > - } > - else if (buf[0] == 0x1a) /* LEA S, (S, xxxx) */ > - { > - int simm = (signed char) buf[1]; > - frame_size -= simm; > - } > - addr += nb; > + while (this_pc > addr) > + { > + enum optr optr = OP_INVALID; > + short osize; > + int n_operands = 0; > + struct operand *operands[6]; > + mra.memaddr = addr; > + int n_bytes = > + decode_s12z (&optr, &osize, &n_operands, operands, > + (mem_read_abstraction_base *) &mra); > + > + switch (optr) > + { > + case OP_tbNE: > + case OP_tbPL: > + case OP_tbMI: > + case OP_tbGT: > + case OP_tbLE: > + case OP_dbNE: > + case OP_dbEQ: > + case OP_dbPL: > + case OP_dbMI: > + case OP_dbGT: > + case OP_dbLE: > + /* Conditional Branches. If any of these are encountered, then > + it is likely that a RTS will terminate it. So we need to save > + the frame size so it can be restored. */ > + saved_frame_size = frame_size; > + break; > + case OP_rts: > + /* Restore the frame size from a previously saved value. */ > + frame_size = saved_frame_size; > + break; > + case OP_push: > + frame_size += push_pull_get_stack_adjustment (n_operands, operands); > + break; > + case OP_pull: > + frame_size -= push_pull_get_stack_adjustment (n_operands, operands); > + break; > + case OP_lea: > + if (operands[0]->cl == OPND_CL_REGISTER) > + { > + int reg = ((struct register_operand *) (operands[0]))->reg; > + if ((reg == REG_S) && (operands[1]->cl == OPND_CL_MEMORY)) > + { > + const struct memory_operand *mo > + = (const struct memory_operand * ) operands[1]; > + if (mo->n_regs == 1 && !mo->indirect > + && mo->regs[0] == REG_S > + && mo->mutation == OPND_RM_NONE) > + { > + /* LEA S, (xxx, S) -- Decrement the stack. This is > + almost certainly the start of a frame. */ > + int simm = (signed char) mo->base_offset; > + frame_size -= simm; > + } > + } > + } > + break; > + default: > + break; > + } > + addr += n_bytes; > + for (int o = 0; o < n_operands; ++o) > + free (operands[o]); > } > > /* If the PC has not actually got to this point, then the frame > -- > 2.11.0 Thanks, Andrew