[1/5] btrace: prepare for throwing exceptions when enabling btrace
Commit Message
We indicate success or failure for enabling branch tracing via the pointer
return value. Depending on the type of error, errno may provide additional
information.
Prepare for using exceptions with more descriptive error messages by using smart
pointers and objects with automatic destruction to hold intermediate results.
2018-01-25 Markus Metzger <markus.t.metzger@intel.com>
gdb/
* nat/linux-btrace.c (perf_event_pt_event_type): Use gdb_file_up.
(scoped_close_fd, scoped_mmap): New.
(linux_enable_bts, linux_enable_pt): Use gdb::unique_xmalloc_ptr,
scoped_close_fd, and scoped_mmap.
---
gdb/nat/linux-btrace.c | 183 +++++++++++++++++++++++++++++++------------------
1 file changed, 117 insertions(+), 66 deletions(-)
Comments
Hi Markus,
A few quick comments on the utilities added by the patch.
On 01/25/2018 09:12 AM, Markus Metzger wrote:
> We indicate success or failure for enabling branch tracing via the pointer
> return value. Depending on the type of error, errno may provide additional
> information.
>
> Prepare for using exceptions with more descriptive error messages by using smart
> pointers and objects with automatic destruction to hold intermediate results.
>
> 2018-01-25 Markus Metzger <markus.t.metzger@intel.com>
>
> gdb/
> * nat/linux-btrace.c (perf_event_pt_event_type): Use gdb_file_up.
> (scoped_close_fd, scoped_mmap): New.
> (linux_enable_bts, linux_enable_pt): Use gdb::unique_xmalloc_ptr,
> scoped_close_fd, and scoped_mmap.
> ---
> gdb/nat/linux-btrace.c | 183 +++++++++++++++++++++++++++++++------------------
> 1 file changed, 117 insertions(+), 66 deletions(-)
>
> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
> index bbd0fe6..280d9f1 100644
> --- a/gdb/nat/linux-btrace.c
> +++ b/gdb/nat/linux-btrace.c
> @@ -179,17 +179,12 @@ perf_event_read_all (struct perf_event_buffer *pev, gdb_byte **data,
> static int
> perf_event_pt_event_type (int *type)
> {
> - FILE *file;
> - int found;
> -
> - file = fopen ("/sys/bus/event_source/devices/intel_pt/type", "r");
> - if (file == NULL)
> + gdb_file_up file =
> + gdb_fopen_cloexec ("/sys/bus/event_source/devices/intel_pt/type", "r");
" = " goes on the next line.
> + if (file.get () == nullptr)
> return -1;
The ".get()" shouldn't be necessary here.
>
> - found = fscanf (file, "%d", type);
> -
> - fclose (file);
> -
> + int found = fscanf (file.get (), "%d", type);
> if (found == 1)
> return 0;
> return -1;
> @@ -657,19 +652,95 @@ linux_supports_btrace (struct target_ops *ops, enum btrace_format format)
> internal_error (__FILE__, __LINE__, _("Unknown branch trace format"));
> }
>
> +class scoped_close_fd
Missing intro comment.
> +{
> + int m_fd;
Please put private data fields after the public interface.
> +
> +public:
> + scoped_close_fd (int fd = -1) noexcept : m_fd (fd) {}
Explicit?
> + ~scoped_close_fd () noexcept
Note: destructors are noexcept by default.
> + {
> + if (m_fd >= 0)
> + close (m_fd);
> + }
> +
> + scoped_close_fd (const scoped_close_fd &) = delete;
> + scoped_close_fd &operator = (const scoped_close_fd &) = delete;
Use DISABLE_COPY_AND_ASSIGN.
> +
> + int release () noexcept
> + {
> + int fd = m_fd;
> + m_fd = -1;
> + return fd;
> + }
> +
> + operator int () const noexcept
That's unusual. Do we gain much compared to an explicit
fd() or "get()" getter ?
If this is a smart-pointer-like wrapper for a file descriptor,
then maybe it would be better called scoped_fd instead of
scoped_close_fd, just like the mmap one isn't called "scoped_unmmap"
instead. But no biggie.
> + {
> + return m_fd;
> + }
> +};
> +
> +template <typename object_type>
> +class scoped_mmap
> +{
> + void *m_mem;
> + size_t m_length;
> +
> +public:
> + scoped_mmap () noexcept : m_mem (nullptr), m_length (0) {}
> + scoped_mmap (void *addr, size_t length, int prot, int flags, int fd,
> + off_t offset) noexcept : m_mem (nullptr), m_length (length)
> + {
> + m_mem = mmap (addr, m_length, prot, flags, fd, offset);
If scoped_mmap is templated on object_type, why do we need
the length parameter, in bytes? Or conversely, if the interface
is in terms of bytes, why do we need the template parameter?
(AFAICS, it's for operator->, but, is that really useful
compared to a mismatch in the API? Seems like a layering
violation to me.)
> + }
> + ~scoped_mmap () noexcept
> + {
> + if (m_mem != nullptr)
> + munmap (m_mem, m_length);
> + }
> +
> + scoped_mmap (const scoped_mmap &) = delete;
> + scoped_mmap &operator = (const scoped_mmap &) = delete;
> +
> + object_type *release () noexcept
> + {
> + void *mem = m_mem;
> + m_mem = nullptr;
> + return static_cast<object_type *> (mem);
> + }
> +
> + void reset (void *addr, size_t length, int prot, int flags, int fd,
> + off_t offset) noexcept
> + {
> + if (m_mem != nullptr)
> + munmap (m_mem, m_length);
> +
> + m_length = length;
> + m_mem = mmap (addr, m_length, prot, flags, fd, offset);
> + }
> +
> + size_t size () const noexcept { return m_length; }
> + uint8_t *raw () const noexcept { return static_cast<uint8_t *> (m_mem); }
> +
> + operator void * () const noexcept { return m_mem; }
> + object_type *operator -> () const noexcept
> + {
> + return static_cast<object_type *> (m_mem);
> + }
> +};
Basically the same comments to scoped_close_fd apply here too.
You should add describing comments to the methods too.
These seem like general enough utilities that might be
better for the long run to put them straight in
gdb/common/. (Unit tests would be nice.)
BTW, I find the indentation of the function-open "{"s a
little unusual.
Thanks,
Pedro Alves
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: 25 January 2018 15:47
> To: Metzger, Markus T <markus.t.metzger@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 1/5] btrace: prepare for throwing exceptions when enabling
> btrace
Hello Pedro,
Thanks for your comments.
> > + scoped_mmap () noexcept : m_mem (nullptr), m_length (0) {}
> > + scoped_mmap (void *addr, size_t length, int prot, int flags, int fd,
> > + off_t offset) noexcept : m_mem (nullptr), m_length (length)
> > + {
> > + m_mem = mmap (addr, m_length, prot, flags, fd, offset);
>
> If scoped_mmap is templated on object_type, why do we need the length
> parameter, in bytes? Or conversely, if the interface is in terms of bytes, why do we
> need the template parameter?
> (AFAICS, it's for operator->, but, is that really useful compared to a mismatch in the
> API? Seems like a layering violation to me.)
We need to store the length for the munmap().
The isn't really a template in the sense that it maps/unmaps an object. It maps
some kernel-managed buffer and there happens to be a user-page at the front
that is described by this struct. I made it a template to save casts when accessing
the user-page in the buffer.
I see how this can be confusing, though, so I'll change it to have this class just take
care about mapping and unmapping memory.
> You should add describing comments to the methods too.
Hmmm, given that they are both inline and trivial I wouldn't really know what to
write that isn't completely obvious.
> These seem like general enough utilities that might be better for the long run to put
> them straight in gdb/common/. (Unit tests would be nice.)
I moved them into gdb/common/ but have not tried to make them any more generic.
I ran unit tests using "maint selftest". Are they included in "make check", as well?
Thanks,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
@@ -179,17 +179,12 @@ perf_event_read_all (struct perf_event_buffer *pev, gdb_byte **data,
static int
perf_event_pt_event_type (int *type)
{
- FILE *file;
- int found;
-
- file = fopen ("/sys/bus/event_source/devices/intel_pt/type", "r");
- if (file == NULL)
+ gdb_file_up file =
+ gdb_fopen_cloexec ("/sys/bus/event_source/devices/intel_pt/type", "r");
+ if (file.get () == nullptr)
return -1;
- found = fscanf (file, "%d", type);
-
- fclose (file);
-
+ int found = fscanf (file.get (), "%d", type);
if (found == 1)
return 0;
return -1;
@@ -657,19 +652,95 @@ linux_supports_btrace (struct target_ops *ops, enum btrace_format format)
internal_error (__FILE__, __LINE__, _("Unknown branch trace format"));
}
+class scoped_close_fd
+{
+ int m_fd;
+
+public:
+ scoped_close_fd (int fd = -1) noexcept : m_fd (fd) {}
+ ~scoped_close_fd () noexcept
+ {
+ if (m_fd >= 0)
+ close (m_fd);
+ }
+
+ scoped_close_fd (const scoped_close_fd &) = delete;
+ scoped_close_fd &operator = (const scoped_close_fd &) = delete;
+
+ int release () noexcept
+ {
+ int fd = m_fd;
+ m_fd = -1;
+ return fd;
+ }
+
+ operator int () const noexcept
+ {
+ return m_fd;
+ }
+};
+
+template <typename object_type>
+class scoped_mmap
+{
+ void *m_mem;
+ size_t m_length;
+
+public:
+ scoped_mmap () noexcept : m_mem (nullptr), m_length (0) {}
+ scoped_mmap (void *addr, size_t length, int prot, int flags, int fd,
+ off_t offset) noexcept : m_mem (nullptr), m_length (length)
+ {
+ m_mem = mmap (addr, m_length, prot, flags, fd, offset);
+ }
+ ~scoped_mmap () noexcept
+ {
+ if (m_mem != nullptr)
+ munmap (m_mem, m_length);
+ }
+
+ scoped_mmap (const scoped_mmap &) = delete;
+ scoped_mmap &operator = (const scoped_mmap &) = delete;
+
+ object_type *release () noexcept
+ {
+ void *mem = m_mem;
+ m_mem = nullptr;
+ return static_cast<object_type *> (mem);
+ }
+
+ void reset (void *addr, size_t length, int prot, int flags, int fd,
+ off_t offset) noexcept
+ {
+ if (m_mem != nullptr)
+ munmap (m_mem, m_length);
+
+ m_length = length;
+ m_mem = mmap (addr, m_length, prot, flags, fd, offset);
+ }
+
+ size_t size () const noexcept { return m_length; }
+ uint8_t *raw () const noexcept { return static_cast<uint8_t *> (m_mem); }
+
+ operator void * () const noexcept { return m_mem; }
+ object_type *operator -> () const noexcept
+ {
+ return static_cast<object_type *> (m_mem);
+ }
+};
+
/* Enable branch tracing in BTS format. */
static struct btrace_target_info *
linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
{
- struct perf_event_mmap_page *header;
- struct btrace_target_info *tinfo;
struct btrace_tinfo_bts *bts;
size_t size, pages;
__u64 data_offset;
int pid, pg;
- tinfo = XCNEW (struct btrace_target_info);
+ gdb::unique_xmalloc_ptr<btrace_target_info> tinfo
+ (XCNEW (btrace_target_info));
tinfo->ptid = ptid;
tinfo->conf.format = BTRACE_FORMAT_BTS;
@@ -692,9 +763,10 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
pid = ptid_get_pid (ptid);
errno = 0;
- bts->file = syscall (SYS_perf_event_open, &bts->attr, pid, -1, -1, 0);
- if (bts->file < 0)
- goto err_out;
+ scoped_close_fd fd (syscall (SYS_perf_event_open, &bts->attr, pid,
+ -1, -1, 0));
+ if (fd < 0)
+ return nullptr;
/* Convert the requested size in bytes to pages (rounding up). */
pages = ((size_t) conf->size / PAGE_SIZE
@@ -711,6 +783,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
/* We try to allocate the requested size.
If that fails, try to get as much as we can. */
+ scoped_mmap<perf_event_mmap_page> header;
for (; pages > 0; pages >>= 1)
{
size_t length;
@@ -730,14 +803,13 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
continue;
/* The number of pages we request needs to be a power of two. */
- header = ((struct perf_event_mmap_page *)
- mmap (NULL, length, PROT_READ, MAP_SHARED, bts->file, 0));
+ header.reset (NULL, length, PROT_READ, MAP_SHARED, fd, 0);
if (header != MAP_FAILED)
break;
}
if (pages == 0)
- goto err_file;
+ return nullptr;
data_offset = PAGE_SIZE;
@@ -753,29 +825,19 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
/* Check for overflows. */
if ((__u64) size != data_size)
- {
- munmap ((void *) header, size + PAGE_SIZE);
- goto err_file;
- }
+ return nullptr;
}
#endif /* defined (PERF_ATTR_SIZE_VER5) */
- bts->header = header;
- bts->bts.mem = ((const uint8_t *) header) + data_offset;
bts->bts.size = size;
bts->bts.data_head = &header->data_head;
+ bts->bts.mem = header.raw () + data_offset;
bts->bts.last_head = 0ull;
+ bts->header = header.release ();
+ bts->file = fd.release ();
tinfo->conf.bts.size = (unsigned int) size;
- return tinfo;
-
- err_file:
- /* We were not able to allocate any buffer. */
- close (bts->file);
-
- err_out:
- xfree (tinfo);
- return NULL;
+ return tinfo.release ();
}
#if defined (PERF_ATTR_SIZE_VER5)
@@ -785,10 +847,8 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
static struct btrace_target_info *
linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
{
- struct perf_event_mmap_page *header;
- struct btrace_target_info *tinfo;
struct btrace_tinfo_pt *pt;
- size_t pages, size;
+ size_t pages;
int pid, pg, errcode, type;
if (conf->size == 0)
@@ -802,7 +862,8 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
if (pid == 0)
pid = ptid_get_pid (ptid);
- tinfo = XCNEW (struct btrace_target_info);
+ gdb::unique_xmalloc_ptr<btrace_target_info> tinfo
+ (XCNEW (btrace_target_info));
tinfo->ptid = ptid;
tinfo->conf.format = BTRACE_FORMAT_PT;
@@ -816,16 +877,16 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
pt->attr.exclude_idle = 1;
errno = 0;
- pt->file = syscall (SYS_perf_event_open, &pt->attr, pid, -1, -1, 0);
- if (pt->file < 0)
- goto err;
+ scoped_close_fd fd (syscall (SYS_perf_event_open, &pt->attr, pid, -1, -1, 0));
+ if (fd < 0)
+ return nullptr;
/* Allocate the configuration page. */
- header = ((struct perf_event_mmap_page *)
- mmap (NULL, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
- pt->file, 0));
+ scoped_mmap<perf_event_mmap_page> header (NULL, PAGE_SIZE,
+ PROT_READ | PROT_WRITE, MAP_SHARED,
+ fd, 0);
if (header == MAP_FAILED)
- goto err_file;
+ return nullptr;
header->aux_offset = header->data_offset + header->data_size;
@@ -844,6 +905,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
/* We try to allocate the requested size.
If that fails, try to get as much as we can. */
+ scoped_mmap<uint8_t> aux;
for (; pages > 0; pages >>= 1)
{
size_t length;
@@ -855,41 +917,30 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
if ((__u64) UINT_MAX < data_size)
continue;
- size = (size_t) data_size;
+ length = (size_t) data_size;
/* Check for overflows. */
- if ((__u64) size != data_size)
+ if ((__u64) length != data_size)
continue;
header->aux_size = data_size;
- length = size;
- pt->pt.mem = ((const uint8_t *)
- mmap (NULL, length, PROT_READ, MAP_SHARED, pt->file,
- header->aux_offset));
- if (pt->pt.mem != MAP_FAILED)
+ aux.reset (NULL, length, PROT_READ, MAP_SHARED, fd, header->aux_offset);
+ if (aux != MAP_FAILED)
break;
}
if (pages == 0)
- goto err_conf;
+ return nullptr;
- pt->header = header;
- pt->pt.size = size;
+ pt->pt.size = aux.size ();
+ pt->pt.mem = aux.release ();
pt->pt.data_head = &header->aux_head;
+ pt->header = header.release ();
+ pt->file = fd.release ();
- tinfo->conf.pt.size = (unsigned int) size;
- return tinfo;
-
- err_conf:
- munmap((void *) header, PAGE_SIZE);
-
- err_file:
- close (pt->file);
-
- err:
- xfree (tinfo);
- return NULL;
+ tinfo->conf.pt.size = (unsigned int) pt->pt.size;
+ return tinfo.release ();
}
#else /* !defined (PERF_ATTR_SIZE_VER5) */