public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v3] gdb: add 'maintenance print record-instruction' command
@ 2022-12-22 15:43 Bruno Larsen
  2023-01-02 15:37 ` Alexandra Petlanova Hajkova
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Bruno Larsen @ 2022-12-22 15:43 UTC (permalink / raw)
  To: gdb-patches; +Cc: Bruno Larsen, Eli Zaretskii

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.

If there isn't enough history to print the desired instruction (such as
when the user hasn't started recording yet or when they request 2
instructions back but only 1 was recorded), GDB warns the user like so:

(gdb) maint print record-instruction
Not enough recorded history

If there is enough, GDB prints the instruction like so:

(gdb) maint print record-instruction
4 bytes of memory at address 0x00007fffffffd5dc changed from: 01 00 00 00
Register eflags changed: [ IF ]
Register rip changed: (void (*)()) 0x401115 <main+15>

Approved-by: Eli Zaretskii <eliz@gnu.org>
---
Changes for v3:
* Added minimal test for the new command
* Changed register printing to use value_print

Changes for v2:
* Style changes
---
 gdb/NEWS                                      |  6 ++
 gdb/doc/gdb.texinfo                           |  8 ++
 gdb/record-full.c                             | 95 +++++++++++++++++++
 .../gdb.reverse/maint-print-instruction.c     | 25 +++++
 .../gdb.reverse/maint-print-instruction.exp   | 66 +++++++++++++
 5 files changed, 200 insertions(+)
 create mode 100644 gdb/testsuite/gdb.reverse/maint-print-instruction.c
 create mode 100644 gdb/testsuite/gdb.reverse/maint-print-instruction.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index e61f06081de..2578412d017 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -107,6 +107,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 bef63eb4e8e..eea3037ea5b 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}
+print how GDB recorded a given instruction.  If @var{n} is not positive
+number, it prints the values stored by the inferior before the @var{n}-th previous
+instruction was exectued.  If @var{n} is positive, print the values after the @var{n}-th
+following instruction is executed.  If @var{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..6c47813706b 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -39,6 +39,7 @@
 #include "gdbsupport/gdb_unlinker.h"
 #include "gdbsupport/byte-vector.h"
 #include "async-event.h"
+#include "valprint.h"
 
 #include <signal.h>
 
@@ -2764,6 +2765,90 @@ 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 record_full_end.  */
+	  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;
+
+  /* if we're in the first record, there are no actual instructions
+     recorded.  Warn the user and leave.  */
+  if (to_print == &record_full_first)
+    error (_("Not enough recorded history"));
+
+  while (to_print->type != record_full_end)
+    {
+      switch (to_print->type)
+	{
+	  case record_full_reg:
+	    {
+	      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: ",
+			  gdbarch_register_name (target_gdbarch (),
+						 to_print->u.reg.num));
+	      struct value_print_options opts;
+	      get_user_print_options (&opts);
+	      opts.raw = true;
+	      value_print (val, gdb_stdout, &opts);
+	      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 +2953,14 @@ 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.\n\
+If no argument is provided, print the last instruction recorded.\n\
+If a negative argument is given, prints how the nth previous \
+instruction will be undone.\n\
+If a positive argument is given, prints \
+how the nth following instruction will be redone."), &maintenanceprintlist);
 }
diff --git a/gdb/testsuite/gdb.reverse/maint-print-instruction.c b/gdb/testsuite/gdb.reverse/maint-print-instruction.c
new file mode 100644
index 00000000000..f06ed530620
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/maint-print-instruction.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main ()
+{
+  int x = 0;
+  x ++;
+  x --;
+  return x;
+}
diff --git a/gdb/testsuite/gdb.reverse/maint-print-instruction.exp b/gdb/testsuite/gdb.reverse/maint-print-instruction.exp
new file mode 100644
index 00000000000..b78b696add6
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/maint-print-instruction.exp
@@ -0,0 +1,66 @@
+#   Copyright 2008-2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite.  it tests the functionality of
+# the maintenance print record-instruction command, but does not check the
+# syntax, only if the command finds or fails to find recorded history.
+
+if ![supports_reverse] {
+    return
+}
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+proc test_print { has_history level test_name } {
+    gdb_test_multiple "maint print record-instruction $level" $test_name {
+	-re -wrap ".*Not enough recorded history.*" {
+	    gdb_assert !$has_history $test_name
+	}
+
+    -re -wrap ".*changed.*" {
+	    gdb_assert $has_history $test_name
+	}
+    }
+}
+
+if { ![runto_main] } {
+    return 0
+}
+
+test_print false "" "print before starting to record"
+
+if [supports_process_record] {
+    # Activate process record/replay
+    gdb_test_no_output "record" "turn on process record"
+}
+
+test_print false "" "print before any instruction"
+
+gdb_test "stepi 3" ".*" "collecting history"
+test_print true "" "print current after executing a bit"
+test_print true "-1" "print previous after executing a bit"
+test_print false "1" "print following after executing a bit"
+
+gdb_test "reverse-stepi" ".*" "moving back"
+test_print true "" "print current after reversing"
+test_print true "-1" "print previous after reversing"
+test_print true "1" "print following after reversing"
+
+test_print false "-10" "trying to print too far back"
+test_print false "10" "trying to print too far forward"
-- 
2.38.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] gdb: add 'maintenance print record-instruction' command
  2022-12-22 15:43 [PATCH v3] gdb: add 'maintenance print record-instruction' command Bruno Larsen
@ 2023-01-02 15:37 ` Alexandra Petlanova Hajkova
  2023-01-03  9:00   ` Bruno Larsen
  2023-01-02 16:26 ` Lancelot SIX
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Alexandra Petlanova Hajkova @ 2023-01-02 15:37 UTC (permalink / raw)
  To: Bruno Larsen; +Cc: gdb-patches, Eli Zaretskii

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

