Add method/format information to =record-started

  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?
  

> -----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