From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 20EB7388FB64 for ; Wed, 7 Dec 2022 14:37:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 20EB7388FB64 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 2BE6E80A15; Wed, 7 Dec 2022 14:37:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=lancelotsix.com; s=2021; t=1670423858; bh=UwWBdq+bSGpQTd5P9DcVrqQ3umXgaoEPk+xltXQKjFM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=uFaoXg5oyzuN5unD3tg6rh1M3EGDpNWvqqujBxCI4VK1rYgJzDp22mfKrvR2tzpU8 f66shDfgFSKY8BGE1frx+K2ZgNh7vJ3OkHa+P/EygK2yy42wbpTo8XO6OOAuJVlnYu ReV4RAun78rhhSdfWcwxQuZE7bIrVWlR6uf9jdy5bHxhJxxmtyHqsT791wqP6o6+F2 7MqDU5UBD3C8zRgYXHnpGqnF4zT5+vJ+zllo1VitLYPLsdb0k8O7//QQfxYpFTD/qJ 7640RZjZs9MivxmHD/65+Wx2NtpLRGvMICBDSf2eHL8BkaTfglwOffq9ZoFe0vTdw4 3wBm5EJ5vTDEA== Date: Wed, 7 Dec 2022 14:37:31 +0000 From: Lancelot SIX To: Bruno Larsen Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: add 'maintenance print record-instruction' command Message-ID: <20221207143731.uopir7l4ilssjkqv@ubuntu.lan> References: <20221207135000.1344331-1-blarsen@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221207135000.1344331-1-blarsen@redhat.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Wed, 07 Dec 2022 14:37:38 +0000 (UTC) X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi, I do not not know this part well (to say the least), so my comments will only focus on style On Wed, Dec 07, 2022 at 02:50:00PM +0100, Bruno Larsen via Gdb-patches wrote: > While chasing some reverse debugging bugs, I found myself wondering what > was recorded by GDB to undo and redo a certain instruction. This commit > implements a simple way of printing that information. > --- > gdb/NEWS | 6 ++++ > gdb/doc/gdb.texinfo | 8 +++++ > gdb/record-full.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 95 insertions(+) > > diff --git a/gdb/NEWS b/gdb/NEWS > index c4ccfcc9e32..d6ce6bf86a0 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -103,6 +103,12 @@ > > * New commands > > +maintenance print record-instruction [ N ] > + Print the recorded information for a given instruction. If N is not given > + prints how GDB would undo the last instruction executed. If N is negative, > + prints how GDB would undo the N-th previous instruction, and if N is > + positive, it prints how GDB will redo the N-th following instruction. > + > maintenance set ignore-prologue-end-flag on|off > maintenance show ignore-prologue-end-flag > This setting, which is off by default, controls whether GDB ignores the > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 5b566669975..807af351e79 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -40531,6 +40531,14 @@ that symbol is described. The type chain produced by this command is > a recursive definition of the data type as stored in @value{GDBN}'s > data structures, including its flags and contained types. > > +@kindex maint print record-instruction > +@item maint print record-instruction > +@itemx maint print record-instruction @var{N} > +@cindex print how GDB recorded a given instruction. If N is not positive > +number, it prints the values stored by the inferior before the N-th previous > +instruction was exectued. If N is positive, print the values after the N-th > +following instruction is executed. If N is not given, 0 is assumed. > + > @kindex maint selftest > @cindex self tests > @item maint selftest @r{[}-verbose@r{]} @r{[}@var{filter}@r{]} > diff --git a/gdb/record-full.c b/gdb/record-full.c > index 48b92281fe6..47cdf75eea4 100644 > --- a/gdb/record-full.c > +++ b/gdb/record-full.c > @@ -2764,6 +2764,79 @@ set_record_full_insn_max_num (const char *args, int from_tty, > } > } > > +/* Implement the 'maintenance print record-instruction' command. */ > + > +static void > +maintenance_print_record_instruction (const char *args, int from_tty) I think, usually the command implementation have a _command suffix in the name. I do not know if this is a hard rule. > +{ > + struct record_full_entry* to_print = record_full_list; > + > + if (args != nullptr) > + { > + int offset = value_as_long (parse_and_eval (args)); > + if (offset > 0) > + { > + /* Move forward OFFSET instructions. We know we found the > + end of an instruction when to_print->type is 0. */ > + while (to_print->next && offset > 0) to_print->next != nullptr > + { > + to_print = to_print->next; > + if (!to_print->type) I would find it easier to read if you spelled it as: if (to_print->type != record_full_end) rather than relying on record_full_end's value being 0. When dealing with enum I find it clearer to use the symbolic name rather than the underlying value. > + offset --; Is the space before "--" intentional? > + } > + if (offset != 0) > + error (_("Not enough recorded history")); > + } > + else > + { > + while (to_print->prev && offset < 0) to_print->prev != nullptr > + { > + to_print = to_print->prev; > + if (!to_print->type) to_print->type == record_full_end ? > + offset++; > + } > + if (offset != 0) > + error (_("Not enough recorded history")); > + } > + } > + gdb_assert (to_print != nullptr); > + > + /* Go back to the start of the instruction. */ > + while (to_print->prev && to_print->prev->type) Similarly: to_print->prev != nullptr && to_print->prev->type != record_full_end? Best, Lancelot. > + to_print = to_print->prev; > + > + while (to_print->type) > + { > + switch (to_print->type) > + { > + case record_full_reg: > + { > + gdb_byte* b = record_full_get_loc (to_print); > + gdb_printf ("Register %%%s changed:", > + gdbarch_register_name (target_gdbarch (), > + to_print->u.reg.num)); > + for (int i = 0; i < to_print->u.reg.len; i++) > + gdb_printf (" %02x",b[i]); > + gdb_printf ("\n"); > + break; > + } > + case record_full_mem: > + { > + gdb_byte* b = record_full_get_loc (to_print); > + gdb_printf ("%d bytes of memory at address %s changed from:", > + to_print->u.mem.len, > + print_core_address (target_gdbarch (), > + to_print->u.mem.addr)); > + for (int i = 0; i < to_print->u.mem.len; i++) > + gdb_printf (" %02x",b[i]); > + gdb_printf ("\n"); > + break; > + } > + } > + to_print = to_print->next; > + } > +} > + > void _initialize_record_full (); > void > _initialize_record_full () > @@ -2868,4 +2941,12 @@ When ON, query if PREC cannot record memory change of next instruction."), > c = add_alias_cmd ("memory-query", record_full_memory_query_cmds.show, > no_class, 1,&show_record_cmdlist); > deprecate_cmd (c, "show record full memory-query"); > + > + add_cmd ("record-instruction", class_maintenance, > + maintenance_print_record_instruction, > + _("\ > +Print a recorded instruction.\nIf no argument is provided, print the last \ > +instruction recorded.\nIf a negative argument is given, prints how the nth \ > +previous instruction will be undone.\nIf a positive argument is given, prints \ > +how the nth following instruction will be redone."), &maintenanceprintlist); > } > -- > 2.38.1 >