> +# This file is part of the GDB testsuite.  it tests the functionality of
> +# the maintenance print record-instruction command, but does not check the
> +# syntax, only if the command finds or fails to find recorded history.
> +
> +if ![supports_reverse] {
> +    return
> +}
> +
>

I think this patch is good to go in general.
 But it might be nice to add a small summary about how the test works as a
comment at the beginning of the test?

Regards,
Alexandra

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] gdb: add 'maintenance print record-instruction' command
  2022-12-22 15:43 [PATCH v3] gdb: add 'maintenance print record-instruction' command Bruno Larsen
  2023-01-02 15:37 ` Alexandra Petlanova Hajkova
@ 2023-01-02 16:26 ` Lancelot SIX
  2023-01-02 16:56   ` Bruno Larsen
  2023-01-03 17:04 ` Tom Tromey
  2023-01-04 10:30 ` Bruno Larsen
  3 siblings, 1 reply; 8+ messages in thread
From: Lancelot SIX @ 2023-01-02 16:26 UTC (permalink / raw)
  To: Bruno Larsen; +Cc: gdb-patches, Eli Zaretskii

Hi Bruno

> diff --git a/gdb/testsuite/gdb.reverse/maint-print-instruction.c b/gdb/testsuite/gdb.reverse/maint-print-instruction.c
> new file mode 100644
> index 00000000000..f06ed530620
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/maint-print-instruction.c
> @@ -0,0 +1,25 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 Free Software Foundation, Inc.

Just mentioning so you do not forget, this will need to change to
2022-2023 before this gets pushed.

> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int
> +main ()
> +{
> +  int x = 0;
> +  x ++;
> +  x --;
> +  return x;
> +}
> diff --git a/gdb/testsuite/gdb.reverse/maint-print-instruction.exp b/gdb/testsuite/gdb.reverse/maint-print-instruction.exp
> new file mode 100644
> index 00000000000..b78b696add6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/maint-print-instruction.exp
> @@ -0,0 +1,66 @@
> +#   Copyright 2008-2022 Free Software Foundation, Inc.

Should be 2022-2023.

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This file is part of the GDB testsuite.  it tests the functionality of
                                              ^
Missing an uppercase here.

> +# the maintenance print record-instruction command, but does not check the
> +# syntax, only if the command finds or fails to find recorded history.
> +
> +if ![supports_reverse] {
> +    return
> +}
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +    return -1
> +}
> +
> +proc test_print { has_history level test_name } {
> +    gdb_test_multiple "maint print record-instruction $level" $test_name {
> +	-re -wrap ".*Not enough recorded history.*" {
> +	    gdb_assert !$has_history $test_name
> +	}
> +
> +    -re -wrap ".*changed.*" {
> +	    gdb_assert $has_history $test_name
> +	}
> +    }
> +}
> +
> +if { ![runto_main] } {
> +    return 0
> +}
> +
> +test_print false "" "print before starting to record"
> +
> +if [supports_process_record] {
> +    # Activate process record/replay
> +    gdb_test_no_output "record" "turn on process record"
> +}
> +

I do not think the rest of the test makes much sense if
[supports_process_record] returns false, does it?  I guess that
everything below this point should be be in the if block.

Another approach might to have the initial test (at the top of the file)
check for both supports_reverse and supports_process_record and ignore
the test if either feature is not supported.  WDYT?

For what it is worth, the rest of the patch looks OK to me.

Best,
Lancelot.

> +test_print false "" "print before any instruction"
> +
> +gdb_test "stepi 3" ".*" "collecting history"
> +test_print true "" "print current after executing a bit"
> +test_print true "-1" "print previous after executing a bit"
> +test_print false "1" "print following after executing a bit"
> +
> +gdb_test "reverse-stepi" ".*" "moving back"
> +test_print true "" "print current after reversing"
> +test_print true "-1" "print previous after reversing"
> +test_print true "1" "print following after reversing"
> +
> +test_print false "-10" "trying to print too far back"
> +test_print false "10" "trying to print too far forward"
> -- 
> 2.38.1
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] gdb: add 'maintenance print record-instruction' command
  2023-01-02 16:26 ` Lancelot SIX
