Re: [patch v4 05/13] linux, btrace: perf_event based branch tracing

[Pedro Alves <palves at redhat dot com>]

On 11/27/2012 10:49 AM, markus.t.metzger@intel.com wrote:
> From: Markus Metzger <markus.t.metzger@intel.com>
> 
> Implement branch tracing on Linux based on perf_event such taht it can be shared
> between gdb and gdbserver.
> 
> The actual btrace target ops will be implemented on top.
> 
> 2012-11-27 Markus Metzger <markus.t.metzger@intel.com>
> 
> 	* common/linux_btrace.h: New file.
> 	* common/linux_btrace.c: New file.
> 	* Makefile.in: Add linux-btrace rules.

Please spell out the rules in the change log.  Grep for "New rule"
in existing entries for examples.

> 
> gdbserver/
> 	* Makefile.in: Add linux-btrace rules.

Ditto.

> 
> 
> ---
>  gdb/Makefile.in           |    6 +-
>  gdb/common/linux-btrace.c |  382 +++++++++++++++++++++++++++++++++++++++++++++
>  gdb/common/linux-btrace.h |   76 +++++++++
>  gdb/gdbserver/Makefile.in |    6 +-
>  4 files changed, 468 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/common/linux-btrace.c
>  create mode 100644 gdb/common/linux-btrace.h
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index a8dbe83..584de8a 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -833,7 +833,7 @@ gnulib/import/extra/snippet/arg-nonnull.h gnulib/import/extra/snippet/c++defs.h
>  gnulib/import/extra/snippet/warn-on-use.h \
>  gnulib/import/stddef.in.h gnulib/import/inttypes.in.h inline-frame.h skip.h \
>  common/common-utils.h common/xml-utils.h common/buffer.h common/ptid.h \
> -common/format.h common/host-defs.h utils.h \
> +common/format.h common/host-defs.h common/linux-btrace.h utils.h \
>  common/linux-osdata.h gdb-dlfcn.h auto-load.h probe.h stap-probe.h gdb_bfd.h
>  
>  # Header files that already have srcdir in them, or which are in objdir.
> @@ -1950,6 +1950,10 @@ vec.o: ${srcdir}/common/vec.c
>  	$(COMPILE) $(srcdir)/common/vec.c
>  	$(POSTCOMPILE)
>  
> +linux-btrace.o: ${srcdir}/common/linux-btrace.c
> +	$(COMPILE) $(srcdir)/common/linux-btrace.c
> +	$(POSTCOMPILE)
> +
>  #
>  # gdb/tui/ dependencies
>  #
> diff --git a/gdb/common/linux-btrace.c b/gdb/common/linux-btrace.c
> new file mode 100644
> index 0000000..da858e1
> --- /dev/null
> +++ b/gdb/common/linux-btrace.c
> @@ -0,0 +1,382 @@
> +/* Linux-dependent part of branch trace support for GDB, and GDBserver.
> +
> +   Copyright (C) 2012 Free Software Foundation, Inc.
> +
> +   Contributed by Intel Corp. <markus.t.metzger@intel.com>
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* Needed for _ () used in gdb_assert ().  */

Drop this comment.  server.h/defs.h always need to be included, and always
need to be the first header included.

> +#ifdef GDBSERVER
> +#include "server.h"
> +#else
> +#include "defs.h"
> +#endif
> +
> +#include "linux-btrace.h"
> +#include "common-utils.h"
> +#include "gdb_assert.h"
> +
> +#if HAVE_LINUX_PERF_EVENT_H
> +
> +#include <errno.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +#include <sys/mman.h>
> +#include <sys/user.h>
> +
> +#if defined(__GNUC__)
> +#  define memory_barrier() asm volatile ("" : : : "memory")
> +#else
> +#  define memory_barrier() do {} while (0)
> +#endif
> +
> +/* A branch trace record in perf_event.  */
> +struct perf_event_bts
> +{
> +  uint64_t from;
> +  uint64_t to;
> +};
> +
> +/* A perf_event branch trace sample.  */
> +struct perf_event_sample
> +{
> +  struct perf_event_header header;
> +  struct perf_event_bts bts;
> +};

