From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29943 invoked by alias); 28 Nov 2012 17:31:55 -0000 Received: (qmail 29931 invoked by uid 22791); 28 Nov 2012 17:31:53 -0000 X-SWARE-Spam-Status: No, hits=-7.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_CP,TW_XZ X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 28 Nov 2012 17:31:43 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qASHVebS023410 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 28 Nov 2012 12:31:40 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qASHVbJu013929; Wed, 28 Nov 2012 12:31:38 -0500 Message-ID: <50B64A79.6050608@redhat.com> Date: Wed, 28 Nov 2012 17:31:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: markus.t.metzger@intel.com CC: gdb-patches@sourceware.org, markus.t.metzger@gmail.com, jan.kratochvil@redhat.com, tromey@redhat.com, kettenis@gnu.org Subject: Re: [patch v4 05/13] linux, btrace: perf_event based branch tracing References: <1354013351-14791-1-git-send-email-markus.t.metzger@intel.com> <1354013351-14791-6-git-send-email-markus.t.metzger@intel.com> In-Reply-To: <1354013351-14791-6-git-send-email-markus.t.metzger@intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-11/txt/msg00817.txt.bz2 On 11/27/2012 10:49 AM, markus.t.metzger@intel.com wrote: > From: Markus Metzger > > 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 > > * 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. > + > + 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 . */ > + > +/* 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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. > + > + 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 . */ > + > +#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 > +#include > + > +#if HAVE_LINUX_PERF_EVENT_H > +# include > +#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