From: Andrew Burgess <andrew.burgess@embecosm.com>
To: John Darrington <john@darrington.wattle.id.au>
Cc: gdb-patches@sourceware.org
Subject: Re: [PING] Re: [PATCH] GDB (s12z): Improve reliability of the stack unwinder.
Date: Thu, 09 May 2019 07:52:00 -0000 [thread overview]
Message-ID: <20190509075207.GC2568@embecosm.com> (raw)
In-Reply-To: <20190509045919.7bkcjg2xil7co4gq@jocasta.intra>
* John Darrington <john@darrington.wattle.id.au> [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
next prev parent reply other threads:[~2019-05-09 7:52 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-25 13:45 John Darrington
2019-05-09 4:59 ` [PING] " John Darrington
2019-05-09 7:52 ` Andrew Burgess [this message]
2019-05-09 10:22 ` John Darrington
2019-05-14 17:13 ` Andrew Burgess
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190509075207.GC2568@embecosm.com \
--to=andrew.burgess@embecosm.com \
--cc=gdb-patches@sourceware.org \
--cc=john@darrington.wattle.id.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox