Add method/format information to =record-started
Commit Message
Eclipse CDT now supports enabling execution recording using two methods
(full and btrace) and both formats for btrace (bts and pt). In the
event that recording is enabled behind the back of the GUI (by the user
on the command line, or a script), we need to know which method/format
are being used, so it can be correctly reflected in the interface. This
patch adds this information to the =record-started async record.
Before:
=record-started,thread-group="i1"
After:
=record-started,thread-group="i1",method="btrace",format="bts"
=record-started,thread-group="i1",method="btrace",format="pt"
=record-started,thread-group="i1",method="full"
The "format" field is only present when the current method supports
multiple formats (only the btrace method as of now).
gdb/ChangeLog:
* NEWS: Mention the new fields in =record-started.
* mi/mi-interp.c (mi_record_changed): Output method and format
fields in the =record-started record.
* record-btrace.c (record_btrace_open): Adapt record_changed
notification.
* record-full.c (record_full_open): Likewise.
* record.c (cmd_record_stop): Likewise.
gdb/doc/ChangeLog:
* gdb.texinfo (GDB/MI Async Records): Document method and
format fields in =record-started.
* observer.texi (record_changed): Add method and format
parameters.
gdb/testsuite/ChangeLog:
* gdb.mi/mi-record-changed.exp: Adjust =record-started output
matching.
---
gdb/NEWS | 5 +++++
gdb/doc/gdb.texinfo | 7 ++++++-
gdb/doc/observer.texi | 7 ++++++-
gdb/mi/mi-interp.c | 32 ++++++++++++++++++++++++++----
gdb/record-btrace.c | 4 +++-
gdb/record-full.c | 2 +-
gdb/record.c | 2 +-
gdb/testsuite/gdb.mi/mi-record-changed.exp | 4 ++--
8 files changed, 52 insertions(+), 11 deletions(-)
Comments
On 16-06-03 11:52 AM, Simon Marchi wrote:
> diff --git a/gdb/NEWS b/gdb/NEWS
> index dce79a2..5d59bc7 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -67,6 +67,11 @@ maint selftest
> including JIT compiling fast tracepoint's conditional expression
> bytecode into native code.
>
> +* MI async record =record-started new includes the method and format used for
Oops, typo here. new -> now.
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Simon Marchi
> Sent: Friday, June 3, 2016 5:52 PM
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@ericsson.com>
> Subject: [PATCH] Add method/format information to =record-started
>
> Eclipse CDT now supports enabling execution recording using two methods
> (full and btrace) and both formats for btrace (bts and pt). In the
> event that recording is enabled behind the back of the GUI (by the user
> on the command line, or a script), we need to know which method/format
> are being used, so it can be correctly reflected in the interface. This
> patch adds this information to the =record-started async record.
>
> Before:
>
> =record-started,thread-group="i1"
>
> After:
>
> =record-started,thread-group="i1",method="btrace",format="bts"
> =record-started,thread-group="i1",method="btrace",format="pt"
> =record-started,thread-group="i1",method="full"
>
> The "format" field is only present when the current method supports
> multiple formats (only the btrace method as of now).
Thanks for adding this.
> @@ -407,8 +409,30 @@ mi_record_changed (struct inferior *inferior, int started)
> old_chain = make_cleanup_restore_target_terminal ();
> target_terminal_ours_for_output ();
>
> - fprintf_unfiltered (mi->event_channel, "record-%s,thread-group=\"i%d\"",
> - started ? "started" : "stopped", inferior->num);
> + if (started)
> + {
> + if (format != NULL)
> + {
Do we really need braces, here...
> + fprintf_unfiltered (
> + mi->event_channel,
> + "record-started,thread-
> group=\"i%d\",method=\"%s\",format=\"%s\"",
> + inferior->num, method, format);
> + }
> + else
> + {
> +
...and here...
> + fprintf_unfiltered (
> + mi->event_channel,
> + "record-started,thread-group=\"i%d\",method=\"%s\"",
> + inferior->num, method);
> + }
> + }
> + else
> + {
...and here?
> + fprintf_unfiltered (mi->event_channel,
> + "record-stopped,thread-group=\"i%d\"", inferior-
> >num);
> + }
> +
> @@ -234,7 +235,8 @@ record_btrace_open (const char *args, int from_tty)
> NULL);
> record_btrace_generating_corefile = 0;
>
> - observer_notify_record_changed (current_inferior (), 1);
> + format = record_btrace_conf.format == BTRACE_FORMAT_PT ? "pt" : "bts";
I'd use a switch here and not assume that format != pt means bts. If we added
another format (LBR maybe?) we might miss this.
Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
"Metzger, Markus T" <markus.t.metzger@intel.com> writes:
Hi Markus,
Thanks for reviewing the patch....
>> + if (format != NULL)
>> + {
>
> Do we really need braces, here...
>
Yes, two or more lines in code should be wrapped in braces.
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
>> @@ -234,7 +235,8 @@ record_btrace_open (const char *args, int from_tty)
>> NULL);
>> record_btrace_generating_corefile = 0;
>>
>> - observer_notify_record_changed (current_inferior (), 1);
>> + format = record_btrace_conf.format == BTRACE_FORMAT_PT ? "pt" : "bts";
>
> I'd use a switch here and not assume that format != pt means bts. If we added
> another format (LBR maybe?) we might miss this.
I agree, otherwise, the patch is good to me.
On 16-06-06 09:03 AM, Yao Qi wrote:
> "Metzger, Markus T" <markus.t.metzger@intel.com> writes:
>
> Hi Markus,
> Thanks for reviewing the patch....
>
>>> + if (format != NULL)
>>> + {
>>
>> Do we really need braces, here...
>>
>
> Yes, two or more lines in code should be wrapped in braces.
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
Just to be clear, the inner if/else doesn't need them, but the outer one does.
Here's the result:
if (started)
{
if (format != NULL)
fprintf_unfiltered (
mi->event_channel,
"record-started,thread-group=\"i%d\",method=\"%s\",format=\"%s\"",
inferior->num, method, format);
else
fprintf_unfiltered (
mi->event_channel,
"record-started,thread-group=\"i%d\",method=\"%s\"",
inferior->num, method);
}
else
fprintf_unfiltered (mi->event_channel,
"record-stopped,thread-group=\"i%d\"", inferior->num);
Simon Marchi <simon.marchi@ericsson.com> writes:
> Just to be clear, the inner if/else doesn't need them, but the outer one does.
>
No, it is not "GDB C Coding Standard" compliant, IMO. In the quoted url
I gave,
"Any two or more lines in code should be wrapped in braces, even if they
are comments, as they look like separate statements:"
so...
> Here's the result:
>
>
> if (started)
> {
> if (format != NULL)
brace is needed here...
> fprintf_unfiltered (
> mi->event_channel,
> "record-started,thread-group=\"i%d\",method=\"%s\",format=\"%s\"",
> inferior->num, method, format);
and here
> else
> fprintf_unfiltered (
> mi->event_channel,
> "record-started,thread-group=\"i%d\",method=\"%s\"",
> inferior->num, method);
> }
> else
> fprintf_unfiltered (mi->event_channel,
> "record-stopped,thread-group=\"i%d\"", inferior->num);
On 16-06-07 05:32 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
>
>> Just to be clear, the inner if/else doesn't need them, but the outer one does.
>>
>
> No, it is not "GDB C Coding Standard" compliant, IMO. In the quoted url
> I gave,
>
> "Any two or more lines in code should be wrapped in braces, even if they
> are comments, as they look like separate statements:"
Ahh ok, I thought braces were only needed in the case where you added a comment.
> so...
>
>> Here's the result:
>>
>>
>> if (started)
>> {
>> if (format != NULL)
>
> brace is needed here...
>
>> fprintf_unfiltered (
>> mi->event_channel,
>> "record-started,thread-group=\"i%d\",method=\"%s\",format=\"%s\"",
>> inferior->num, method, format);
>
> and here
>
>> else
>> fprintf_unfiltered (
>> mi->event_channel,
>> "record-started,thread-group=\"i%d\",method=\"%s\"",
>> inferior->num, method);
>> }
>> else
>> fprintf_unfiltered (mi->event_channel,
>> "record-stopped,thread-group=\"i%d\"", inferior->num);
I suppose here too?
Simon Marchi <simon.marchi@ericsson.com> writes:
>>> else
>>> fprintf_unfiltered (mi->event_channel,
>>> "record-stopped,thread-group=\"i%d\"", inferior->num);
>
> I suppose here too?
Yes, I think so.
@@ -67,6 +67,11 @@ maint selftest
including JIT compiling fast tracepoint's conditional expression
bytecode into native code.
+* MI async record =record-started new includes the method and format used for
+ recording. For example:
+
+ =record-started,thread-group="i1",method="btrace",format="bts"
+
*** Changes in GDB 7.11
* GDB now supports debugging kernel-based threads on FreeBSD.
@@ -26423,12 +26423,17 @@ breakpoint commands; @xref{GDB/MI Breakpoint Commands}. The
Note that if a breakpoint is emitted in the result record of a
command, then it will not also be emitted in an async record.
-@item =record-started,thread-group="@var{id}"
+@item =record-started,thread-group="@var{id}",method="@var{method}"[,format="@var{format}"]
@itemx =record-stopped,thread-group="@var{id}"
Execution log recording was either started or stopped on an
inferior. The @var{id} is the @value{GDBN} identifier of the thread
group corresponding to the affected inferior.
+The @var{method} field contains the method used to record execution. If the
+method in use supports multiple recording formats, @var{format} will be present
+and contain the currently used format. See @xref{Process Record and Replay}
+for existing method and format values.
+
@item =cmd-param-changed,param=@var{param},value=@var{value}
Reports that a parameter of the command @code{set @var{param}} is
changed to @var{value}. In the multi-word @code{set} command,
@@ -141,11 +141,16 @@ at the entry-point instruction. For @samp{attach} and @samp{core},
inferior, and before any information on the inferior has been printed.
@end deftypefun
-@deftypefun void record_changed (struct inferior *@var{inferior}, int @var{started})
+@deftypefun void record_changed (struct inferior *@var{inferior}, int @var{started}, const char *@var{method}, const char *@var{format})
The status of process record for inferior @var{inferior} in
@value{GDBN} has changed. The process record is started if
@var{started} is true, and the process record is stopped if
@var{started} is false.
+
+When @var{started} is true, @var{method} indicates the short name of the method
+used for recording. If the method supports multiple formats, @var{format}
+indicates which one is being used, otherwise it is NULL. When @var{started} is
+false, they are both NULL.
@end deftypefun
@deftypefun void solib_loaded (struct so_list *@var{solib})
@@ -65,7 +65,8 @@ static void mi_on_no_history (void);
static void mi_new_thread (struct thread_info *t);
static void mi_thread_exit (struct thread_info *t, int silent);
-static void mi_record_changed (struct inferior*, int);
+static void mi_record_changed (struct inferior*, int, const char *,
+ const char *);
static void mi_inferior_added (struct inferior *inf);
static void mi_inferior_appeared (struct inferior *inf);
static void mi_inferior_exit (struct inferior *inf);
@@ -399,7 +400,8 @@ mi_thread_exit (struct thread_info *t, int silent)
/* Emit notification on changing the state of record. */
static void
-mi_record_changed (struct inferior *inferior, int started)
+mi_record_changed (struct inferior *inferior, int started, const char *method,
+ const char *format)
{
struct mi_interp *mi = (struct mi_interp *) top_level_interpreter_data ();
struct cleanup *old_chain;
@@ -407,8 +409,30 @@ mi_record_changed (struct inferior *inferior, int started)
old_chain = make_cleanup_restore_target_terminal ();
target_terminal_ours_for_output ();
- fprintf_unfiltered (mi->event_channel, "record-%s,thread-group=\"i%d\"",
- started ? "started" : "stopped", inferior->num);
+ if (started)
+ {
+ if (format != NULL)
+ {
+ fprintf_unfiltered (
+ mi->event_channel,
+ "record-started,thread-group=\"i%d\",method=\"%s\",format=\"%s\"",
+ inferior->num, method, format);
+ }
+ else
+ {
+
+ fprintf_unfiltered (
+ mi->event_channel,
+ "record-started,thread-group=\"i%d\",method=\"%s\"",
+ inferior->num, method);
+ }
+ }
+ else
+ {
+ fprintf_unfiltered (mi->event_channel,
+ "record-stopped,thread-group=\"i%d\"", inferior->num);
+ }
+
gdb_flush (mi->event_channel);
@@ -206,6 +206,7 @@ record_btrace_open (const char *args, int from_tty)
{
struct cleanup *disable_chain;
struct thread_info *tp;
+ const char *format;
DEBUG ("open");
@@ -234,7 +235,8 @@ record_btrace_open (const char *args, int from_tty)
NULL);
record_btrace_generating_corefile = 0;
- observer_notify_record_changed (current_inferior (), 1);
+ format = record_btrace_conf.format == BTRACE_FORMAT_PT ? "pt" : "bts";
+ observer_notify_record_changed (current_inferior (), 1, "btrace", format);
discard_cleanups (disable_chain);
}
@@ -869,7 +869,7 @@ record_full_open (const char *name, int from_tty)
record_full_init_record_breakpoints ();
- observer_notify_record_changed (current_inferior (), 1);
+ observer_notify_record_changed (current_inferior (), 1, "full", NULL);
}
/* "to_close" target method. Close the process record target. */
@@ -268,7 +268,7 @@ cmd_record_stop (char *args, int from_tty)
printf_unfiltered (_("Process record is stopped and all execution "
"logs are deleted.\n"));
- observer_notify_record_changed (current_inferior (), 0);
+ observer_notify_record_changed (current_inferior (), 0, NULL, NULL);
}
/* The "set record" command. */
@@ -31,14 +31,14 @@ if [mi_gdb_start] {
}
mi_run_to_main
-mi_gdb_test "record" ".*=record-started,thread-group=\"i${decimal}\".*\\^done" \
+mi_gdb_test "record" ".*=record-started,thread-group=\"i${decimal}\",method=\"full\".*\\^done" \
"record"
mi_gdb_test "record stop" \
".*=record-stopped,thread-group=\"i${decimal}\".*\\^done" \
"record end"
mi_gdb_test "target record" \
- ".*=record-started,thread-group=\"i${decimal}\".*\\^done" \
+ ".*=record-started,thread-group=\"i${decimal}\",method=\"full\".*\\^done" \
"target record"
return 0