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 C35D638518AB for ; Wed, 14 Dec 2022 00:46:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C35D638518AB 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::146]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 93A9D80092; Wed, 14 Dec 2022 00:46:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=lancelotsix.com; s=2021; t=1670978787; bh=y6tupLK4DsASguPYfhYx9eNLKO+43d4nG42upUpc8+s=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=NhHKLxFvoYdCPbbV9Z8sZuBkvp5x6x8CeEPVG7qgOym7fp+FbPCYzSEs75GcuzNqe r/1esQUc09ddVzP74lBCD+O8Hd43BPghAfGPJ56dNT+eh6z5NcFTU4DZ1iDYkexWVw Ltp0oXY4vmR6oOgwe3ZMprkNxRfHSxWeLWmsgZqupXMQsrHs5Adgd8ycpygwCxR3fD +sJ0ZWSO3quA5OYkajIcEVKvMGBtqC7QyYJcPre0S/0jljJQW3UyfEY38JrVoVk0d6 c8STQrcUKKUcyKHE25M7LUCTgmLa2Tr7Hf6dqmvhtXP1lTnHNPjXR6xm5KluSv9lhw wYy4FcZIHcmSQ== Date: Wed, 14 Dec 2022 00:46:22 +0000 From: Lancelot SIX To: Bruno Larsen Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2] gdb: add 'maintenance print record-instruction' command Message-ID: <20221214004529.3hgz2yuhv6rx2r4c@ubuntu.lan> References: <20221212104417.136536-1-blarsen@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221212104417.136536-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, 14 Dec 2022 00:46:27 +0000 (UTC) X-Spam-Status: No, score=-10.3 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 Bruno, I have a couple more comments and suggestions below. On Mon, Dec 12, 2022 at 11:44:17AM +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..25e1fd22c6f 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) > +{ > + 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. */ I think the literal 0 in the comment is a left-over from V1, right? > + while (to_print->next != nullptr && offset > 0) > + { > + to_print = to_print->next; > + if (to_print->type == record_full_end) > + offset--; > + } > + if (offset != 0) > + error (_("Not enough recorded history")); > + } > + else > + { > + while (to_print->prev != nullptr && offset < 0) > + { > + to_print = to_print->prev; > + if (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 != nullptr && to_print->prev->type != record_full_end) > + to_print = to_print->prev; > + > + while (to_print->type != record_full_end) > + { > + switch (to_print->type) > + { > + case record_full_reg: > + { > + gdb_byte* b = record_full_get_loc (to_print); ^^ The space comes before the * here. > + gdb_printf ("Register %%%s changed:", The '%' prefix for register names is specific to the att assembler syntax. Do we want this here? I think in most places GDB uses the plain register name without such prefix. > + 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]); ^ Space after the ",". > + gdb_printf ("\n"); Did you consider printing the register value instead of the bytes composing the value? I think something like this could do: auto mark = value_mark (); type *regtype = gdbarch_register_type (target_gdbarch (), to_print->u.reg.num); value *val = value_from_contents (regtype, record_full_get_loc (to_print)); gdb_printf ("Register %s changed: %s\n", gdbarch_register_name (target_gdbarch (), to_print->u.reg.num), hex_string (value_as_long (val))); value_free_to_mark (mark); This would however not work for vector registers (anything which does not fit in a long long). You might want to use something like `value_print` to handle all cases, unless its output is too fancy: struct value_print_options opts; get_user_print_options (&opts); auto mark = value_mark (); type *regtype = gdbarch_register_type (target_gdbarch (), to_print->u.reg.num); value *val = value_from_contents (regtype, record_full_get_loc (to_print)); gdb_printf ("Register %s changed: %s\n", gdbarch_register_name (target_gdbarch (), to_print->u.reg.num)); value_print (val, gdb_stdout, &opts); gdb_printf ("\n"); value_free_to_mark (mark); Not sure if there is a cleaner way to achieve this, but it should do the trick. > + break; > + } > + case record_full_mem: > + { > + gdb_byte* b = record_full_get_loc (to_print); Same here, the space shoud be before the "*". > + 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]); Space after the ",". Best, Lancelot. > + 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 >