Please add comments for the fields as well.


> +
> +/* Get the perf_event header.  */
> +static inline volatile struct perf_event_mmap_page *

Empty line between describing comment and function, here and
everywhere.

> +perf_event_header (struct btrace_target_info* tinfo)
> +{
> +  return tinfo->buffer;
> +}
> +
> +/* Get the size of the perf_event mmap buffer.  */
> +static inline size_t
> +perf_event_mmap_size (const struct btrace_target_info *tinfo)
> +{
> +  /* The branch trace buffer is preceded by a configuration page.  */
> +  return (tinfo->size + 1) * PAGE_SIZE;
> +}
> +
> +/* Get the size of the perf_event buffer.  */
> +static inline size_t
> +perf_event_buffer_size (struct btrace_target_info* tinfo)
> +{
> +  return tinfo->size * PAGE_SIZE;
> +}
> +
> +/* Get the start address of the perf_event buffer.  */
> +static inline const uint8_t *
> +perf_event_buffer_begin (struct btrace_target_info* tinfo)
> +{
> +  return ((const uint8_t *) tinfo->buffer) + PAGE_SIZE;
> +}
> +
> +/* Get the end address of the perf_event buffer.  */
> +static inline const uint8_t *
> +perf_event_buffer_end (struct btrace_target_info* tinfo)
> +{
> +  return perf_event_buffer_begin (tinfo) + perf_event_buffer_size (tinfo);
> +}
> +
> +/* Check whether an address is in the kernel.  */
> +static inline int
> +perf_event_is_kernel_addr (const struct btrace_target_info *tinfo,
> +			   uint64_t addr)
> +{
> +  return tinfo->ptr_bits && (addr & ((uint64_t) 1 << (tinfo->ptr_bits - 1)));

This magic deserves a comment.

> +}
> +
> +/* Check whether a perf_event record should be skipped.  */
> +static inline int
> +perf_event_skip_record (const struct btrace_target_info *tinfo,
> +			const struct perf_event_bts *bts)
> +{
> +  return perf_event_is_kernel_addr (tinfo, bts->from);

  /* We're only interested in (...fill me...).  */

> +}
> +
> +/* Check whether a perf_event sample record is OK.  */

What is the definition of "OK"?  (I understand it from reading
the code further, but the point of the description should be spare
the reader that trouble).

> +static inline int
> +perf_event_sample_ok (const struct perf_event_sample *sample)
> +{
> +  if (sample->header.type != PERF_RECORD_SAMPLE)
> +    return 0;
> +
> +  if (sample->header.size != sizeof (*sample))
> +    return 0;

Is this really safe?  Can't we be looking at the middle of an event
that happens to have coincidentally have those values in the right place?

> +
> +  return 1;
> +}
> +
> +/* Branch trace is collected in a circular buffer [begin; end) as pairs of from
> +   and to addresses (plus some header).

"plus a header"?

> +
> +   Start points into that buffer at the next sample position.
> +   We read the collected samples backwards from start.
> +
> +   While reading the samples, we convert the information into a list of blocks.
> +   For two adjacent samples s1 and s2, we form a block b such that b.begin =
> +   s1.to and b.end = s2.from.
> +
> +   In case the buffer overflows during sampling, samples may be split.  */

"may be split" - what does that mean?  What's the visible effect?

