RE: [PATCH v5 2/7] add btrace coresight related commands

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Zied,

>This patch extends the commands needed for using branch tracing
>with ARM CoreSight traces.
>Those commands are:
>set record btrace etm sink
>set record btrace etm buffer-size
>record btrace etm

This patch looks good to me.  I'll skip it in future reviews.  Please point
out updates explicitly.

See a few nits below.


>diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>index d13cbda2c11..48627d5d71a 100644
>--- a/gdb/doc/gdb.texinfo
>+++ b/gdb/doc/gdb.texinfo
>@@ -7796,6 +7816,29 @@ also need longer to process the branch trace data
>before it can be used.
> Show the current setting of the requested ring buffer size for branch
> tracing in Intel Processor Trace format.
>
>+@kindex set record btrace etm
>+@item set record btrace etm buffer-size @var{size}
>+@itemx set record btrace etm buffer-size unlimited
>+Set the requested ring buffer size for branch tracing in ARM
>+CoreSight ETM Trace format.  Default is 16KB.
>+
>+If @var{size} is a positive number, then @value{GDBN} will try to
>+allocate a buffer of at least @var{size} bytes for each new thread
>+that uses the btrace recording method and the ARM CoreSight ETM
>+format.  The actually obtained buffer size may differ from the
>+requested @var{size}.  Use the @code{info record} command to see the
>+actual buffer size for each thread.
>+
>+If @var{limit} is @code{unlimited} or zero, @value{GDBN} will try to
>+allocate a buffer of 4MB.

Is this 4MB or rather 4GB?


>diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
>index b7b3c91f85d..83768694193 100644
>--- a/gdb/record-btrace.c
>+++ b/gdb/record-btrace.c
>@@ -2675,7 +2699,7 @@ record_btrace_target::stop (ptid_t ptid)
> 	  tp->btrace.flags |= BTHR_STOP;
> 	}
>     }
>- }
>+}

That looks odd.  What changed?


>@@ -3233,4 +3342,6 @@ to see the actual buffer size."), NULL,
>show_record_pt_buffer_size_value,
>
>   record_btrace_conf.bts.size = 64 * 1024;
>   record_btrace_conf.pt.size = 16 * 1024;
>+#define DEFAULT_ETM_BUFFER_SIZE (8 * 1024)
>+  record_btrace_conf.etm.size = DEFAULT_ETM_BUFFER_SIZE;
> }

Doesn't look like this macro is used anywhere else so I don't quite see the
point of it.


>diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h
>index 26d26ec957f..153b977723a 100644
>--- a/gdbsupport/btrace-common.h
>+++ b/gdbsupport/btrace-common.h
>@@ -119,6 +122,20 @@ struct btrace_config_pt
>   unsigned int size;
> };
>
>+/* An ARM CoreSight ETM Trace configuration.  */
>+
>+struct btrace_config_etm
>+{
>+  /* The size of the branch trace buffer in bytes.
>+
>+     This is unsigned int and not size_t since it is registered as
>+     control variable for "set record btrace etm buffer-size".  */
>+  unsigned int size;
>+
>+  /* The sink used to collect the traces.  */
>+  char *sink;

Should this be const char *?  Who would free the memory used to
store those sink names?

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928