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

Pedro Alves
2012-11-28T17:31:55Z

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