> +
> +static VEC (btrace_block_s) *
> +perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
> +		     const uint8_t *end, const uint8_t *start)
> +{
> +  VEC (btrace_block_s) *btrace = NULL;
> +  struct perf_event_sample sample;
> +  int read = 0, size = (end - begin);

"int" doesn't look like the proper type here.  Other similar cases in the function.

> +  struct btrace_block block = { 0, 0 };
> +
> +  gdb_assert (begin <= start);
> +  gdb_assert (start <= end);
> +
> +  /* The buffer may contain a partial record as its last entry (i.e. when the
> +     buffer size is not a mulitple of the sample size).  */
> +  read = sizeof (sample) - 1;

Typo: "multiple".

> +
> +  for (; read < size; read += sizeof (sample))



> +    {
> +      const struct perf_event_sample *psample;
> +
> +      /* Find the next perf_event sample.  */
> +      start -= sizeof (sample);
> +      if (begin <= start)
> +	psample = (const struct perf_event_sample *) start;

Is something taking care of making sure these casts are valid, wrt
to e.g., resulting in a pointer with a valid alignment?
It seems like perf_event_sample_ok is relying on undefined behavior.


> +      else
> +	{
> +	  int missing = (begin - start);
> +
> +	  start = (end - missing);
> +
> +	  if (missing == sizeof (sample))
> +	    psample = (const struct perf_event_sample *) start;
> +	  else
> +	    {
> +	      uint8_t *stack = (uint8_t *) &sample;
> +
> +	      memcpy (stack, start, missing);
> +	      memcpy (stack + missing, begin, sizeof (sample) - missing);
> +
> +	      psample = &sample;
> +	    }

This could use a couple comments.

> +	}
> +
> +      if (!perf_event_sample_ok (psample))
> +	{
> +	  warning (_("Branch trace may be incomplete."));

Is there anything the user can do when she sees this warning?  Should
it be a bit more descriptive?

> +	  break;
> +	}
> +
> +      if (perf_event_skip_record (tinfo, &psample->bts))
> +	continue;
> +
> +      /* We found a valid sample, so we can complete the current block.  */
> +      block.begin = psample->bts.to;
> +
> +      VEC_safe_push (btrace_block_s, btrace, &block);
> +
> +      /* Start the next block.  */
> +      block.end = psample->bts.from;
> +    }
> +
> +  return btrace;
> +}
> +
> +/* See linux-btrace.h.  */
> +int
> +linux_supports_btrace (void)
> +{
> +  return 1;
> +}
> +
> +/* See linux-btrace.h.  */
> +int
> +linux_btrace_has_changed (struct btrace_target_info *tinfo)
> +{
> +  volatile struct perf_event_mmap_page *header = perf_event_header (tinfo);
> +
> +  if (!header)
> +    return 0;

When can this happen?

> +
> +  return header->data_head != tinfo->data_head;
> +}
> +
> +/* See linux-btrace.h.  */
> +struct btrace_target_info *
> +linux_enable_btrace (ptid_t ptid)
> +{
> +  struct btrace_target_info *tinfo;
> +  int pid;
> +
> +  tinfo = xzalloc (sizeof (*tinfo));
> +  tinfo->attr.size = sizeof (tinfo->attr);
> +
> +  tinfo->attr.type = PERF_TYPE_HARDWARE;
> +  tinfo->attr.config = PERF_COUNT_HW_BRANCH_INSTRUCTIONS;
> +  tinfo->attr.sample_period = 1;
> +
> +  /* We sample from and to address.  */
> +  tinfo->attr.sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_ADDR;
> +
> +  tinfo->attr.exclude_kernel = 1;

If we do this, do we still need perf_event_skip_record/perf_event_is_kernel_addr?

> +  tinfo->attr.exclude_hv = 1;
> +  tinfo->attr.exclude_idle = 1;
> +
> +  tinfo->ptr_bits = 0;
> +
> +  pid = ptid_get_lwp (ptid);
> +  if (!pid)
> +    pid = ptid_get_pid (ptid);
> +
> +  errno = 0;
> +  tinfo->file = syscall (SYS_perf_event_open, &(tinfo->attr), pid, -1, -1, 0);

Unnecessary parens.

> +  if (tinfo->file < 0)
> +    goto err;
> +
> +  /* We hard-code the trace buffer size.
> +     At some later time, we should make this configurable.  */
> +  tinfo->size = 1;
> +  tinfo->buffer = mmap (NULL, perf_event_mmap_size (tinfo),
> +			PROT_READ, MAP_SHARED, tinfo->file, 0);
> +  if (tinfo->buffer == MAP_FAILED)
> +    goto err_file;
> +
> +  return tinfo;
> +
> +err_file:
> +  close (tinfo->file);
> +
> +err:
> +  xfree (tinfo);
> +  return NULL;
> +}
> +
> +/* See linux-btrace.h.  */
> +int
> +linux_disable_btrace (struct btrace_target_info *tinfo)
> +{
> +  int errcode;
> +
> +  if (!tinfo)
> +    return EINVAL;

How can this happen?

> +
> +  errno = 0;
> +  errcode = munmap (tinfo->buffer, perf_event_mmap_size (tinfo));
> +  if (errcode)
> +    return errno;
> +
> +  close (tinfo->file);
> +  xfree (tinfo);
> +
> +  return 0;
> +}
> +
> +/* See linux-btrace.h.  */
> +VEC (btrace_block_s) *
> +linux_read_btrace (struct btrace_target_info *tinfo)
> +{
> +  VEC (btrace_block_s) *btrace = NULL;
> +  volatile struct perf_event_mmap_page *header;
> +  const uint8_t *begin, *end, *start;
> +  unsigned long data_head, retries = 5;
> +  size_t buffer_size;
> +
> +  header = perf_event_header (tinfo);
> +  if (!header)
> +    return btrace;
> +
> +  buffer_size = perf_event_buffer_size (tinfo);
> +
> +  /* We may need to retry reading the trace.  See below.  */
> +  while (retries--)
> +    {
> +      data_head = header->data_head;
> +
> +      /* Make sure the trace data is up-to-date.  */
> +      memory_barrier ();
> +
> +      /* If there new trace, let's read it.  */

Typo: "there's".

> +      if (data_head != tinfo->data_head)
> +	{
> +	  /* Data_head keeps growing; the buffer itself is circular.  */
> +	  begin = perf_event_buffer_begin (tinfo);
> +	  start = begin + (data_head % buffer_size);

Redundant parens.

> +
> +	  if (data_head <= buffer_size)
> +	    end = start;
> +	  else
> +	    end = perf_event_buffer_end (tinfo);
> +
> +	  btrace = perf_event_read_bts (tinfo, begin, end, start);
> +	}
> +
> +      /* The stopping thread notifies its ptracer before it is scheduled out.
> +	 On multi-core systems, the debugger might therefore run while the
> +	 kernel might be writing the last branch trace records.

Is this working around something that should be changed in the kernel?

> +
> +	 Let's check whether the data head moved while we read the trace.  */
> +      if (data_head == header->data_head)
> +	break;
> +    }

What does it mean then if retries reaches 0 ?

> +
> +  tinfo->data_head = data_head;
> +
> +  return btrace;
> +}
> +
> +#else /* !HAVE_LINUX_PERF_EVENT_H */
> +
> +/* See linux-btrace.h.  */
> +int
> +linux_supports_btrace (void)
> +{
> +  return 0;
> +}
> +
> +/* See linux-btrace.h.  */
> +int
> +linux_btrace_has_changed (struct btrace_target_info *tinfo)
> +{
> +  return 0;
> +}
> +
> +/* See linux-btrace.h.  */
> +struct btrace_target_info *
> +linux_enable_btrace (ptid_t ptid)
> +{
> +  return NULL;
> +}
> +
> +/* See linux-btrace.h.  */
> +int
> +linux_disable_btrace (struct btrace_target_info *tinfo)
> +{
> +  return ENOSYS;
> +}
> +
> +/* See linux-btrace.h.  */
> +VEC (btrace_block_s) *
> +linux_read_btrace (struct btrace_target_info *tinfo)
> +{
> +}

