Re: [PATCH 05/12] record-btrace: add bts buffer size configuration option

[Pedro Alves <palves at redhat dot com>]

On 07/14/2014 02:56 PM, Markus Metzger wrote:
> --- a/gdb/features/btrace-conf.dtd
> +++ b/gdb/features/btrace-conf.dtd
> @@ -5,6 +5,7 @@
>       notice and this notice are preserved.  -->
>  
>  <!ELEMENT btrace-conf	(bts?)>
> -<!ATTLIST btrace-conf	version	CDATA	#FIXED "1.0">
> +<!ATTLIST btrace-conf	version	CDATA	#FIXED "1.1">

I don't see why we need to bump the version.  GDB that know
about attribute cope with it being missing.  And GDBs that aren't
expecting the attribute just ignore it.

>
>  <!ELEMENT bts	EMPTY>
> +<!ATTLIST bts	size	CDATA	#IMPLIED>

>  supported_btrace_packets (char *buf)
>  {
>    if (target_supports_btrace (BTRACE_FORMAT_BTS))
> -    strcat (buf, ";Qbtrace:bts+");
> +    {
> +      strcat (buf, ";Qbtrace:bts+");
> +      strcat (own_buf, ";Qbtrace-conf:bts:size+");
> +    }

'buf' or 'own_buf' ?

> +  if (pages == 0) {

Misplaced {

> +    errno = EINVAL;
> +    goto err;
> +  }
> +



> +/* Adjusts the size and returns a human readable size suffix.  */
> +
> +static const char *
> +record_btrace_adjust_size (unsigned int *size)
> +{
> +  unsigned int sz;
> +
> +  sz = *size;
> +
> +  if ((sz & ((1u << 30) - 1)) == 0)
> +    {
> +      *size = sz >> 30;
> +      return "GB";
> +    }
> +  else if ((sz & ((1u << 20) - 1)) == 0)
> +    {
> +      *size = sz >> 20;
> +      return "MB";
> +    }
> +  else if ((sz & ((1u << 10) - 1)) == 0)
> +    {
> +      *size = sz >> 10;
> +      return "KB";

Pedantically, this should be "kB".




> +  add_setshow_uinteger_cmd ("buffer-size", no_class,
> +			    &record_btrace_conf.bts.size,
> +			    _("Set the record/replay bts buffer size."),
> +			    _("Show the record/replay bts buffer size."), _("\
> +Bigger buffers allow longer recording but also take more time to process \
> +the recorded execution.\n\
> +The actual buffer size may differ from the requested size.  Use \"info record\" \
> +to see the actual buffer size."), NULL, NULL,
> +			    &set_record_btrace_bts_cmdlist,
> +			    &show_record_btrace_bts_cmdlist);

I think the documentation of these knobs should mention that
the setting takes effect the next time btrace is enabled, not
immediately.



> +gdb_test_no_output "set record btrace bts buffer-size 1"
> +gdb_test "show record btrace bts buffer-size" "The record/replay bts buffer size is 1\.\r"
> +
> +gdb_test_no_output "record btrace bts"
> +gdb_test "show record btrace bts buffer-size" "The record/replay bts buffer size is 1\.\r"

See:
  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

Thanks,
Pedro Alves