@ 2023-01-02 16:56   ` Bruno Larsen
  2023-01-03 16:10     ` Lancelot SIX
  0 siblings, 1 reply; 8+ messages in thread
From: Bruno Larsen @ 2023-01-02 16:56 UTC (permalink / raw)
  To: Lancelot SIX; +Cc: gdb-patches, Eli Zaretskii

On 02/01/2023 17:26, Lancelot SIX wrote:
> Hi Bruno
>
>> diff --git a/gdb/testsuite/gdb.reverse/maint-print-instruction.c b/gdb/testsuite/gdb.reverse/maint-print-instruction.c
>> new file mode 100644
>> index 00000000000..f06ed530620
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.reverse/maint-print-instruction.c
>> @@ -0,0 +1,25 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2022 Free Software Foundation, Inc.
> Just mentioning so you do not forget, this will need to change to
> 2022-2023 before this gets pushed.
nice catch, thanks!
>
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +int
>> +main ()
>> +{
>> +  int x = 0;
>> +  x ++;
>> +  x --;
>> +  return x;
>> +}
>> diff --git a/gdb/testsuite/gdb.reverse/maint-print-instruction.exp b/gdb/testsuite/gdb.reverse/maint-print-instruction.exp
>> new file mode 100644
>> index 00000000000..b78b696add6
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.reverse/maint-print-instruction.exp
>> @@ -0,0 +1,66 @@
>> +#   Copyright 2008-2022 Free Software Foundation, Inc.
> Should be 2022-2023.
>
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# This file is part of the GDB testsuite.  it tests the functionality of
>                                                ^
> Missing an uppercase here.
>
>> +# the maintenance print record-instruction command, but does not check the
>> +# syntax, only if the command finds or fails to find recorded history.
>> +
>> +if ![supports_reverse] {
>> +    return
>> +}
>> +
>> +standard_testfile
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>> +    return -1
>> +}
>> +
>> +proc test_print { has_history level test_name } {
>> +    gdb_test_multiple "maint print record-instruction $level" $test_name {
>> +	-re -wrap ".*Not enough recorded history.*" {
>> +	    gdb_assert !$has_history $test_name
>> +	}
>> +
>> +    -re -wrap ".*changed.*" {
>> +	    gdb_assert $has_history $test_name
>> +	}
>> +    }
>> +}
>> +
>> +if { ![runto_main] } {
>> +    return 0
>> +}
>> +
>> +test_print false "" "print before starting to record"
>> +
>> +if [supports_process_record] {
>> +    # Activate process record/replay
>> +    gdb_test_no_output "record" "turn on process record"
>> +}
>> +
> I do not think the rest of the test makes much sense if
> [supports_process_record] returns false, does it?  I guess that
> everything below this point should be be in the if block.
>
> Another approach might to have the initial test (at the top of the file)
> check for both supports_reverse and supports_process_record and ignore
> the test if either feature is not supported.  WDYT?
Good call, but since I do think it makes sense to test that GDB doesn't 
go wild even if recording is not supported, I think I'll make the test 
exit early here instead.
>
> For what it is worth, the rest of the patch looks OK to me.
Awesome, may I add your R-b tag, or do you want to see my change to the 
test first?
>
> Best,
> Lancelot.
>
>> +test_print false "" "print before any instruction"
>> +
>> +gdb_test "stepi 3" ".*" "collecting history"
>> +test_print true "" "print current after executing a bit"
>> +test_print true "-1" "print previous after executing a bit"
>> +test_print false "1" "print following after executing a bit"
>> +
>> +gdb_test "reverse-stepi" ".*" "moving back"
>> +test_print true "" "print current after reversing"
>> +test_print true "-1" "print previous after reversing"
>> +test_print true "1" "print following after reversing"
>> +
>> +test_print false "-10" "trying to print too far back"
>> +test_print false "10" "trying to print too far forward"
>> -- 
>> 2.38.1
>>

