gdb: add 'maintenance print record-instruction' command
Commit Message
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(+)
Comments
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
>
On 07/12/2022 15:37, Lancelot SIX wrote:
> 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.
None maintenance_print functions end with _command, so I don't think it
is a hard rule, but if you think I should add it, I can do it.
I agree with everything else you said, it'll all be fixed in v2.
> None maintenance_print functions end with _command, so I don't think it is a
> hard rule, but if you think I should add it, I can do it.
Indeed, maintenance commands do not have seem to have this prefix, so
you should leave it as it is. My bad.
Best,
Lancelot.
>
> I agree with everything else you said, it'll all be fixed in v2.
>
> --
> Cheers,
> Bruno
>
>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
>> +static void
>> +maintenance_print_record_instruction (const char *args, int from_tty)
Lancelot> I think, usually the command implementation have a _command suffix in
Lancelot> the name. I do not know if this is a hard rule.
Historically there were a couple of conventions like this, but IMO as
long as the command implementation has the command name in it, then that
is great -- it makes it easy to find it via tags.
Tom
>>>>> "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. The idea seems fine to me.
Bruno> + gdb_byte* b = record_full_get_loc (to_print);
The "* " should be " *", there's a few of these.
Bruno> + gdb_printf (" %02x",b[i]);
Missing a space after the ",", also a few of these.
Bruno> + add_cmd ("record-instruction", class_maintenance,
Bruno> + maintenance_print_record_instruction,
Bruno> + _("\
Bruno> +Print a recorded instruction.\nIf no argument is provided, print the last \
Bruno> +instruction recorded.\nIf a negative argument is given, prints how the nth \
Bruno> +previous instruction will be undone.\nIf a positive argument is given, prints \
Bruno> +how the nth following instruction will be redone."), &maintenanceprintlist);
IMO it's better to end the lines with \n\ so that the line breaks are
very clear when reading the text in the source.
Tom
@@ -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
@@ -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{]}
@@ -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. */
+ while (to_print->next && offset > 0)
+ {
+ to_print = to_print->next;
+ if (!to_print->type)
+ offset --;
+ }
+ if (offset != 0)
+ error (_("Not enough recorded history"));
+ }
+ else
+ {
+ while (to_print->prev && offset < 0)
+ {
+ to_print = to_print->prev;
+ if (!to_print->type)
+ 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)
+ 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);
}