Add method/format information to =record-started
Commit Message
On 16-06-06 02:37 AM, Metzger, Markus T wrote:
>> @@ -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...
Right, thanks.
>> + fprintf_unfiltered (
>> + mi->event_channel,
>> + "record-started,thread-
>> group=\"i%d\",method=\"%s\",format=\"%s\"",
>> + inferior->num, method, format);
>> + }
>> + else
>> + {
>> +
>
> ...and here...
Same.
>> + fprintf_unfiltered (
>> + mi->event_channel,
>> + "record-started,thread-group=\"i%d\",method=\"%s\"",
>> + inferior->num, method);
>> + }
>> + }
>> + else
>> + {
>
> ...and here?
Ditto.
>> + 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.
You're right. What do you think of adding a convenience function like this?
Comments
> -----Original Message-----
> From: Simon Marchi [mailto:simon.marchi@ericsson.com]
> Sent: Monday, June 6, 2016 3:07 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH] Add method/format information to =record-started
Hi Simon,
Please see Yao's reply regarding formatting.
> >> @@ -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.
>
> You're right. What do you think of adding a convenience function like this?
>
>
> diff --git a/gdb/common/btrace-common.c b/gdb/common/btrace-common.c
> index eba3979..ebf3ed2 100644
> --- a/gdb/common/btrace-common.c
> +++ b/gdb/common/btrace-common.c
> @@ -43,6 +43,23 @@ btrace_format_string (enum btrace_format format)
>
> /* See btrace-common.h. */
>
> +const char *
> +btrace_format_short_string (enum btrace_format format)
> +{
> + switch (format)
> + {
> + case BTRACE_FORMAT_BTS:
> + return "bts";
> +
> + case BTRACE_FORMAT_PT:
> + return "pt";
> + }
> +
> + internal_error (__FILE__, __LINE__, _("Unknown branch trace format"));
> +}
> +
> +/* See btrace-common.h. */
> +
> void
> btrace_data_init (struct btrace_data *data)
> {
> diff --git a/gdb/common/btrace-common.h b/gdb/common/btrace-common.h
> index ad208cd..ba707a2 100644
> --- a/gdb/common/btrace-common.h
> +++ b/gdb/common/btrace-common.h
> @@ -214,6 +214,10 @@ enum btrace_error
> /* Return a string representation of FORMAT. */
> extern const char *btrace_format_string (enum btrace_format format);
>
> +/* Return a short abbreviation string of FORMAT. FORMAT can have the value
> + BTRACE_FORMAT_NONE. */
> +extern const char *btrace_format_short_string (enum btrace_format format);
Looks good except that BTRACE_FORMAT_NONE is not handled in the switch.
We could leave it as a bad format string to be detected by the MI consumer. This would
leave the short and long versions of btrace_format_string symmetric.
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
On 16-06-06 09:20 AM, Metzger, Markus T wrote:
> Looks good except that BTRACE_FORMAT_NONE is not handled in the switch.
>
> We could leave it as a bad format string to be detected by the MI consumer. This would
> leave the short and long versions of btrace_format_string symmetric.
What do you mean by MI consumer? The MI front-end (e.g. Eclipse), or the MI interpreter
code in gdb (mi-interp.c) ?
My thought was that not handling it in the switch (and let gdb implode) would help catch
a bug earlier, compared to emitting some invalid string. But I don't really mind. I can
add
case BTRACE_FORMAT_NONE:
return "unknown";
> -----Original Message-----
> From: Simon Marchi [mailto:simon.marchi@ericsson.com]
> Sent: Monday, June 6, 2016 3:40 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH] Add method/format information to =record-started
>
> On 16-06-06 09:20 AM, Metzger, Markus T wrote:
> > Looks good except that BTRACE_FORMAT_NONE is not handled in the switch.
> >
> > We could leave it as a bad format string to be detected by the MI consumer.
> This would
> > leave the short and long versions of btrace_format_string symmetric.
>
> What do you mean by MI consumer? The MI front-end (e.g. Eclipse), or the MI
> interpreter
> code in gdb (mi-interp.c) ?
I was thinking about Eclipse. It has to handle unknown (e.g. new) formats, anyway.
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
@@ -43,6 +43,23 @@ btrace_format_string (enum btrace_format format)
/* See btrace-common.h. */
+const char *
+btrace_format_short_string (enum btrace_format format)
+{
+ switch (format)
+ {
+ case BTRACE_FORMAT_BTS:
+ return "bts";
+
+ case BTRACE_FORMAT_PT:
+ return "pt";
+ }
+
+ internal_error (__FILE__, __LINE__, _("Unknown branch trace format"));
+}
+
+/* See btrace-common.h. */
+
void
btrace_data_init (struct btrace_data *data)
{
@@ -214,6 +214,10 @@ enum btrace_error
/* Return a string representation of FORMAT. */
extern const char *btrace_format_string (enum btrace_format format);
+/* Return a short abbreviation string of FORMAT. FORMAT can have the value
+ BTRACE_FORMAT_NONE. */
+extern const char *btrace_format_short_string (enum btrace_format format);
+
/* Initialize DATA. */
extern void btrace_data_init (struct btrace_data *data);
@@ -235,7 +235,7 @@ record_btrace_open (const char *args, int from_tty)
NULL);
record_btrace_generating_corefile = 0;
- format = record_btrace_conf.format == BTRACE_FORMAT_PT ? "pt" : "bts";
+ format = btrace_format_short_string (record_btrace_conf.format);
observer_notify_record_changed (current_inferior (), 1, "btrace", format);
discard_cleanups (disable_chain);