-- 
Cheers,
Bruno


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] gdb: add 'maintenance print record-instruction' command
  2023-01-02 15:37 ` Alexandra Petlanova Hajkova
@ 2023-01-03  9:00   ` Bruno Larsen
  0 siblings, 0 replies; 8+ messages in thread
From: Bruno Larsen @ 2023-01-03  9:00 UTC (permalink / raw)
  To: Alexandra Petlanova Hajkova; +Cc: gdb-patches, Eli Zaretskii

On 02/01/2023 16:37, Alexandra Petlanova Hajkova wrote:
>
>     +# This file is part of the GDB testsuite.  it tests the
>     functionality of
>     +# the maintenance print record-instruction command, but does not
>     check the
>     +# syntax, only if the command finds or fails to find recorded
>     history.
>     +
>     +if ![supports_reverse] {
>     +    return
>     +}
>     +
>
>
> I think this patch is good to go in general.
>  But it might be nice to add a small summary about how the test works 
> as a comment at the beginning of the test?
>
I see. I've expanded that starting comment to the following:

# This file is part of the GDB testsuite.  It tests the functionality of
# the maintenance print record-instruction command, but does not check the
# syntax, only if the command finds or fails to find recorded history.
# This is done by putting the inferior in mulpitle states with and without
# history to be printed, then checking if GDB is able to print an
# instruction or not.
# To identify if GDB has printed an instruction, we can see if some
# change is printed, since any instruction must have at least a change
# to the PC.

What do you think?

-- 
Cheers,
Bruno

> Regards,
> Alexandra



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] gdb: add 'maintenance print record-instruction' command
  2023-01-02 16:56   ` Bruno Larsen
@ 2023-01-03 16:10     ` Lancelot SIX
  0 siblings, 0 replies; 8+ messages in thread
From: Lancelot SIX @ 2023-01-03 16:10 UTC (permalink / raw)
  To: Bruno Larsen; +Cc: gdb-patches, Eli Zaretskii

Hi,

> > I do not think the rest of the test makes much sense if
> > [supports_process_record] returns false, does it?  I guess that
> > everything below this point should be be in the if block.
> > 
> > Another approach might to have the initial test (at the top of the file)
> > check for both supports_reverse and supports_process_record and ignore
> > the test if either feature is not supported.  WDYT?
> Good call, but since I do think it makes sense to test that GDB doesn't go
> wild even if recording is not supported, I think I'll make the test exit
> early here instead.

