* [PATCH 2/5] btrace, gdbserver: use exceptions to convey btrace enable/disable errors
2018-01-25 9:12 [PATCH 0/5] improve btrace enable error reporting Markus Metzger
@ 2018-01-25 9:12 ` Markus Metzger
2018-01-25 9:12 ` [PATCH 5/5] btrace: check perf_event_paranoid Markus Metzger
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Markus Metzger @ 2018-01-25 9:12 UTC (permalink / raw)
To: gdb-patches
Change error reporting to use exceptions and be prepared to catch them in
gdbserver. We use the exception message in our error reply to GDB.
This may remove some detail from the error message in the native case since
errno is no longer printed. Later patches will improve that.
2018-01-25 Markus Metzger <markus.t.metzger@intel.com>
gdbserver/
* server.c (handle_btrace_enable_bts, handle_btrace_enable_pt)
(handle_btrace_disable): Change return type to void. Use exceptions
to report errors.
(handle_btrace_general_set): Catch exception and copy message to
return message.
gdb/
* nat/linux-btrace.c (linux_enable_btrace): Throw exception if enabling
btrace failed.
* x86-linux-nat.c (x86_linux_enable_btrace): Catch btrace enabling
exception and use message in own exception.
---
gdb/gdbserver/server.c | 55 ++++++++++++++++++++++----------------------------
gdb/nat/linux-btrace.c | 3 +++
gdb/x86-linux-nat.c | 19 +++++++++--------
3 files changed, 38 insertions(+), 39 deletions(-)
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 9d12ce6..5ce6281 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -380,50 +380,41 @@ write_qxfer_response (char *buf, const gdb_byte *data, int len, int is_more)
/* Handle btrace enabling in BTS format. */
-static const char *
+static void
handle_btrace_enable_bts (struct thread_info *thread)
{
if (thread->btrace != NULL)
- return "E.Btrace already enabled.";
+ error (_("Btrace already enabled."));
current_btrace_conf.format = BTRACE_FORMAT_BTS;
thread->btrace = target_enable_btrace (thread->id, ¤t_btrace_conf);
- if (thread->btrace == NULL)
- return "E.Could not enable btrace.";
-
- return NULL;
}
/* Handle btrace enabling in Intel Processor Trace format. */
-static const char *
+static void
handle_btrace_enable_pt (struct thread_info *thread)
{
if (thread->btrace != NULL)
- return "E.Btrace already enabled.";
+ error (_("Btrace already enabled."));
current_btrace_conf.format = BTRACE_FORMAT_PT;
thread->btrace = target_enable_btrace (thread->id, ¤t_btrace_conf);
- if (thread->btrace == NULL)
- return "E.Could not enable btrace.";
-
- return NULL;
}
/* Handle btrace disabling. */
-static const char *
+static void
handle_btrace_disable (struct thread_info *thread)
{
if (thread->btrace == NULL)
- return "E.Branch tracing not enabled.";
+ error (_("Branch tracing not enabled."));
if (target_disable_btrace (thread->btrace) != 0)
- return "E.Could not disable branch tracing.";
+ error (_("Could not disable branch tracing."));
thread->btrace = NULL;
- return NULL;
}
/* Handle the "Qbtrace" packet. */
@@ -432,7 +423,6 @@ static int
handle_btrace_general_set (char *own_buf)
{
struct thread_info *thread;
- const char *err;
char *op;
if (!startswith (own_buf, "Qbtrace:"))
@@ -454,21 +444,24 @@ handle_btrace_general_set (char *own_buf)
return -1;
}
- err = NULL;
-
- if (strcmp (op, "bts") == 0)
- err = handle_btrace_enable_bts (thread);
- else if (strcmp (op, "pt") == 0)
- err = handle_btrace_enable_pt (thread);
- else if (strcmp (op, "off") == 0)
- err = handle_btrace_disable (thread);
- else
- err = "E.Bad Qbtrace operation. Use bts, pt, or off.";
+ TRY
+ {
+ if (strcmp (op, "bts") == 0)
+ handle_btrace_enable_bts (thread);
+ else if (strcmp (op, "pt") == 0)
+ handle_btrace_enable_pt (thread);
+ else if (strcmp (op, "off") == 0)
+ handle_btrace_disable (thread);
+ else
+ error (_("Bad Qbtrace operation. Use bts, pt, or off."));
- if (err != 0)
- strcpy (own_buf, err);
- else
- write_ok (own_buf);
+ write_ok (own_buf);
+ }
+ CATCH (exception, RETURN_MASK_ERROR)
+ {
+ sprintf (own_buf, "E.%s", exception.message);
+ }
+ END_CATCH
return 1;
}
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index 280d9f1..8ebfe9c 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -976,6 +976,9 @@ linux_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
break;
}
+ if (tinfo == NULL)
+ error (_("Unknown error."));
+
return tinfo;
}
diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
index 46bb6a4..75f68de 100644
--- a/gdb/x86-linux-nat.c
+++ b/gdb/x86-linux-nat.c
@@ -216,14 +216,17 @@ static struct btrace_target_info *
x86_linux_enable_btrace (struct target_ops *self, ptid_t ptid,
const struct btrace_config *conf)
{
- struct btrace_target_info *tinfo;
-
- errno = 0;
- tinfo = linux_enable_btrace (ptid, conf);
-
- if (tinfo == NULL)
- error (_("Could not enable branch tracing for %s: %s."),
- target_pid_to_str (ptid), safe_strerror (errno));
+ struct btrace_target_info *tinfo = nullptr;
+ TRY
+ {
+ tinfo = linux_enable_btrace (ptid, conf);
+ }
+ CATCH (exception, RETURN_MASK_ERROR)
+ {
+ error (_("Could not enable branch tracing for %s: %s"),
+ target_pid_to_str (ptid), exception.message);
+ }
+ END_CATCH
return tinfo;
}
--
1.8.3.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 5/5] btrace: check perf_event_paranoid
2018-01-25 9:12 [PATCH 0/5] improve btrace enable error reporting Markus Metzger
2018-01-25 9:12 ` [PATCH 2/5] btrace, gdbserver: use exceptions to convey btrace enable/disable errors Markus Metzger
@ 2018-01-25 9:12 ` Markus Metzger
2018-01-25 9:12 ` [PATCH 4/5] btrace: improve enable error messages Markus Metzger
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Markus Metzger @ 2018-01-25 9:12 UTC (permalink / raw)
To: gdb-patches
One recurring error on Debian systems is that the default perf_event_paranoid
setting disables the perf_event interface for user-space.
Check the current level and point the user to the file.
2018-01-25 Markus Metzger <markus.t.metzger@intel.com>
gdb/
* nat/linux-btrace.c (diagnose_perf_event_open_fail): New.
(linux_enable_pt, linux_enable_bts): Call
diagnose_perf_event_open_fail.
---
gdb/nat/linux-btrace.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index db93739..956b945 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -484,6 +484,34 @@ public:
}
};
+/* The perf_event_open syscall failed. Try to print a helpful error
+ message. */
+
+static void
+diagnose_perf_event_open_fail (void)
+{
+ switch (errno)
+ {
+ case EPERM:
+ case EACCES:
+ {
+ const char *filename = "/proc/sys/kernel/perf_event_paranoid";
+ gdb_file_up file = gdb_fopen_cloexec (filename, "r");
+ if (file.get () == nullptr)
+ break;
+
+ int level, found = fscanf (file.get (), "%d", &level);
+ if (found == 1 && level > 2)
+ error (_("You do not have permission to record the process. "
+ "Try setting %s to 2 or less."), filename);
+ }
+
+ break;
+ }
+
+ error (_("Failed to start recording: %s"), safe_strerror (errno));
+}
+
/* Enable branch tracing in BTS format. */
static struct btrace_target_info *
@@ -524,7 +552,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
scoped_close_fd fd (syscall (SYS_perf_event_open, &bts->attr, pid,
-1, -1, 0));
if (fd < 0)
- error (_("Failed to start recording: %s"), safe_strerror (errno));
+ diagnose_perf_event_open_fail ();
/* Convert the requested size in bytes to pages (rounding up). */
pages = ((size_t) conf->size / PAGE_SIZE
@@ -650,7 +678,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
errno = 0;
scoped_close_fd fd (syscall (SYS_perf_event_open, &pt->attr, pid, -1, -1, 0));
if (fd < 0)
- error (_("Failed to start recording: %s"), safe_strerror (errno));
+ diagnose_perf_event_open_fail ();
/* Allocate the configuration page. */
scoped_mmap<perf_event_mmap_page> header (NULL, PAGE_SIZE,
--
1.8.3.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 4/5] btrace: improve enable error messages
2018-01-25 9:12 [PATCH 0/5] improve btrace enable error reporting Markus Metzger
2018-01-25 9:12 ` [PATCH 2/5] btrace, gdbserver: use exceptions to convey btrace enable/disable errors Markus Metzger
2018-01-25 9:12 ` [PATCH 5/5] btrace: check perf_event_paranoid Markus Metzger
@ 2018-01-25 9:12 ` Markus Metzger
2018-01-25 9:12 ` [PATCH 3/5] btrace, gdbserver: remove the to_supports_btrace target method Markus Metzger
2018-01-25 9:12 ` [PATCH 1/5] btrace: prepare for throwing exceptions when enabling btrace Markus Metzger
4 siblings, 0 replies; 8+ messages in thread
From: Markus Metzger @ 2018-01-25 9:12 UTC (permalink / raw)
To: gdb-patches
Improve the error message when GDB fails to start recording branch trace.
This patch also removes a zero buffer size check for PT to align with BTS. The
buffer size can not be configured to be zero.
2018-01-25 Markus Metzger <markus.t.metzger@intel.com>
gdb/
* nat/linux-btrace.c (perf_event_pt_event_type): Improve error message.
Return type. Update callers.
(linux_enable_bts, linux_enable_pt): Improve error message.
(linux_enable_pt): Remove zero buffer size check.
(linux_enable_btrace): Improve error messages. Remove NULL return
check.
---
gdb/nat/linux-btrace.c | 69 +++++++++++++++++++++-----------------------------
1 file changed, 29 insertions(+), 40 deletions(-)
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index 08a9716..db93739 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -524,7 +524,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
scoped_close_fd fd (syscall (SYS_perf_event_open, &bts->attr, pid,
-1, -1, 0));
if (fd < 0)
- return nullptr;
+ error (_("Failed to start recording: %s"), safe_strerror (errno));
/* Convert the requested size in bytes to pages (rounding up). */
pages = ((size_t) conf->size / PAGE_SIZE
@@ -560,6 +560,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
if ((__u64) length != data_size + PAGE_SIZE)
continue;
+ errno = 0;
/* The number of pages we request needs to be a power of two. */
header.reset (NULL, length, PROT_READ, MAP_SHARED, fd, 0);
if (header != MAP_FAILED)
@@ -567,7 +568,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
}
if (pages == 0)
- return nullptr;
+ error (_("Failed to map trace buffer: %s."), safe_strerror (errno));
data_offset = PAGE_SIZE;
@@ -583,7 +584,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
/* Check for overflows. */
if ((__u64) size != data_size)
- return nullptr;
+ error (_("Failed to determine trace buffer size."));
}
#endif /* defined (PERF_ATTR_SIZE_VER5) */
@@ -600,21 +601,23 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
#if defined (PERF_ATTR_SIZE_VER5)
-/* Determine the event type.
- Returns zero on success and fills in TYPE; returns -1 otherwise. */
+/* Determine the event type. */
static int
-perf_event_pt_event_type (int *type)
+perf_event_pt_event_type (void)
{
- gdb_file_up file =
- gdb_fopen_cloexec ("/sys/bus/event_source/devices/intel_pt/type", "r");
+ const char *filename = "/sys/bus/event_source/devices/intel_pt/type";
+
+ errno = 0;
+ gdb_file_up file = gdb_fopen_cloexec (filename, "r");
if (file.get () == nullptr)
- return -1;
+ error (_("Failed to open %s: %s."), filename, safe_strerror (errno));
- int found = fscanf (file.get (), "%d", type);
- if (found == 1)
- return 0;
- return -1;
+ int type, found = fscanf (file.get (), "%d", &type);
+ if (found != 1)
+ error (_("Failed to read the PT event type from %s."), filename);
+
+ return type;
}
/* Enable branch tracing in Intel Processor Trace format. */
@@ -624,14 +627,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
{
struct btrace_tinfo_pt *pt;
size_t pages;
- int pid, pg, errcode, type;
-
- if (conf->size == 0)
- return NULL;
-
- errcode = perf_event_pt_event_type (&type);
- if (errcode != 0)
- return NULL;
+ int pid, pg;
pid = ptid_get_lwp (ptid);
if (pid == 0)
@@ -645,7 +641,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
pt = &tinfo->variant.pt;
pt->attr.size = sizeof (pt->attr);
- pt->attr.type = type;
+ pt->attr.type = perf_event_pt_event_type ();
pt->attr.exclude_kernel = 1;
pt->attr.exclude_hv = 1;
@@ -654,14 +650,14 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
errno = 0;
scoped_close_fd fd (syscall (SYS_perf_event_open, &pt->attr, pid, -1, -1, 0));
if (fd < 0)
- return nullptr;
+ error (_("Failed to start recording: %s"), safe_strerror (errno));
/* Allocate the configuration page. */
scoped_mmap<perf_event_mmap_page> header (NULL, PAGE_SIZE,
PROT_READ | PROT_WRITE, MAP_SHARED,
fd, 0);
if (header == MAP_FAILED)
- return nullptr;
+ error (_("Failed to map trace user page: %s."), safe_strerror (errno));
header->aux_offset = header->data_offset + header->data_size;
@@ -700,13 +696,14 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
header->aux_size = data_size;
+ errno = 0;
aux.reset (NULL, length, PROT_READ, MAP_SHARED, fd, header->aux_offset);
if (aux != MAP_FAILED)
break;
}
if (pages == 0)
- return nullptr;
+ error (_("Failed to map trace buffer: %s."), safe_strerror (errno));
pt->pt.size = aux.size ();
pt->pt.mem = aux.release ();
@@ -723,8 +720,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
static struct btrace_target_info *
linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
{
- errno = EOPNOTSUPP;
- return NULL;
+ error (_("GDB does not support Intel PT."));
}
#endif /* !defined (PERF_ATTR_SIZE_VER5) */
@@ -734,27 +730,20 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
struct btrace_target_info *
linux_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
{
- struct btrace_target_info *tinfo;
-
- tinfo = NULL;
switch (conf->format)
{
case BTRACE_FORMAT_NONE:
- break;
+ error (_("Bad branch trace format."));
+
+ default:
+ error (_("Unknown branch trace format."));
case BTRACE_FORMAT_BTS:
- tinfo = linux_enable_bts (ptid, &conf->bts);
- break;
+ return linux_enable_bts (ptid, &conf->bts);
case BTRACE_FORMAT_PT:
- tinfo = linux_enable_pt (ptid, &conf->pt);
- break;
+ return linux_enable_pt (ptid, &conf->pt);
}
-
- if (tinfo == NULL)
- error (_("Unknown error."));
-
- return tinfo;
}
/* Disable BTS tracing. */
--
1.8.3.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 3/5] btrace, gdbserver: remove the to_supports_btrace target method
2018-01-25 9:12 [PATCH 0/5] improve btrace enable error reporting Markus Metzger
` (2 preceding siblings ...)
2018-01-25 9:12 ` [PATCH 4/5] btrace: improve enable error messages Markus Metzger
@ 2018-01-25 9:12 ` Markus Metzger
2018-01-25 9:12 ` [PATCH 1/5] btrace: prepare for throwing exceptions when enabling btrace Markus Metzger
4 siblings, 0 replies; 8+ messages in thread
From: Markus Metzger @ 2018-01-25 9:12 UTC (permalink / raw)
To: gdb-patches
Remove the to_supports_btrace target method and instead rely on detecting errors
when trying to enable recording. This will also provide a suitable error
message explaining why recording is not possible.
2018-01-25 Markus Metzger <markus.t.metzger@intel.com>
gdb/
* btrace.c (btrace_enable): Remove target_supports_btrace call.
* nat/linux-btrace.c (perf_event_pt_event_type): Move.
(kernel_supports_bts, kernel_supports_pt, linux_supports_bts)
(linux_supports_pt, linux_supports_btrace): Remove.
(linux_enable_bts): Call cpu_supports_bts.
* nat/linux-btrace.h (linux_supports_btrace): Remove.
* remote.c (remote_supports_btrace): Remove.
(init_remote_ops): Remove remote_supports_btrace.
* target-delegates.c: Regenerated.
* target.c (target_supports_btrace): Remove.
* target.h (target_ops) <to_supports_btrace>: Remove
(target_supports_btrace): Remove.
* x86-linux-nat.c (x86_linux_create_target): Remove
linux_supports_btrace.
gdbserver/
* linux-low.c (linux_target_ops): Remove linux_supports_btrace.
* nto-low.c (nto_target_ops): Remove NULL for supports_btrace.
* spu-low.c (spu_target_ops): Likewise.
* win32-low.c (win32_target_ops): Likewise.
* server.c (supported_btrace_packets): Report packets unconditionally.
* target.h (target_ops) <supports_btrace>: Remove.
(target_supports_btrace): Remove.
---
gdb/btrace.c | 3 -
gdb/gdbserver/linux-low.c | 2 -
gdb/gdbserver/nto-low.c | 1 -
gdb/gdbserver/server.c | 25 +----
gdb/gdbserver/spu-low.c | 1 -
gdb/gdbserver/target.h | 7 --
gdb/gdbserver/win32-low.c | 1 -
gdb/nat/linux-btrace.c | 273 ++++------------------------------------------
gdb/nat/linux-btrace.h | 3 -
gdb/remote.c | 32 ------
gdb/target-delegates.c | 33 ------
gdb/target.c | 8 --
gdb/target.h | 7 --
gdb/x86-linux-nat.c | 1 -
14 files changed, 24 insertions(+), 373 deletions(-)
diff --git a/gdb/btrace.c b/gdb/btrace.c
index ffcf53a..2b031a4 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1582,9 +1582,6 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
error (_("GDB does not support Intel Processor Trace."));
#endif /* !defined (HAVE_LIBIPT) */
- if (!target_supports_btrace (conf->format))
- error (_("Target does not support branch tracing."));
-
DEBUG ("enable thread %s (%s)", print_thread_id (tp),
target_pid_to_str (tp->ptid));
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 38142bb..c1ba961 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7531,7 +7531,6 @@ static struct target_ops linux_target_ops = {
linux_qxfer_libraries_svr4,
linux_supports_agent,
#ifdef HAVE_LINUX_BTRACE
- linux_supports_btrace,
linux_enable_btrace,
linux_low_disable_btrace,
linux_low_read_btrace,
@@ -7541,7 +7540,6 @@ static struct target_ops linux_target_ops = {
NULL,
NULL,
NULL,
- NULL,
#endif
linux_supports_range_stepping,
linux_proc_pid_to_exec_file,
diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index a570ca9..b68b811 100644
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -991,7 +991,6 @@ static struct target_ops nto_target_ops = {
NULL, /* get_min_fast_tracepoint_insn_len */
NULL, /* qxfer_libraries_svr4 */
NULL, /* support_agent */
- NULL, /* support_btrace */
NULL, /* enable_btrace */
NULL, /* disable_btrace */
NULL, /* read_btrace */
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 5ce6281..cb02b58 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2104,27 +2104,10 @@ crc32 (CORE_ADDR base, int len, unsigned int crc)
static void
supported_btrace_packets (char *buf)
{
- int btrace_supported = 0;
-
- if (target_supports_btrace (BTRACE_FORMAT_BTS))
- {
- strcat (buf, ";Qbtrace:bts+");
- strcat (buf, ";Qbtrace-conf:bts:size+");
-
- btrace_supported = 1;
- }
-
- if (target_supports_btrace (BTRACE_FORMAT_PT))
- {
- strcat (buf, ";Qbtrace:pt+");
- strcat (buf, ";Qbtrace-conf:pt:size+");
-
- btrace_supported = 1;
- }
-
- if (!btrace_supported)
- return;
-
+ strcat (buf, ";Qbtrace:bts+");
+ strcat (buf, ";Qbtrace-conf:bts:size+");
+ strcat (buf, ";Qbtrace:pt+");
+ strcat (buf, ";Qbtrace-conf:pt:size+");
strcat (buf, ";Qbtrace:off+");
strcat (buf, ";qXfer:btrace:read+");
strcat (buf, ";qXfer:btrace-conf:read+");
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index fd1f8d6..5b880d2 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -717,7 +717,6 @@ static struct target_ops spu_target_ops = {
NULL, /* get_min_fast_tracepoint_insn_len */
NULL, /* qxfer_libraries_svr4 */
NULL, /* support_agent */
- NULL, /* support_btrace */
NULL, /* enable_btrace */
NULL, /* disable_btrace */
NULL, /* read_btrace */
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 295e64d..dcefe1a 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -391,9 +391,6 @@ struct target_ops
/* Return true if target supports debugging agent. */
int (*supports_agent) (void);
- /* Check whether the target supports branch tracing. */
- int (*supports_btrace) (struct target_ops *, enum btrace_format);
-
/* Enable branch tracing for PTID based on CONF and allocate a branch trace
target information struct for reading and for disabling branch trace. */
struct btrace_target_info *(*enable_btrace)
@@ -623,10 +620,6 @@ int kill_inferior (int);
(the_target->supports_agent ? \
(*the_target->supports_agent) () : 0)
-#define target_supports_btrace(format) \
- (the_target->supports_btrace \
- ? (*the_target->supports_btrace) (the_target, format) : 0)
-
#define target_enable_btrace(ptid, conf) \
(*the_target->enable_btrace) (ptid, conf)
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index b1d9b51..9f0c4e4 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -1842,7 +1842,6 @@ static struct target_ops win32_target_ops = {
NULL, /* get_min_fast_tracepoint_insn_len */
NULL, /* qxfer_libraries_svr4 */
NULL, /* support_agent */
- NULL, /* support_btrace */
NULL, /* enable_btrace */
NULL, /* disable_btrace */
NULL, /* read_btrace */
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index 8ebfe9c..08a9716 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -173,23 +173,6 @@ perf_event_read_all (struct perf_event_buffer *pev, gdb_byte **data,
pev->last_head = data_head;
}
-/* Determine the event type.
- Returns zero on success and fills in TYPE; returns -1 otherwise. */
-
-static int
-perf_event_pt_event_type (int *type)
-{
- gdb_file_up file =
- gdb_fopen_cloexec ("/sys/bus/event_source/devices/intel_pt/type", "r");
- if (file.get () == nullptr)
- return -1;
-
- int found = fscanf (file.get (), "%d", type);
- if (found == 1)
- return 0;
- return -1;
-}
-
/* Try to determine the start address of the Linux kernel. */
static uint64_t
@@ -374,176 +357,6 @@ perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
return btrace;
}
-/* Check whether the kernel supports BTS. */
-
-static int
-kernel_supports_bts (void)
-{
- struct perf_event_attr attr;
- pid_t child, pid;
- int status, file;
-
- errno = 0;
- child = fork ();
- switch (child)
- {
- case -1:
- warning (_("test bts: cannot fork: %s."), safe_strerror (errno));
- return 0;
-
- case 0:
- status = ptrace (PTRACE_TRACEME, 0, NULL, NULL);
- if (status != 0)
- {
- warning (_("test bts: cannot PTRACE_TRACEME: %s."),
- safe_strerror (errno));
- _exit (1);
- }
-
- status = raise (SIGTRAP);
- if (status != 0)
- {
- warning (_("test bts: cannot raise SIGTRAP: %s."),
- safe_strerror (errno));
- _exit (1);
- }
-
- _exit (1);
-
- default:
- pid = waitpid (child, &status, 0);
- if (pid != child)
- {
- warning (_("test bts: bad pid %ld, error: %s."),
- (long) pid, safe_strerror (errno));
- return 0;
- }
-
- if (!WIFSTOPPED (status))
- {
- warning (_("test bts: expected stop. status: %d."),
- status);
- return 0;
- }
-
- memset (&attr, 0, sizeof (attr));
-
- attr.type = PERF_TYPE_HARDWARE;
- attr.config = PERF_COUNT_HW_BRANCH_INSTRUCTIONS;
- attr.sample_period = 1;
- attr.sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_ADDR;
- attr.exclude_kernel = 1;
- attr.exclude_hv = 1;
- attr.exclude_idle = 1;
-
- file = syscall (SYS_perf_event_open, &attr, child, -1, -1, 0);
- if (file >= 0)
- close (file);
-
- kill (child, SIGKILL);
- ptrace (PTRACE_KILL, child, NULL, NULL);
-
- pid = waitpid (child, &status, 0);
- if (pid != child)
- {
- warning (_("test bts: bad pid %ld, error: %s."),
- (long) pid, safe_strerror (errno));
- if (!WIFSIGNALED (status))
- warning (_("test bts: expected killed. status: %d."),
- status);
- }
-
- return (file >= 0);
- }
-}
-
-/* Check whether the kernel supports Intel Processor Trace. */
-
-static int
-kernel_supports_pt (void)
-{
- struct perf_event_attr attr;
- pid_t child, pid;
- int status, file, type;
-
- errno = 0;
- child = fork ();
- switch (child)
- {
- case -1:
- warning (_("test pt: cannot fork: %s."), safe_strerror (errno));
- return 0;
-
- case 0:
- status = ptrace (PTRACE_TRACEME, 0, NULL, NULL);
- if (status != 0)
- {
- warning (_("test pt: cannot PTRACE_TRACEME: %s."),
- safe_strerror (errno));
- _exit (1);
- }
-
- status = raise (SIGTRAP);
- if (status != 0)
- {
- warning (_("test pt: cannot raise SIGTRAP: %s."),
- safe_strerror (errno));
- _exit (1);
- }
-
- _exit (1);
-
- default:
- pid = waitpid (child, &status, 0);
- if (pid != child)
- {
- warning (_("test pt: bad pid %ld, error: %s."),
- (long) pid, safe_strerror (errno));
- return 0;
- }
-
- if (!WIFSTOPPED (status))
- {
- warning (_("test pt: expected stop. status: %d."),
- status);
- return 0;
- }
-
- status = perf_event_pt_event_type (&type);
- if (status != 0)
- file = -1;
- else
- {
- memset (&attr, 0, sizeof (attr));
-
- attr.size = sizeof (attr);
- attr.type = type;
- attr.exclude_kernel = 1;
- attr.exclude_hv = 1;
- attr.exclude_idle = 1;
-
- file = syscall (SYS_perf_event_open, &attr, child, -1, -1, 0);
- if (file >= 0)
- close (file);
- }
-
- kill (child, SIGKILL);
- ptrace (PTRACE_KILL, child, NULL, NULL);
-
- pid = waitpid (child, &status, 0);
- if (pid != child)
- {
- warning (_("test pt: bad pid %ld, error: %s."),
- (long) pid, safe_strerror (errno));
- if (!WIFSIGNALED (status))
- warning (_("test pt: expected killed. status: %d."),
- status);
- }
-
- return (file >= 0);
- }
-}
-
/* Check whether an Intel cpu supports BTS. */
static int
@@ -594,64 +407,6 @@ cpu_supports_bts (void)
}
}
-/* Check whether the linux target supports BTS. */
-
-static int
-linux_supports_bts (void)
-{
- static int cached;
-
- if (cached == 0)
- {
- if (!kernel_supports_bts ())
- cached = -1;
- else if (!cpu_supports_bts ())
- cached = -1;
- else
- cached = 1;
- }
-
- return cached > 0;
-}
-
-/* Check whether the linux target supports Intel Processor Trace. */
-
-static int
-linux_supports_pt (void)
-{
- static int cached;
-
- if (cached == 0)
- {
- if (!kernel_supports_pt ())
- cached = -1;
- else
- cached = 1;
- }
-
- return cached > 0;
-}
-
-/* See linux-btrace.h. */
-
-int
-linux_supports_btrace (struct target_ops *ops, enum btrace_format format)
-{
- switch (format)
- {
- case BTRACE_FORMAT_NONE:
- return 0;
-
- case BTRACE_FORMAT_BTS:
- return linux_supports_bts ();
-
- case BTRACE_FORMAT_PT:
- return linux_supports_pt ();
- }
-
- internal_error (__FILE__, __LINE__, _("Unknown branch trace format"));
-}
-
class scoped_close_fd
{
int m_fd;
@@ -739,6 +494,9 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
__u64 data_offset;
int pid, pg;
+ if (!cpu_supports_bts ())
+ error (_("GDB does not support BTS for the target cpu."));
+
gdb::unique_xmalloc_ptr<btrace_target_info> tinfo
(XCNEW (btrace_target_info));
tinfo->ptid = ptid;
@@ -842,6 +600,23 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
#if defined (PERF_ATTR_SIZE_VER5)
+/* Determine the event type.
+ Returns zero on success and fills in TYPE; returns -1 otherwise. */
+
+static int
+perf_event_pt_event_type (int *type)
+{
+ gdb_file_up file =
+ gdb_fopen_cloexec ("/sys/bus/event_source/devices/intel_pt/type", "r");
+ if (file.get () == nullptr)
+ return -1;
+
+ int found = fscanf (file.get (), "%d", type);
+ if (found == 1)
+ return 0;
+ return -1;
+}
+
/* Enable branch tracing in Intel Processor Trace format. */
static struct btrace_target_info *
@@ -1213,14 +988,6 @@ linux_btrace_conf (const struct btrace_target_info *tinfo)
/* See linux-btrace.h. */
-int
-linux_supports_btrace (struct target_ops *ops, enum btrace_format format)
-{
- return 0;
-}
-
-/* See linux-btrace.h. */
-
struct btrace_target_info *
linux_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
{
diff --git a/gdb/nat/linux-btrace.h b/gdb/nat/linux-btrace.h
index 31a8d9e..1180301 100644
--- a/gdb/nat/linux-btrace.h
+++ b/gdb/nat/linux-btrace.h
@@ -103,9 +103,6 @@ struct btrace_target_info
#endif /* HAVE_LINUX_PERF_EVENT_H */
};
-/* See to_supports_btrace in target.h. */
-extern int linux_supports_btrace (struct target_ops *, enum btrace_format);
-
/* See to_enable_btrace in target.h. */
extern struct btrace_target_info *
linux_enable_btrace (ptid_t ptid, const struct btrace_config *conf);
diff --git a/gdb/remote.c b/gdb/remote.c
index 5ac84df..b12b690 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -13096,37 +13096,6 @@ remote_btrace_reset (void)
memset (&rs->btrace_config, 0, sizeof (rs->btrace_config));
}
-/* Check whether the target supports branch tracing. */
-
-static int
-remote_supports_btrace (struct target_ops *self, enum btrace_format format)
-{
- if (packet_support (PACKET_Qbtrace_off) != PACKET_ENABLE)
- return 0;
- if (packet_support (PACKET_qXfer_btrace) != PACKET_ENABLE)
- return 0;
-
- switch (format)
- {
- case BTRACE_FORMAT_NONE:
- return 0;
-
- case BTRACE_FORMAT_BTS:
- return (packet_support (PACKET_Qbtrace_bts) == PACKET_ENABLE);
-
- case BTRACE_FORMAT_PT:
- /* The trace is decoded on the host. Even if our target supports it,
- we still need to have libipt to decode the trace. */
-#if defined (HAVE_LIBIPT)
- return (packet_support (PACKET_Qbtrace_pt) == PACKET_ENABLE);
-#else /* !defined (HAVE_LIBIPT) */
- return 0;
-#endif /* !defined (HAVE_LIBIPT) */
- }
-
- internal_error (__FILE__, __LINE__, _("Unknown branch trace format"));
-}
-
/* Synchronize the configuration with the target. */
static void
@@ -13650,7 +13619,6 @@ Specify the serial device it is connected to\n\
remote_ops.to_traceframe_info = remote_traceframe_info;
remote_ops.to_use_agent = remote_use_agent;
remote_ops.to_can_use_agent = remote_can_use_agent;
- remote_ops.to_supports_btrace = remote_supports_btrace;
remote_ops.to_enable_btrace = remote_enable_btrace;
remote_ops.to_disable_btrace = remote_disable_btrace;
remote_ops.to_teardown_btrace = remote_teardown_btrace;
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index bc84791..3d90c06 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -3438,35 +3438,6 @@ debug_can_use_agent (struct target_ops *self)
return result;
}
-static int
-delegate_supports_btrace (struct target_ops *self, enum btrace_format arg1)
-{
- self = self->beneath;
- return self->to_supports_btrace (self, arg1);
-}
-
-static int
-tdefault_supports_btrace (struct target_ops *self, enum btrace_format arg1)
-{
- return 0;
-}
-
-static int
-debug_supports_btrace (struct target_ops *self, enum btrace_format arg1)
-{
- int result;
- fprintf_unfiltered (gdb_stdlog, "-> %s->to_supports_btrace (...)\n", debug_target.to_shortname);
- result = debug_target.to_supports_btrace (&debug_target, arg1);
- fprintf_unfiltered (gdb_stdlog, "<- %s->to_supports_btrace (", debug_target.to_shortname);
- target_debug_print_struct_target_ops_p (&debug_target);
- fputs_unfiltered (", ", gdb_stdlog);
- target_debug_print_enum_btrace_format (arg1);
- fputs_unfiltered (") = ", gdb_stdlog);
- target_debug_print_int (result);
- fputs_unfiltered ("\n", gdb_stdlog);
- return result;
-}
-
static struct btrace_target_info *
delegate_enable_btrace (struct target_ops *self, ptid_t arg1, const struct btrace_config *arg2)
{
@@ -4436,8 +4407,6 @@ install_delegators (struct target_ops *ops)
ops->to_use_agent = delegate_use_agent;
if (ops->to_can_use_agent == NULL)
ops->to_can_use_agent = delegate_can_use_agent;
- if (ops->to_supports_btrace == NULL)
- ops->to_supports_btrace = delegate_supports_btrace;
if (ops->to_enable_btrace == NULL)
ops->to_enable_btrace = delegate_enable_btrace;
if (ops->to_disable_btrace == NULL)
@@ -4624,7 +4593,6 @@ install_dummy_methods (struct target_ops *ops)
ops->to_traceframe_info = tdefault_traceframe_info;
ops->to_use_agent = tdefault_use_agent;
ops->to_can_use_agent = tdefault_can_use_agent;
- ops->to_supports_btrace = tdefault_supports_btrace;
ops->to_enable_btrace = tdefault_enable_btrace;
ops->to_disable_btrace = tdefault_disable_btrace;
ops->to_teardown_btrace = tdefault_teardown_btrace;
@@ -4784,7 +4752,6 @@ init_debug_target (struct target_ops *ops)
ops->to_traceframe_info = debug_traceframe_info;
ops->to_use_agent = debug_use_agent;
ops->to_can_use_agent = debug_can_use_agent;
- ops->to_supports_btrace = debug_supports_btrace;
ops->to_enable_btrace = debug_enable_btrace;
ops->to_disable_btrace = debug_disable_btrace;
ops->to_teardown_btrace = debug_teardown_btrace;
diff --git a/gdb/target.c b/gdb/target.c
index ce630f4..70d6842 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3556,14 +3556,6 @@ target_ranged_break_num_registers (void)
/* See target.h. */
-int
-target_supports_btrace (enum btrace_format format)
-{
- return current_target.to_supports_btrace (¤t_target, format);
-}
-
-/* See target.h. */
-
struct btrace_target_info *
target_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
{
diff --git a/gdb/target.h b/gdb/target.h
index 52361ba..1520e33 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1108,10 +1108,6 @@ struct target_ops
int (*to_can_use_agent) (struct target_ops *)
TARGET_DEFAULT_RETURN (0);
- /* Check whether the target supports branch tracing. */
- int (*to_supports_btrace) (struct target_ops *, enum btrace_format)
- TARGET_DEFAULT_RETURN (0);
-
/* Enable branch tracing for PTID using CONF configuration.
Return a branch trace target information struct for reading and for
disabling branch trace. */
@@ -2424,9 +2420,6 @@ extern void update_target_permissions (void);
\f
/* Imported from machine dependent code. */
-/* See to_supports_btrace in struct target_ops. */
-extern int target_supports_btrace (enum btrace_format);
-
/* See to_enable_btrace in struct target_ops. */
extern struct btrace_target_info *
target_enable_btrace (ptid_t ptid, const struct btrace_config *);
diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
index 75f68de..a3bdaff 100644
--- a/gdb/x86-linux-nat.c
+++ b/gdb/x86-linux-nat.c
@@ -340,7 +340,6 @@ x86_linux_create_target (void)
t->to_read_description = x86_linux_read_description;
/* Add btrace methods. */
- t->to_supports_btrace = linux_supports_btrace;
t->to_enable_btrace = x86_linux_enable_btrace;
t->to_disable_btrace = x86_linux_disable_btrace;
t->to_teardown_btrace = x86_linux_teardown_btrace;
--
1.8.3.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/5] btrace: prepare for throwing exceptions when enabling btrace
2018-01-25 9:12 [PATCH 0/5] improve btrace enable error reporting Markus Metzger
` (3 preceding siblings ...)
2018-01-25 9:12 ` [PATCH 3/5] btrace, gdbserver: remove the to_supports_btrace target method Markus Metzger
@ 2018-01-25 9:12 ` Markus Metzger
2018-01-25 14:46 ` Pedro Alves
4 siblings, 1 reply; 8+ messages in thread
From: Markus Metzger @ 2018-01-25 9:12 UTC (permalink / raw)
To: gdb-patches
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");
+ 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) */
--
1.8.3.1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/5] btrace: prepare for throwing exceptions when enabling btrace
2018-01-25 9:12 ` [PATCH 1/5] btrace: prepare for throwing exceptions when enabling btrace Markus Metzger
@ 2018-01-25 14:46 ` Pedro Alves
2018-01-26 13:55 ` Metzger, Markus T
0 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2018-01-25 14:46 UTC (permalink / raw)
To: Markus Metzger, gdb-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH 1/5] btrace: prepare for throwing exceptions when enabling btrace
2018-01-25 14:46 ` Pedro Alves
@ 2018-01-26 13:55 ` Metzger, Markus T
0 siblings, 0 replies; 8+ messages in thread
From: Metzger, Markus T @ 2018-01-26 13:55 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
> -----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
^ permalink raw reply [flat|nested] 8+ messages in thread