This doesn't look like would even compile.  Please be sure to
test the case of when the perf header isn't found.

> +
> +#endif /* !HAVE_LINUX_PERF_EVENT_H */
> diff --git a/gdb/common/linux-btrace.h b/gdb/common/linux-btrace.h
> new file mode 100644
> index 0000000..b0f8459
> --- /dev/null
> +++ b/gdb/common/linux-btrace.h
> @@ -0,0 +1,76 @@
> +/* Linux-dependent part of branch trace support for GDB, and GDBserver.
> +
> +   Copyright (C) 2012 Free Software Foundation, Inc.
> +
> +   Contributed by Intel Corp. <markus.t.metzger@intel.com>
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef LINUX_BTRACE_H
> +#define LINUX_BTRACE_H
> +
> +#include "btrace-common.h"
> +#include "config.h"

This include of "config.h" here, if necessary, hints at something else wrong.
The .c files should always include defs.h/server.h first.

> +#include "vec.h"
> +#include "ptid.h"
> +#include <stddef.h>
> +#include <stdint.h>
> +
> +#if HAVE_LINUX_PERF_EVENT_H
> +#  include <linux/perf_event.h>
> +#endif
> +
> +/* Branch trace target information per thread.  */
> +struct btrace_target_info
> +{
> +#if HAVE_LINUX_PERF_EVENT_H
> +  /* The Linux perf_event configuration for collecting the branch trace.  */
> +  struct perf_event_attr attr;
> +
> +  /* The mmap configuration mapping the branch trace perf_event buffer.
> +
> +     file      .. the file descriptor
> +     buffer    .. the mmapped memory buffer
> +     size      .. the buffer's size in pages without the configuration page
> +     data_head .. the data head from the last read  */
> +  int file;
> +  void *buffer;
> +  size_t size;
> +  unsigned long data_head;
> +#endif /* HAVE_LINUX_PERF_EVENT_H */
> +
> +  /* The size of a pointer in bits for this thread.
> +     The information is used to identify kernel addresses in order to skip
> +     records from/to kernel space.  */
> +  int ptr_bits;
> +};
> +
> +/* Check whether branch tracing is supported.  */
> +extern int linux_supports_btrace (void);
> +
> +/* Enable branch tracing for @ptid.  */
> +extern struct btrace_target_info *linux_enable_btrace (ptid_t ptid);
> +
> +/* Disable branch tracing and deallocate @tinfo.  */
> +extern int linux_disable_btrace (struct btrace_target_info *tinfo);
> +
> +/* Check whether there is new trace data available.  */
> +extern int linux_btrace_has_changed (struct btrace_target_info *);
> +
> +/* Read branch trace data.  */
> +extern VEC (btrace_block_s) *linux_read_btrace (struct btrace_target_info *);
> +
> +#endif /* LINUX_BTRACE_H */
> diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
> index fc4fd1d..c85d6b4 100644
> --- a/gdb/gdbserver/Makefile.in
> +++ b/gdb/gdbserver/Makefile.in
> @@ -143,7 +143,7 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
>  	$(srcdir)/common/vec.c $(srcdir)/common/gdb_vecs.c \
>  	$(srcdir)/common/common-utils.c $(srcdir)/common/xml-utils.c \
>  	$(srcdir)/common/linux-osdata.c $(srcdir)/common/ptid.c \
> -	$(srcdir)/common/buffer.c
> +	$(srcdir)/common/buffer.c $(srcdir)/common/linux-btrace.c
>  
>  DEPFILES = @GDBSERVER_DEPFILES@
>  
> @@ -412,6 +412,7 @@ signals_h = $(srcdir)/../../include/gdb/signals.h $(signals_def)
>  ptid_h = $(srcdir)/../common/ptid.h
>  ax_h = $(srcdir)/ax.h
>  agent_h = $(srcdir)/../common/agent.h
> +linux_btrace_h = $(srcdir)/../common/linux-btrace.h

Looks like linux-btrace.h depends on btrace-common.h/ptid.h/vec.h.
Those (and other I might have missed) should be listed here.

>  linux_osdata_h = $(srcdir)/../common/linux-osdata.h
>  vec_h = $(srcdir)/../common/vec.h
>  gdb_vecs_h = $(srcdir)/../common/gdb_vecs.h
> @@ -532,6 +533,9 @@ format.o: ../common/format.c $(server_h)
>  agent.o: ../common/agent.c $(server_h) $(agent_h)
>  	$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
>  
> +linux-btrace.o: ../common/linux-btrace.c $(linux_btrace_h) $(server_h)
> +	$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
> +
>  # We build vasprintf with -DHAVE_CONFIG_H because we want that unit to
>  # include our config.h file.  Otherwise, some system headers do not get
>  # included, and the compiler emits a warning about implicitly defined
> 


-- 
Pedro Alves