That seems good to me.

> > 
> > For what it is worth, the rest of the patch looks OK to me.
> Awesome, may I add your R-b tag, or do you want to see my change to the test
> first?

Sure, with the test modified as discussed above this seems OK to me.  Please add
Reviewed-By: Lancelot SIX <lancelot.six@amd.com>

Best,
Lancelot.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] gdb: add 'maintenance print record-instruction' command
  2022-12-22 15:43 [PATCH v3] gdb: add 'maintenance print record-instruction' command Bruno Larsen
  2023-01-02 15:37 ` Alexandra Petlanova Hajkova
  2023-01-02 16:26 ` Lancelot SIX
@ 2023-01-03 17:04 ` Tom Tromey
  2023-01-04 10:30 ` Bruno Larsen
  3 siblings, 0 replies; 8+ messages in thread
From: Tom Tromey @ 2023-01-03 17:04 UTC (permalink / raw)
  To: Bruno Larsen via Gdb-patches; +Cc: Bruno Larsen, Eli Zaretskii

>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

Bruno> While chasing some reverse debugging bugs, I found myself wondering what
Bruno> was recorded by GDB to undo and redo a certain instruction. This commit
Bruno> implements a simple way of printing that information.

Thanks for doing this.

Bruno> --- a/gdb/NEWS
Bruno> +++ b/gdb/NEWS
Bruno> @@ -107,6 +107,12 @@
 
Bruno>  * New commands
 
Bruno> +maintenance print record-instruction [ N ]
Bruno> +  Print the recorded information for a given instruction.  If N is not given
Bruno> +  prints how GDB would undo the last instruction executed.  If N is negative,
Bruno> +  prints how GDB would undo the N-th previous instruction, and if N is
Bruno> +  positive, it prints how GDB will redo the N-th following instruction.

You'll probably have to update this a bit to apply it to trunk, so that
it doesn't end up in the gdb 13 section.

I looked through the thread, the updated patch is ok with the NEWS
change.

Tom

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] gdb: add 'maintenance print record-instruction' command
  2022-12-22 15:43 [PATCH v3] gdb: add 'maintenance print record-instruction' command Bruno Larsen
                   ` (2 preceding siblings ...)
  2023-01-03 17:04 ` Tom Tromey
@ 2023-01-04 10:30 ` Bruno Larsen
  3 siblings, 0 replies; 8+ messages in thread
From: Bruno Larsen @ 2023-01-04 10:30 UTC (permalink / raw)
  To: gdb-patches; +Cc: Eli Zaretskii

On 22/12/2022 16:43, Bruno Larsen 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.
>
> If there isn't enough history to print the desired instruction (such as
> when the user hasn't started recording yet or when they request 2
> instructions back but only 1 was recorded), GDB warns the user like so:
>
> (gdb) maint print record-instruction
> Not enough recorded history
>
> If there is enough, GDB prints the instruction like so:
>
> (gdb) maint print record-instruction
> 4 bytes of memory at address 0x00007fffffffd5dc changed from: 01 00 00 00
> Register eflags changed: [ IF ]
> Register rip changed: (void (*)()) 0x401115 <main+15>
>
> Approved-by: Eli Zaretskii<eliz@gnu.org>

Thanks everyone who reviewed. I implemented the suggestions and pushed 
the patch

-- 
Cheers,
Bruno


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-01-04 10:30 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-22 15:43 [PATCH v3] gdb: add 'maintenance print record-instruction' command Bruno Larsen
2023-01-02 15:37 ` Alexandra Petlanova Hajkova
2023-01-03  9:00   ` Bruno Larsen
2023-01-02 16:26 ` Lancelot SIX
2023-01-02 16:56   ` Bruno Larsen
2023-01-03 16:10     ` Lancelot SIX
2023-01-03 17:04 ` Tom Tromey
2023-01-04 10:30 ` Bruno Larsen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).