[PATCH v2 0/7] improve btrace enable error reporting

[PATCH v2 0/7] improve btrace enable error reporting

[Markus Metzger]

Recording may fail for a variety of reasons.  Improve the error
messages by stating more clearly what operation failed and try to give
a reason why it failed.

Further align the error messages for native and remote debugging.

Changes to v1:
  - move helper classes into gdb/common/
  - add unit tests for helpers
  - simplify helpers

Markus Metzger (7):
  common: add scoped_fd
  common: add scoped_mmap
  btrace: prepare for throwing exceptions when enabling btrace
  btrace, gdbserver: use exceptions to convey btrace enable/disable
    errors
  btrace, gdbserver: remove the to_supports_btrace target method
  btrace: improve enable error messages
  btrace: check perf_event_paranoid

 gdb/Makefile.in                       |   2 +
 gdb/btrace.c                          |   3 -
 gdb/common/scoped_fd.h                |  60 +++++
 gdb/common/scoped_mmap.h              |  76 ++++++
 gdb/gdbserver/linux-low.c             |   2 -
 gdb/gdbserver/nto-low.c               |   1 -
 gdb/gdbserver/server.c                |  80 +++----
 gdb/gdbserver/spu-low.c               |   1 -
 gdb/gdbserver/target.h                |   7 -
 gdb/gdbserver/win32-low.c             |   1 -
 gdb/nat/linux-btrace.c                | 427 ++++++++--------------------------
 gdb/nat/linux-btrace.h                |   3 -
 gdb/remote.c                          |  32 ---
 gdb/target-delegates.c                |  33 ---
 gdb/target.c                          |   8 -
 gdb/target.h                          |   7 -
 gdb/unittests/scoped_fd-selftests.c   |  90 +++++++
 gdb/unittests/scoped_mmap-selftests.c |  92 ++++++++
 gdb/x86-linux-nat.c                   |  20 +-
 19 files changed, 458 insertions(+), 487 deletions(-)
 create mode 100644 gdb/common/scoped_fd.h
 create mode 100644 gdb/common/scoped_mmap.h
 create mode 100644 gdb/unittests/scoped_fd-selftests.c
 create mode 100644 gdb/unittests/scoped_mmap-selftests.c

-- 
1.8.3.1

[PATCH v2 2/7] common: add scoped_mmap

[Markus Metzger]

Add a simple helper to automatically unmap a memory mapping.

2018-01-26  Markus Metzger  <markus.t.metzger@intel.com>

gdb/
	* common/scoped_mmap.h: New.
	* unittests/scoped_mmap-selftest.c: New.
	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/scoped_mmap-selftest.c.

---
 gdb/Makefile.in                       |  1 +
 gdb/common/scoped_mmap.h              | 76 +++++++++++++++++++++++++++++
 gdb/unittests/scoped_mmap-selftests.c | 92 +++++++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+)
 create mode 100644 gdb/common/scoped_mmap.h
 create mode 100644 gdb/unittests/scoped_mmap-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index a982cdf5..957654c 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -425,6 +425,7 @@ SUBDIR_UNITTESTS_SRCS = \
 	unittests/ptid-selftests.c \
 	unittests/rsp-low-selftests.c \
 	unittests/scoped_fd-selftests.c \
+	unittests/scoped_mmap-selftests.c \
 	unittests/scoped_restore-selftests.c \
 	unittests/xml-utils-selftests.c
 
diff --git a/gdb/common/scoped_mmap.h b/gdb/common/scoped_mmap.h
new file mode 100644
index 0000000..739cc70
--- /dev/null
+++ b/gdb/common/scoped_mmap.h
@@ -0,0 +1,76 @@
+/* scoped_mmap, automatically unmap files
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   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 SCOPED_MMAP_H
+#define SCOPED_MMAP_H
+
+#include "config.h"
+
+#ifdef HAVE_SYS_MMAN_H
+
+#include <sys/mman.h>
+
+/* A smart-pointer-like class to mmap() and automatically munmap() a memory
+   mapping.  */
+
+class scoped_mmap
+{
+public:
+  scoped_mmap () noexcept : m_mem (MAP_FAILED), m_length (0) {}
+  scoped_mmap (void *addr, size_t length, int prot, int flags, int fd,
+	       off_t offset) noexcept : m_length (length)
+    {
+      m_mem = mmap (addr, m_length, prot, flags, fd, offset);
+    }
+  ~scoped_mmap ()
+    {
+      if (m_mem != MAP_FAILED)
+	munmap (m_mem, m_length);
+    }
+
+  DISABLE_COPY_AND_ASSIGN (scoped_mmap);
+
+  void *release () noexcept
+    {
+      void *mem = m_mem;
+      m_mem = MAP_FAILED;
+      m_length = 0;
+      return mem;
+    }
+
+  void reset (void *addr, size_t length, int prot, int flags, int fd,
+	      off_t offset) noexcept
+    {
+      if (m_mem != MAP_FAILED)
+	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; }
+  void *get () const noexcept { return m_mem; }
+
+private:
+  void *m_mem;
+  size_t m_length;
+};
+
+#endif /* HAVE_SYS_MMAN_H */
+#endif /* SCOPED_MMAP_H */
diff --git a/gdb/unittests/scoped_mmap-selftests.c b/gdb/unittests/scoped_mmap-selftests.c
new file mode 100644
index 0000000..ece3d7a
--- /dev/null
+++ b/gdb/unittests/scoped_mmap-selftests.c
@@ -0,0 +1,92 @@
+/* Self tests for scoped_mmap for GDB, the GNU debugger.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   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/>.  */
+
+#include "defs.h"
+
+#include "common/scoped_mmap.h"
+#include "config.h"
+
+#if defined(HAVE_SYS_MMAN_H) && defined(HAVE_UNISTD_H)
+
+#include "selftest.h"
+
+#include <unistd.h>
+
+namespace selftests {
+namespace scoped_mmap {
+
+/* Test that the file is unmapped.  */
+static void
+test_destroy ()
+{
+  void *mem;
+
+  errno = 0;
+  {
+    ::scoped_mmap smmap (nullptr, sysconf (_SC_PAGESIZE), PROT_WRITE,
+			 MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
+
+    mem = smmap.get ();
+    SELF_CHECK (mem != nullptr);
+  }
+
+  SELF_CHECK (msync (mem, sysconf (_SC_PAGESIZE), 0) == -1 && errno == ENOMEM);
+}
+
+/* Test that the memory can be released.  */
+static void
+test_release ()
+{
+  void *mem;
+
+  errno = 0;
+  {
+    ::scoped_mmap smmap (nullptr, sysconf (_SC_PAGESIZE), PROT_WRITE,
+			 MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
+
+    mem = smmap.release ();
+    SELF_CHECK (mem != nullptr);
+  }
+
+  SELF_CHECK (msync (mem, sysconf (_SC_PAGESIZE), 0) == 0 || errno != ENOMEM);
+
+  munmap (mem, sysconf (_SC_PAGESIZE));
+}
+
+/* Run selftests.  */
+static void
+run_tests ()
+{
+  test_destroy ();
+  test_release ();
+}
+
+} /* namespace scoped_mmap */
+} /* namespace selftests */
+
+#endif /* !defined(HAVE_SYS_MMAN_H) || !defined(HAVE_UNISTD_H) */
+
+void
+_initialize_scoped_mmap_selftests ()
+{
+#if defined(HAVE_SYS_MMAN_H) && defined(HAVE_UNISTD_H)
+  selftests::register_test ("scoped_mmap",
+			    selftests::scoped_mmap::run_tests);
+#endif
+}
-- 
1.8.3.1

[PATCH v2 6/7] btrace: improve enable error messages

[Markus Metzger]

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-26  Markus Metzger  <markus.t.metzger@intel.com>

gdb/
	* nat/linux-btrace.c (perf_event_pt_event_type): Improve error message.
	Remove parameter and change 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 23ad778..4425539 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -448,7 +448,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
   errno = 0;
   scoped_fd fd (syscall (SYS_perf_event_open, &bts->attr, pid, -1, -1, 0));
   if (fd.get () < 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
@@ -484,6 +484,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.  */
       data.reset (nullptr, length, PROT_READ, MAP_SHARED, fd.get (), 0);
       if (data.get () != MAP_FAILED)
@@ -491,7 +492,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));
 
   struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
     data.get ();
@@ -509,7 +510,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) */
 
@@ -528,21 +529,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 ()
 {
-  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.  */
@@ -552,14 +555,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)
@@ -573,7 +569,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;
@@ -582,13 +578,13 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   errno = 0;
   scoped_fd fd (syscall (SYS_perf_event_open, &pt->attr, pid, -1, -1, 0));
   if (fd.get () < 0)
-    return nullptr;
+    error (_("Failed to start recording: %s"), safe_strerror (errno));
 
   /* Allocate the configuration page. */
   scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
 		    fd.get (), 0);
   if (data.get () == MAP_FAILED)
-    return nullptr;
+    error (_("Failed to map trace user page: %s."), safe_strerror (errno));
 
   struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
     data.get ();
@@ -630,6 +626,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
 
       header->aux_size = data_size;
 
+      errno = 0;
       aux.reset (nullptr, length, PROT_READ, MAP_SHARED, fd.get (),
 		 header->aux_offset);
       if (aux.get () != MAP_FAILED)
@@ -637,7 +634,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
     }
 
   if (pages == 0)
-    return nullptr;
+    error (_("Failed to map trace buffer: %s."), safe_strerror (errno));
 
   pt->pt.size = aux.size ();
   pt->pt.mem = (const uint8_t *) aux.release ();
@@ -656,8 +653,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) */
@@ -667,27 +663,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

[PATCH v2 1/7] common: add scoped_fd

[Markus Metzger]

Add a simple helper to automatically close a file descriptor.

2018-01-26  Markus Metzger  <markus.t.metzger@intel.com>

gdb/
	* common/scoped_fd.h: New.
	* unittests/scoped_fd-selftest.c: New.
	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/scoped_fd-selftest.c.
---
 gdb/Makefile.in                     |  1 +
 gdb/common/scoped_fd.h              | 60 +++++++++++++++++++++++++
 gdb/unittests/scoped_fd-selftests.c | 90 +++++++++++++++++++++++++++++++++++++
 3 files changed, 151 insertions(+)
 create mode 100644 gdb/common/scoped_fd.h
 create mode 100644 gdb/unittests/scoped_fd-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 0f87398..a982cdf5 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -424,6 +424,7 @@ SUBDIR_UNITTESTS_SRCS = \
 	unittests/optional-selftests.c \
 	unittests/ptid-selftests.c \
 	unittests/rsp-low-selftests.c \
+	unittests/scoped_fd-selftests.c \
 	unittests/scoped_restore-selftests.c \
 	unittests/xml-utils-selftests.c
 
diff --git a/gdb/common/scoped_fd.h b/gdb/common/scoped_fd.h
new file mode 100644
index 0000000..a6a8ab9
--- /dev/null
+++ b/gdb/common/scoped_fd.h
@@ -0,0 +1,60 @@
+/* scoped_fd, automatically close a file descriptor
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   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 SCOPED_FD_H
+#define SCOPED_FD_H
+
+#include "config.h"
+
+#ifdef HAVE_UNISTD_H
+
+#include <unistd.h>
+
+/* A smart-pointer-like class to automatically close a file descriptor.  */
+
+class scoped_fd
+{
+public:
+  explicit scoped_fd (int fd = -1) noexcept : m_fd (fd) {}
+  ~scoped_fd ()
+  {
+    if (m_fd >= 0)
+      close (m_fd);
+  }
+
+  DISABLE_COPY_AND_ASSIGN (scoped_fd);
+
+  int release () noexcept
+  {
+    int fd = m_fd;
+    m_fd = -1;
+    return fd;
+  }
+
+  int get () const noexcept
+  {
+    return m_fd;
+  }
+
+private:
+  int m_fd;
+};
+
+#endif /* HAVE_UNISTD_H */
+#endif /* SCOPED_FD_H */
diff --git a/gdb/unittests/scoped_fd-selftests.c b/gdb/unittests/scoped_fd-selftests.c
new file mode 100644
index 0000000..4d74541
--- /dev/null
+++ b/gdb/unittests/scoped_fd-selftests.c
@@ -0,0 +1,90 @@
+/* Self tests for scoped_fd for GDB, the GNU debugger.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   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/>.  */
+
+#include "defs.h"
+
+#include "common/scoped_fd.h"
+#include "config.h"
+
+#ifdef HAVE_UNISTD_H
+
+#include "selftest.h"
+
+namespace selftests {
+namespace scoped_fd {
+
+/* Test that the file descriptor is closed.  */
+static void
+test_destroy ()
+{
+  char filename[] = "scoped_fd-selftest-XXXXXX";
+  int fd = mkstemp (filename);
+  SELF_CHECK (fd >= 0);
+
+  unlink (filename);
+  errno = 0;
+  {
+    ::scoped_fd sfd (fd);
+
+    SELF_CHECK (sfd.get () == fd);
+  }
+
+  SELF_CHECK (close (fd) == -1 && errno == EBADF);
+}
+
+/* Test that the file descriptor can be released.  */
+static void
+test_release ()
+{
+  char filename[] = "scoped_fd-selftest-XXXXXX";
+  int fd = mkstemp (filename);
+  SELF_CHECK (fd >= 0);
+
+  unlink (filename);
+  errno = 0;
+  {
+    ::scoped_fd sfd (fd);
+
+    SELF_CHECK (sfd.release () == fd);
+  }
+
+  SELF_CHECK (close (fd) == 0 || errno != EBADF);
+}
+
+/* Run selftests.  */
+static void
+run_tests ()
+{
+  test_destroy ();
+  test_release ();
+}
+
+} /* namespace scoped_fd */
+} /* namespace selftests */
+
+#endif /* HAVE_UNISTD_H */
+
+void
+_initialize_scoped_fd_selftests ()
+{
+#ifdef HAVE_UNISTD_H
+  selftests::register_test ("scoped_fd",
+			    selftests::scoped_fd::run_tests);
+#endif
+}
-- 
1.8.3.1

Re: [PATCH v2 1/7] common: add scoped_fd

[Yao Qi]

On Fri, Jan 26, 2018 at 2:14 PM, Markus Metzger
<markus.t.metzger@intel.com> wrote:
> +
> +/* Test that the file descriptor is closed.  */
> +static void
> +test_destroy ()
> +{
> +  char filename[] = "scoped_fd-selftest-XXXXXX";
> +  int fd = mkstemp (filename);
> +  SELF_CHECK (fd >= 0);

Hi Markus,
I failed to build this file with i686-w64-mingw32-g++ 4.8.2, shipped
in Ubuntu 14.04.

gdb/unittests/scoped_fd-selftests.c:37:29: error: ‘mkstemp’ was not
declared in this scope
   int fd = mkstemp (filename);
                             ^
gdb/unittests/scoped_fd-selftests.c: In function ‘void
selftests::scoped_fd::test_release()’:
gdb/unittests/scoped_fd-selftests.c:56:29: error: ‘mkstemp’ was not
declared in this scope
   int fd = mkstemp (filename);
                             ^

There is no such error with i686-w64-mingw32-g++ 5.3.1.  Can we use
"open" instead?

-- 
Yao (齐尧)

RE: [PATCH v2 1/7] common: add scoped_fd

[Metzger, Markus T]

Hello Yao,

> There is no such error with i686-w64-mingw32-g++ 5.3.1.  Can we use "open"
> instead?

A fixed filename works most of the time but I'd rather use a temporary file if possible.


> > +/* Test that the file descriptor is closed.  */ static void
> > +test_destroy () {
> > +  char filename[] = "scoped_fd-selftest-XXXXXX";
> > +  int fd = mkstemp (filename);
> > +  SELF_CHECK (fd >= 0);
> 
> Hi Markus,
> I failed to build this file with i686-w64-mingw32-g++ 4.8.2, shipped in Ubuntu
> 14.04.
> 
> gdb/unittests/scoped_fd-selftests.c:37:29: error: ‘mkstemp’ was not declared in
> this scope
>    int fd = mkstemp (filename);
>                              ^

This is supposed to be a glibc function.  But it is guarded by some feature macros.
Quote from mkstemp(3): "

       mkstemp():
           _BSD_SOURCE || _SVID_SOURCE || _XOPEN_SOURCE >= 500 || _XOPEN_SOURCE && _XOPEN_SOURCE_EXTENDED
           || /* Since glibc 2.12: */ _POSIX_C_SOURCE >= 200112L

       mkostemp(): _GNU_SOURCE
       mkstemps(): _BSD_SOURCE || _SVID_SOURCE
       mkostemps(): _GNU_SOURCE
"

Maybe the newer compiler is setting some macros automatically that the older compiler doesn't set.
Could you try setting, say, _POSIX_C_SOURCE or try using mkostemp() instead of mkstemp()?

If that doesn't help, I'll try to find a Ubuntu 14.04 to reproduce the issue.

thanks,
Markus.

> -----Original Message-----
> From: Yao Qi [mailto:qiyaoltc@gmail.com]
> Sent: 13 February 2018 17:48
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: GDB Patches <gdb-patches@sourceware.org>
> Subject: Re: [PATCH v2 1/7] common: add scoped_fd
> 
> On Fri, Jan 26, 2018 at 2:14 PM, Markus Metzger <markus.t.metzger@intel.com>
> wrote:
> > +
> > +/* Test that the file descriptor is closed.  */ static void
> > +test_destroy () {
> > +  char filename[] = "scoped_fd-selftest-XXXXXX";
> > +  int fd = mkstemp (filename);
> > +  SELF_CHECK (fd >= 0);
> 
> Hi Markus,
> I failed to build this file with i686-w64-mingw32-g++ 4.8.2, shipped in Ubuntu
> 14.04.
> 
> gdb/unittests/scoped_fd-selftests.c:37:29: error: ‘mkstemp’ was not declared in
> this scope
>    int fd = mkstemp (filename);
>                              ^
> gdb/unittests/scoped_fd-selftests.c: In function ‘void
> selftests::scoped_fd::test_release()’:
> gdb/unittests/scoped_fd-selftests.c:56:29: error: ‘mkstemp’ was not declared in
> this scope
>    int fd = mkstemp (filename);
>                              ^
> 
> There is no such error with i686-w64-mingw32-g++ 5.3.1.  Can we use "open"
> instead?
> 
> --
> Yao (齐尧)
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

Re: [PATCH v2 1/7] common: add scoped_fd

[Yao Qi]

On Tue, Feb 13, 2018 at 5:28 PM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
>
> This is supposed to be a glibc function.  But it is guarded by some feature macros.
> Quote from mkstemp(3): "
>
>        mkstemp():
>            _BSD_SOURCE || _SVID_SOURCE || _XOPEN_SOURCE >= 500 || _XOPEN_SOURCE && _XOPEN_SOURCE_EXTENDED
>            || /* Since glibc 2.12: */ _POSIX_C_SOURCE >= 200112L
>
>        mkostemp(): _GNU_SOURCE
>        mkstemps(): _BSD_SOURCE || _SVID_SOURCE
>        mkostemps(): _GNU_SOURCE
> "
>
> Maybe the newer compiler is setting some macros automatically that the older compiler doesn't set.
> Could you try setting, say, _POSIX_C_SOURCE or try using mkostemp() instead of mkstemp()?
>

None of them works with my i686-w64-mingw32-g++ 4.8.2.  :(

> If that doesn't help, I'll try to find a Ubuntu 14.04 to reproduce the issue.
>

-- 
Yao (齐尧)

RE: [PATCH v2 1/7] common: add scoped_fd

[Metzger, Markus T]

Hello Yao,

> > Could you try setting, say, _POSIX_C_SOURCE or try using mkostemp() instead
> of mkstemp()?
> >
> 
> None of them works with my i686-w64-mingw32-g++ 4.8.2.  :(

Thanks for trying.  I will look into this.

Regards,
Markus.

> -----Original Message-----
> From: Yao Qi [mailto:qiyaoltc@gmail.com]
> Sent: 14 February 2018 16:23
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: GDB Patches <gdb-patches@sourceware.org>
> Subject: Re: [PATCH v2 1/7] common: add scoped_fd
> 
> On Tue, Feb 13, 2018 at 5:28 PM, Metzger, Markus T
> <markus.t.metzger@intel.com> wrote:
> >
> > This is supposed to be a glibc function.  But it is guarded by some feature
> macros.
> > Quote from mkstemp(3): "
> >
> >        mkstemp():
> >            _BSD_SOURCE || _SVID_SOURCE || _XOPEN_SOURCE >= 500 ||
> _XOPEN_SOURCE && _XOPEN_SOURCE_EXTENDED
> >            || /* Since glibc 2.12: */ _POSIX_C_SOURCE >= 200112L
> >
> >        mkostemp(): _GNU_SOURCE
> >        mkstemps(): _BSD_SOURCE || _SVID_SOURCE
> >        mkostemps(): _GNU_SOURCE
> > "
> >
> > Maybe the newer compiler is setting some macros automatically that the older
> compiler doesn't set.
> > Could you try setting, say, _POSIX_C_SOURCE or try using mkostemp() instead
> of mkstemp()?
> >
> 
> None of them works with my i686-w64-mingw32-g++ 4.8.2.  :(
> 
> > If that doesn't help, I'll try to find a Ubuntu 14.04 to reproduce the issue.
> >
> 
> --
> Yao (齐尧)
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

RE: [PATCH v2 1/7] common: add scoped_fd

[Metzger, Markus T]

Hello Yao,

Looks like they added mkstemp() in 2015 after a few complaints:
https://sourceforge.net/u/jeisele123/mingw-w64/ci/f713f639f6f017371c5176f4deab07d1a92473b4/

How about checking for mkstemp and, if not available, falling back to tmpnam.
Something like this:

#ifndef HAVE_MKSTEMP

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>

/* Define a racy version of mkstemp based on tmpnam for systems that do not
   support mkstemp.  */
static int
mkstemp (char *name)
{
  if (strlen (name) < L_tmpnam)
    return -1;

  name = tmpnam (name);
  if (name == nullptr)
    return -1;

  return open (name);
}

#endif /* HAVE_MKSTEMP */


regards,
markus.

> -----Original Message-----
> From: Metzger, Markus T
> Sent: 14 February 2018 18:26
> To: Yao Qi <qiyaoltc@gmail.com>
> Cc: GDB Patches <gdb-patches@sourceware.org>
> Subject: RE: [PATCH v2 1/7] common: add scoped_fd
> 
> Hello Yao,
> 
> > > Could you try setting, say, _POSIX_C_SOURCE or try using mkostemp()
> > > instead
> > of mkstemp()?
> > >
> >
> > None of them works with my i686-w64-mingw32-g++ 4.8.2.  :(
> 
> Thanks for trying.  I will look into this.
> 
> Regards,
> Markus.
> 
> > -----Original Message-----
> > From: Yao Qi [mailto:qiyaoltc@gmail.com]
> > Sent: 14 February 2018 16:23
> > To: Metzger, Markus T <markus.t.metzger@intel.com>
> > Cc: GDB Patches <gdb-patches@sourceware.org>
> > Subject: Re: [PATCH v2 1/7] common: add scoped_fd
> >
> > On Tue, Feb 13, 2018 at 5:28 PM, Metzger, Markus T
> > <markus.t.metzger@intel.com> wrote:
> > >
> > > This is supposed to be a glibc function.  But it is guarded by some
> > > feature
> > macros.
> > > Quote from mkstemp(3): "
> > >
> > >        mkstemp():
> > >            _BSD_SOURCE || _SVID_SOURCE || _XOPEN_SOURCE >= 500 ||
> > _XOPEN_SOURCE && _XOPEN_SOURCE_EXTENDED
> > >            || /* Since glibc 2.12: */ _POSIX_C_SOURCE >= 200112L
> > >
> > >        mkostemp(): _GNU_SOURCE
> > >        mkstemps(): _BSD_SOURCE || _SVID_SOURCE
> > >        mkostemps(): _GNU_SOURCE
> > > "
> > >
> > > Maybe the newer compiler is setting some macros automatically that
> > > the older
> > compiler doesn't set.
> > > Could you try setting, say, _POSIX_C_SOURCE or try using mkostemp()
> > > instead
> > of mkstemp()?
> > >
> >
> > None of them works with my i686-w64-mingw32-g++ 4.8.2.  :(
> >
> > > If that doesn't help, I'll try to find a Ubuntu 14.04 to reproduce the issue.
> > >
> >
> > --
> > Yao (齐尧)
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

Re: [PATCH v2 1/7] common: add scoped_fd

[Eli Zaretskii]

> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: GDB Patches <gdb-patches@sourceware.org>
> Date: Mon, 19 Feb 2018 15:28:46 +0000
> 
> How about checking for mkstemp and, if not available, falling back to tmpnam.

Why not import the corresponding Gnulib module?

Re: [PATCH v2 1/7] common: add scoped_fd

[Yao Qi]

On Mon, Feb 19, 2018 at 3:28 PM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
> Hello Yao,
>
> Looks like they added mkstemp() in 2015 after a few complaints:
> https://sourceforge.net/u/jeisele123/mingw-w64/ci/f713f639f6f017371c5176f4deab07d1a92473b4/
>

Thanks for looking into this...

> How about checking for mkstemp and, if not available, falling back to tmpnam.

Can we unconditionally use tmpnam+open which are more portable?  I know
it is racy, but mkstemp is only used in a test case.  It is overkill to me to do
the configure check or import gnulib module.

If we use mkstemp in gdb source (not test case) one day in the future, I prefer
importing gnulib module.  What do you think?

> Something like this:
>
> #ifndef HAVE_MKSTEMP
>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <stdio.h>
>
> /* Define a racy version of mkstemp based on tmpnam for systems that do not
>    support mkstemp.  */
> static int
> mkstemp (char *name)
> {
>   if (strlen (name) < L_tmpnam)
>     return -1;
>
>   name = tmpnam (name);
>   if (name == nullptr)
>     return -1;
>
>   return open (name);
> }
>
> #endif /* HAVE_MKSTEMP */
>

-- 
Yao (齐尧)

RE: [PATCH v2 1/7] common: add scoped_fd

[Metzger, Markus T]

Hello Yao, Eli,

> > How about checking for mkstemp and, if not available, falling back to tmpnam.
> 
> Can we unconditionally use tmpnam+open which are more portable?  I know it is
> racy, but mkstemp is only used in a test case.  It is overkill to me to do the configure
> check or import gnulib module.
> 
> If we use mkstemp in gdb source (not test case) one day in the future, I prefer
> importing gnulib module.  What do you think?

I'd rather not use tmpnam.  This is really only a fallback for old systems.


I added mkstemp to update-gnulib.sh ...
 
---
diff --git a/gdb/gnulib/update-gnulib.sh b/gdb/gnulib/update-gnulib.sh
index 55cd6e0..d026536 100755
--- a/gdb/gnulib/update-gnulib.sh
+++ b/gdb/gnulib/update-gnulib.sh
@@ -59,6 +59,7 @@ IMPORTED_GNULIB_MODULES="\
     update-copyright \
     wchar \
     wctype-h \
+    mkstemp \
 "
 
 # The gnulib commit ID to use for the update.
---

... cloned gnulib, updated my automake, ran that script, and ended up with:

#       modified:   gdb/gnulib/aclocal.m4
#       modified:   gdb/gnulib/config.in
#       modified:   gdb/gnulib/configure
#       modified:   gdb/gnulib/import/Makefile.am
#       modified:   gdb/gnulib/import/Makefile.in
#       modified:   gdb/gnulib/import/m4/gnulib-cache.m4
#       modified:   gdb/gnulib/import/m4/gnulib-comp.m4
#       new file:   gdb/gnulib/import/m4/mkstemp.m4
#       new file:   gdb/gnulib/import/m4/secure_getenv.m4
#       new file:   gdb/gnulib/import/m4/tempname.m4
#       new file:   gdb/gnulib/import/mkstemp.c
#       new file:   gdb/gnulib/import/secure_getenv.c
#       new file:   gdb/gnulib/import/tempname.c
#       new file:   gdb/gnulib/import/tempname.h
#       modified:   gdb/gnulib/update-gnulib.sh


If I did everything correctly, it looks like gnulib would only provide mkstemp as a fall-back.  On
my system, it would still use the glibc version.  Good.  I wouldn't want to replace a standard
glibc function.

I have not tried building GDB with mingw cross tools, yet.  Would you be able to test whether
this solves your build problem, Yao?

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

Re: [PATCH v2 1/7] common: add scoped_fd

[Yao Qi]

On Tue, Feb 20, 2018 at 10:46 AM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
> ---
> diff --git a/gdb/gnulib/update-gnulib.sh b/gdb/gnulib/update-gnulib.sh
> index 55cd6e0..d026536 100755
> --- a/gdb/gnulib/update-gnulib.sh
> +++ b/gdb/gnulib/update-gnulib.sh
> @@ -59,6 +59,7 @@ IMPORTED_GNULIB_MODULES="\
>      update-copyright \
>      wchar \
>      wctype-h \
> +    mkstemp \
>  "
>

We should keep IMPORTED_GNULIB_MODULES in alphabetical order.
Otherwise, patch is good to me.

>
> I have not tried building GDB with mingw cross tools, yet.  Would you be able to test whether
> this solves your build problem, Yao?
>

Yes, it fixed my build problem.

-- 
Yao (齐尧)

RE: [PATCH v2 1/7] common: add scoped_fd

[Metzger, Markus T]

> -----Original Message-----
> From: Yao Qi [mailto:qiyaoltc@gmail.com]
> Sent: 20 February 2018 12:10
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: Eli Zaretskii (eliz@gnu.org) <eliz@gnu.org>; GDB Patches <gdb-
> patches@sourceware.org>
> Subject: Re: [PATCH v2 1/7] common: add scoped_fd
> 
> On Tue, Feb 20, 2018 at 10:46 AM, Metzger, Markus T
> <markus.t.metzger@intel.com> wrote:
> > ---
> > diff --git a/gdb/gnulib/update-gnulib.sh b/gdb/gnulib/update-gnulib.sh
> > index 55cd6e0..d026536 100755
> > --- a/gdb/gnulib/update-gnulib.sh
> > +++ b/gdb/gnulib/update-gnulib.sh
> > @@ -59,6 +59,7 @@ IMPORTED_GNULIB_MODULES="\
> >      update-copyright \
> >      wchar \
> >      wctype-h \
> > +    mkstemp \
> >  "
> >
> 
> We should keep IMPORTED_GNULIB_MODULES in alphabetical order.
> Otherwise, patch is good to me.

Pushed.

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

[PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Markus Metzger]

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-26  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 2b37e41..23ad778 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -175,23 +175,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 == 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
@@ -376,176 +359,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
@@ -596,64 +409,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"));
-}
-
 /* Enable branch tracing in BTS format.  */
 
 static struct btrace_target_info *
@@ -664,6 +419,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;
@@ -770,6 +528,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 *
@@ -1146,14 +921,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 (&current_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

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Maciej W. Rozycki]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Fri, 26 Jan 2018, Markus Metzger wrote:

> 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.

 Hmm, v3 of this change (apparently never posted), that is specifically 
commit de6242d30757 ("btrace, gdbserver: remove the to_supports_btrace 
target method"), has broken remote `mips-linux' target debugging 
completely, that is an attempt to make a remote connection fails in the 
initial handshake, e.g.:

Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 25519
Listening on port 2346
target remote 1.2.3.4:2346
Remote debugging using 1.2.3.4:2346
Reading symbols from .../lib/ld.so.1...done.
0x77fc8de0 in __start () from .../lib/ld.so.1
Protocol error: qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.
(gdb) continue
The program is not being run.
(gdb) FAIL: gdb.base/advance.exp: can't run to main

See the attached RSP packet exchange log for details.  Please investigate.

  Maciej

[-- Attachment #2: Type: text/plain, Size: 10082 bytes --]

Remote debugging using 1.2.3.4:2346
Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;xmlRegisters=i386#6a...Ack
Packet received: PacketSize=3fff;QPassSignals+;QProgramSignals+;QStartupWithShell+;QEnvironmentHexEncoded+;QEnvironmentReset+;QEnvironmentUnset+;QSetWorkingDir+;qXfer:libraries-svr4:read+;augmented-libraries-svr4-read+;qXfer:auxv:read+;qXfer:spu:read+;qXfer:spu:write+;qXfer:siginfo:read+;qXfer:siginfo:write+;qXfer:features:read+;QStartNoAckMode+;qXfer:osdata:read+;multiprocess+;fork-events+;vfork-events+;exec-events+;QNonStop+;QDisableRandomization+;qXfer:threads:read+;BreakpointCommands+;QAgent+;Qbtrace:bts+;Qbtrace-conf:b
Packet qSupported (supported-packets) is supported
Sending packet: $vMustReplyEmpty#3a...Ack
Packet received: 
Sending packet: $QStartNoAckMode#b0...Ack
Packet received: OK
Sending packet: $QProgramSignals:0;1;3;4;6;7;8;9;a;b;c;d;e;f;10;11;12;13;14;15;16;17;18;19;1a;1b;1c;1d;1e;1f;20;21;22;23;24;25;26;27;28;29;2a;2b;2c;2d;2e;2f;30;31;32;33;34;35;36;37;38;39;3a;3b;3c;3d;3e;3f;40;41;42;43;44;45;46;47;48;49;4a;4b;4c;4d;4e;4f;50;51;52;53;54;55;56;57;58;59;5a;5b;5c;5d;5e;5f;60;61;62;63;64;65;66;67;68;69;6a;6b;6c;6d;6e;6f;70;71;72;73;74;75;76;77;78;79;7a;7b;7c;7d;7e;7f;80;81;82;83;84;85;86;87;88;89;8a;8b;8c;8d;8e;8f;90;91;92;93;94;95;96;97;#75...Packet received: OK
Sending packet: $Hgp0.0#ad...Packet received: OK
Sending packet: $qXfer:features:read:target.xml:0,fff#7d...Packet received: l<?xml version="1.0"?>\n<!-- Copyright (C) 2012-2018 Free Software Foundation, Inc.\n\n     Copying and distribution of this file, with or without modification,\n     are permitted in any medium without royalty provided the copyright\n     notice and this notice are preserved.  -->\n\n<!DOCTYPE target SYSTEM "gdb-target.dtd">\n<target>\n  <architecture>mips</architecture>\n  <osabi>GNU/Linux</osabi>\n  <xi:include href="mips-cpu.xml"/>\n  <xi:include href="mips-cp0.xml"/>\n  <xi:include href="mips-fpu.xml"/>\n  <xi:inclu[14 bytes omitted]
Sending packet: $qXfer:features:read:mips-cpu.xml:0,fff#24...Packet received: l<?xml version="1.0"?>\n<!-- Copyright (C) 2007-2018 Free Software Foundation, Inc.\n\n     Copying and distribution of this file, with or without modification,\n     are permitted in any medium without royalty provided the copyright\n     notice and this notice are preserved.  -->\n\n<!DOCTYPE feature SYSTEM "gdb-target.dtd">\n<feature name="org.gnu.gdb.mips.cpu">\n  <reg name="r0" bitsize="32" regnum="0"/>\n  <reg name="r1" bitsize="32"/>\n  <reg name="r2" bitsize="32"/>\n  <reg name="r3" bitsize="32"/>\n  <reg name="[13 bytes omitted]
Sending packet: $qXfer:features:read:mips-cp0.xml:0,fff#df...Packet received: l<?xml version="1.0"?>\n<!-- Copyright (C) 2007-2018 Free Software Foundation, Inc.\n\n     Copying and distribution of this file, with or without modification,\n     are permitted in any medium without royalty provided the copyright\n     notice and this notice are preserved.  -->\n\n<!DOCTYPE feature SYSTEM "gdb-target.dtd">\n<feature name="org.gnu.gdb.mips.cp0">\n  <reg name="status" bitsize="32" regnum="32"/>\n  <reg name="badvaddr" bitsize="32" regnum="35"/>\n  <reg name="cause" bitsize="32" regnum="36"/>\n</featu[12 bytes omitted]
Sending packet: $qXfer:features:read:mips-fpu.xml:0,fff#27...Packet received: l<?xml version="1.0"?>\n<!-- Copyright (C) 2007-2018 Free Software Foundation, Inc.\n\n     Copying and distribution of this file, with or without modification,\n     are permitted in any medium without royalty provided the copyright\n     notice and this notice are preserved.  -->\n\n<!DOCTYPE feature SYSTEM "gdb-target.dtd">\n<feature name="org.gnu.gdb.mips.fpu">\n  <reg name="f0" bitsize="32" type="ieee_single" regnum="38"/>\n  <reg name="f1" bitsize="32" type="ieee_single"/>\n  <reg name="f2" bitsize="32" type="ie[11 bytes omitted]
Sending packet: $qXfer:features:read:mips-dsp.xml:0,fff#23...Packet received: l<?xml version="1.0"?>\n<!-- Copyright (C) 2012-2018 Free Software Foundation, Inc.\n\n     Copying and distribution of this file, with or without modification,\n     are permitted in any medium without royalty provided the copyright\n     notice and this notice are preserved.  -->\n\n<!DOCTYPE feature SYSTEM "gdb-target.dtd">\n<feature name="org.gnu.gdb.mips.dsp">\n  <reg name="hi1" bitsize="32" regnum="72"/>\n  <reg name="lo1" bitsize="32" regnum="73"/>\n  <reg name="hi2" bitsize="32" regnum="74"/>\n  <reg name="lo2"[12 bytes omitted]
Sending packet: $QNonStop:0#8c...Packet received: OK
Sending packet: $qTStatus#49...Packet received: 
Packet qTStatus (trace-status) is NOT supported
Sending packet: $?#3f...Packet received: T051d:d0baff7f;25:e08dfc77;thread:p6a61.6a61;core:0;
Sending packet: $qXfer:threads:read::0,fff#03...Packet received: l<threads>\n<thread id="p6a61.6a61" core="0" name="advance"/>\n</threads>\n
Sending packet: $qAttached:6a61#c7...Packet received: 0
Packet qAttached (query-attached) is supported
Sending packet: $Hc-1#09...Packet received: E01
Sending packet: $qOffsets#4b...Packet received: 
Sending packet: $qXfer:libraries-svr4:read::0,fff#91...Packet received: l<library-list-svr4 version="1.0"/>
Sending packet: $qXfer:auxv:read::0,1000#6b...Packet received: l!\000\000\000\000@ÿw\020\000\000\000\000\000\000\000\006\000\000\000\000@\000\000\021\000\000\000d\000\000\000\003\000\000\0004\000@\000\004\000\000\000 \000\000\000\005\000\000\000\t\000\000\000\a\000\000\000\000\200üw\b\000\000\000\000\000\000\000\t\000\000\000À\005@\000\013\000\000\000cy\000\000\f\000\000\000cy\000\000\r\000\000\000\204\003\000\000\016\000\000\000\204\003\000\000\027\000\000\000\000\000\000\000\031\000\000\000Ó»ÿ\177\037\000\000\000\221¿ÿ\177\017\000\000\000ã»ÿ\177\000\000\000\000\000\000\000\000[10 bytes omitted]
Sending packet: $vFile:setfs:0#bf...Packet received: F0
Packet vFile:setfs (hostio-setfs) is supported
Sending packet: $vFile:open:2f70726f632f32373233332f7461736b2f32373233332f6d617073,0,1c0#db...Packet received: F3
Packet vFile:open (hostio-open) is supported
readahead cache miss 251
Sending packet: $vFile:pread:3,3fff,0#96...Packet received: F33c;00400000-00404000 r-xp 00000000 00:1d 357628039  .../gdb/testsuite/outputs/gdb.base/advance/advance\n00410000-00414000 rwxp 00000000 00:1d 357628039  .../gdb/testsuite/outputs/gdb.base/advance/advance\n77fc8000-77fec000 r-xp 00000000 00:1d 3529929641  .../lib/ld-2.27.9000.so\n77ff0000-77ff4000 r--p 00000000 00:00 0   [3 bytes omitted]
Packet vFile:pread (hostio-pread) is supported
readahead cache miss 252
Sending packet: $vFile:pread:3,3fff,33c#2f...Packet received: F0;
Sending packet: $vFile:close:3#b3...Packet received: F0
Packet vFile:close (hostio-close) is supported
Sending packet: $qXfer:libraries-svr4:read::0,fff#91...Packet received: l<library-list-svr4 version="1.0"/>
Reading symbols from .../lib/ld.so.1...done.
Sending packet: $qSymbol::#5b...Packet received: qSymbol:6e70746c5f76657273696f6e
Packet qSymbol (symbol-lookup) is supported
Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Packet received: OK
Sending packet: $m77fd9d40,4#06...Packet received: 0800e003
Sending packet: $m77ff4000,34#fe...Packet received: 7f454c4601010100000000000000000003000800010000009002000034000000c40b00000510007034002000070028000f000e00
Sending packet: $m77ff4034,e0#33...Packet received: 0300007018010000180100001801000018000000180000000400000008000000000000703001000030010000300100001800000018000000040000000400000001000000000000000000000000000000980a0000980a0000050000000000010002000000d0090000d0090000d0090000c0000000c00000000400000004000000040000004802000048020000480200003c0000003c000000040000000400000050e57464000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000004000000
Sending packet: $m77ff4000,1fff#fa...Packet received: 7f454c4601010100000000000000000003000800010000009002000034000000c40b00000510007034002000070028000f000e000300007018010000180100001801000018000000180000000400000008000000000000703001000030010000300100001800000018000000040000000400000001000000000000000000000000000000980a0000980a0000050000000000010002000000d0090000d0090000d0090000c0000000c00000000400000004000000040000004802000048020000480200003c0000003c000000040000000400000050e5746400000000000000000000000000000000000000000000000004000000000000000000000000000000
Sending packet: $m77ff5fff,1fff#9d...Packet received: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
Sending packet: $m77ff7ffe,2#6d...Packet received: 0000
Sending packet: $qSymbol::#5b...Packet received: qSymbol:6e70746c5f76657273696f6e
Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Packet received: OK
Sending packet: $qXfer:threads:read::0,fff#03...Packet received: l<threads>\n<thread id="p6a61.6a61" core="0" name="advance"/>\n</threads>\n
Sending packet: $m77fc8de0,4#35...Packet received: 25c8e003
Sending packet: $m77fc8ddc,4#67...Packet received: 00000000
Sending packet: $m77fc8de0,4#35...Packet received: 25c8e003
Sending packet: $m77fc8ddc,4#67...Packet received: 00000000
Sending packet: $m77fc8de0,4#35...Packet received: 25c8e003
0x77fc8de0 in __start ()
   from .../lib/ld.so.1
Sending packet: $qSymbol::#5b...Packet received: qSymbol:6e70746c5f76657273696f6e
Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Packet received: OK
Sending packet: $qXfer:btrace-conf:read::0,fff#5c...Packet received: 
Protocol error: qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Metzger, Markus T]

Hello Maciej,

> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid =
> 25519 Listening on port 2346 target remote 1.2.3.4:2346 Remote debugging using
> 1.2.3.4:2346 Reading symbols from .../lib/ld.so.1...done.
> 0x77fc8de0 in __start () from .../lib/ld.so.1 Protocol error: qXfer:btrace-conf
> (read-btrace-conf) conflicting enabled responses.
> (gdb) continue
> The program is not being run.
> (gdb) FAIL: gdb.base/advance.exp: can't run to main
> 
> See the attached RSP packet exchange log for details.  Please investigate.

Below is a patch to address this.  I tested it on IA by undefining HAVE_LINUX_BTRACE.
Does it fix the issue you reported?

thanks,
Markus.

---

commit c4d017399e2cc62cfed793bb93967bd6b3d573bb
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Mon Feb 26 11:59:43 2018 +0100

    btrace, gdbserver: check btrace target pointers
    
    By removing the supports_btrace gdbserver target method we relied on GDB trying
    to enable branch tracing and failing on the attempt.
    
    For targets that do not provide the btrace methods, however, an initial request
    from GDB for the branch trace configuration to detect whether gdbserver is
    already recording resulted in a protocol error.
    
    Have the btrace target methods throw a "Target does not suppor branch tracing"
    error and be prepared to handle exceptions in all functions that call btrace
    target methods.  We therefore turn the target_* macros into static inline
    functions.
    
    Also remove the additional btrace target method checks that resulted in the
    above protocol error.
    
    Thanks to Maciej W. Rozycki <macro@mips.com> for reporting this.
    
    Signed-off-by: Markus Metzger  <markus.t.metzger@intel.com>
    
    gdbserver/
    	* target.h (target_enable_btrace, target_disable_btrace,
    	target_read_btrace, target_read_btrace_conf): Turn macro into inline
    	function.  Throw error if target method not defined.
    	* server.c (handle_qxfer_btrace, handle_qxfer_btrace_conf): Remove
    	check for btrace target method.  Be prepared to handle exceptions
    	from btrace target methods.

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index cb02b58..4fd274f 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1818,7 +1818,7 @@ handle_qxfer_btrace (const char *annex,
   enum btrace_read_type type;
   int result;
 
-  if (the_target->read_btrace == NULL || writebuf != NULL)
+  if (writebuf != NULL)
     return -2;
 
   if (ptid_equal (general_thread, null_ptid)
@@ -1857,12 +1857,21 @@ handle_qxfer_btrace (const char *annex,
     {
       buffer_free (&cache);
 
-      result = target_read_btrace (thread->btrace, &cache, type);
-      if (result != 0)
+      TRY
 	{
-	  memcpy (own_buf, cache.buffer, cache.used_size);
-	  return -3;
+	  result = target_read_btrace (thread->btrace, &cache, type);
+	  if (result != 0)
+	    memcpy (own_buf, cache.buffer, cache.used_size);
 	}
+      CATCH (exception, RETURN_MASK_ERROR)
+	{
+	  sprintf (own_buf, "E.%s", exception.message);
+	  result = -1;
+	}
+      END_CATCH
+
+      if (result != 0)
+	return -3;
     }
   else if (offset > cache.used_size)
     {
@@ -1889,7 +1898,7 @@ handle_qxfer_btrace_conf (const char *annex,
   struct thread_info *thread;
   int result;
 
-  if (the_target->read_btrace_conf == NULL || writebuf != NULL)
+  if (writebuf != NULL)
     return -2;
 
   if (annex[0] != '\0')
@@ -1919,12 +1928,21 @@ handle_qxfer_btrace_conf (const char *annex,
     {
       buffer_free (&cache);
 
-      result = target_read_btrace_conf (thread->btrace, &cache);
-      if (result != 0)
+      TRY
 	{
-	  memcpy (own_buf, cache.buffer, cache.used_size);
-	  return -3;
+	  result = target_read_btrace_conf (thread->btrace, &cache);
+	  if (result != 0)
+	    memcpy (own_buf, cache.buffer, cache.used_size);
 	}
+      CATCH (exception, RETURN_MASK_ERROR)
+	{
+	  sprintf (own_buf, "E.%s", exception.message);
+	  result = -1;
+	}
+      END_CATCH
+
+      if (result != 0)
+	return -3;
     }
   else if (offset > cache.used_size)
     {
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index dcefe1a..923a63b 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -620,17 +620,42 @@ int kill_inferior (int);
   (the_target->supports_agent ? \
    (*the_target->supports_agent) () : 0)
 
-#define target_enable_btrace(ptid, conf) \
-  (*the_target->enable_btrace) (ptid, conf)
+static inline struct btrace_target_info *
+target_enable_btrace(ptid_t ptid, const struct btrace_config *conf)
+{
+  if (the_target->enable_btrace == nullptr)
+    error ("Target does not support branch tracing.");
+
+  return (*the_target->enable_btrace) (ptid, conf);
+}
+
+static inline int
+target_disable_btrace(struct btrace_target_info *tinfo)
+{
+  if (the_target->disable_btrace == nullptr)
+    error ("Target does not support branch tracing.");
 
-#define target_disable_btrace(tinfo) \
-  (*the_target->disable_btrace) (tinfo)
+  return (*the_target->disable_btrace) (tinfo);
+}
 
-#define target_read_btrace(tinfo, buffer, type)	\
-  (*the_target->read_btrace) (tinfo, buffer, type)
+static inline int
+target_read_btrace(struct btrace_target_info *tinfo, struct buffer *buffer,
+		   enum btrace_read_type type)
+{
+  if (the_target->read_btrace == nullptr)
+    error ("Target does not support branch tracing.");
+
+  return (*the_target->read_btrace) (tinfo, buffer, type);
+}
+
+static inline int
+target_read_btrace_conf(struct btrace_target_info *tinfo, struct buffer *buffer)
+{
+  if (the_target->read_btrace_conf == nullptr)
+    error ("Target does not support branch tracing.");
 
-#define target_read_btrace_conf(tinfo, buffer)	\
-  (*the_target->read_btrace_conf) (tinfo, buffer)
+  return (*the_target->read_btrace_conf) (tinfo, buffer);
+}
 
 #define target_supports_range_stepping() \
   (the_target->supports_range_stepping ? \

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

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Andreas Arnez]

On Mon, Feb 26 2018, Metzger, Markus T wrote:

> Hello Maciej,
>
>> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid =
>> 25519 Listening on port 2346 target remote 1.2.3.4:2346 Remote debugging using
>> 1.2.3.4:2346 Reading symbols from .../lib/ld.so.1...done.
>> 0x77fc8de0 in __start () from .../lib/ld.so.1 Protocol error: qXfer:btrace-conf
>> (read-btrace-conf) conflicting enabled responses.
>> (gdb) continue
>> The program is not being run.
>> (gdb) FAIL: gdb.base/advance.exp: can't run to main
>> 
>> See the attached RSP packet exchange log for details.  Please investigate.

For the record, the same happens on s390x.  It seems that you broke all
targets without HAVE_LINUX_BTRACE.

> Below is a patch to address this.  I tested it on IA by undefining HAVE_LINUX_BTRACE.
> Does it fix the issue you reported?

I've tested your patch on s390x, and it seems to fix the problem.

--
Andreas

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Maciej W. Rozycki]

Hi Markus,

 Thank you for the quick action on this problem.

On Mon, 26 Feb 2018, Andreas Arnez wrote:

> >> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid =
> >> 25519 Listening on port 2346 target remote 1.2.3.4:2346 Remote debugging using
> >> 1.2.3.4:2346 Reading symbols from .../lib/ld.so.1...done.
> >> 0x77fc8de0 in __start () from .../lib/ld.so.1 Protocol error: qXfer:btrace-conf
> >> (read-btrace-conf) conflicting enabled responses.
> >> (gdb) continue
> >> The program is not being run.
> >> (gdb) FAIL: gdb.base/advance.exp: can't run to main
> >> 
> >> See the attached RSP packet exchange log for details.  Please investigate.
> 
> For the record, the same happens on s390x.  It seems that you broke all
> targets without HAVE_LINUX_BTRACE.

 I suspected that might be the case.  With your future changes that touch 
generic code would you please regression-test them with another target, or 
at least ask people to do so before committing them, so that such a wide 
breakage is avoided?  Having several versions not working at all makes it 
a pain to bisect changes that may have caused regressions meanwhile.

 Also did you verify that old-GDB/new-gdbserver and new-GDB/old-gdbserver 
combinations work correctly?

> > Below is a patch to address this.  I tested it on IA by undefining HAVE_LINUX_BTRACE.
> > Does it fix the issue you reported?
> 
> I've tested your patch on s390x, and it seems to fix the problem.

 I have smoke-tested your change with `gdb.base/advance.exp' and one of 
the configurations that failed previously, and it has scored all-pass now.  
I'll schedule full testing overnight.

  Maciej

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Metzger, Markus T]

Hello Maciej, Andreas,

Sorry about the breakage.  I hope everything's working again with that patch:

https://sourceware.org/ml/gdb-patches/2018-02/msg00420.html


It's the same I sent inline to the email with a recurring white-space error fixed.

Andreas already indicated that the patch fixes the problem on s390x.  Did the full
test show any further issues on your side, Maciej?


>  Also did you verify that old-GDB/new-gdbserver and new-GDB/old-gdbserver
> combinations work correctly?

That was discussed here:
https://sourceware.org/ml/gdb-patches/2018-02/msg00117.html

regards,
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

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Maciej W. Rozycki]

Hi Markus,

> https://sourceware.org/ml/gdb-patches/2018-02/msg00420.html
> 
> 
> It's the same I sent inline to the email with a recurring white-space 
> error fixed.

 With the change description you have also overrun our 74-column limit: 
<https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Column_limits>. 
NB due to how `git log' etc. indents descriptions I prefer to stay within 
72 columns with my own changes for a better visual effect, though you are 
of course free to use your own judgement here as long as you're within 74 
columns.

 Same with ChangeLog entries.  Also as per the the GNU Coding Standard:
<https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs>
long function lists, etc. use `)' as the closing character, so:

	* target.h (target_enable_btrace, target_disable_btrace)
	(target_read_btrace, target_read_btrace_conf): Turn macro into 
	inline function.  Throw error if target method not defined.

 You need to mark new error messages for translation, so:

+    error (_("Target does not support branch tracing."));

etc.

 This:

+static inline int
+target_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
+		    enum btrace_read_type type)

also overruns the 74-column limit.

 The formatting appears otherwise OK to me and I'll leave the matter 
itself of this change up to someone with suitable expertise in this area.

> Andreas already indicated that the patch fixes the problem on s390x.  Did the full
> test show any further issues on your side, Maciej?

 We had a failure last night in the lab literally as I have scheduled the 
tests and consequently they didn't run.  I'll schedule them again as soon 
as possible, but the target system I have been using for testing is not 
back up yet, so it may not happen before tomorrow.  Sorry.

> >  Also did you verify that old-GDB/new-gdbserver and new-GDB/old-gdbserver
> > combinations work correctly?
> 
> That was discussed here:
> https://sourceware.org/ml/gdb-patches/2018-02/msg00117.html

 Thanks for confirming.  Is there going to be any difference here for 
non-x86 targets that needs to be verified?

  Maciej

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Metzger, Markus T]

Hello Maciej,

>  With the change description you have also overrun our 74-column limit:
> <https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-
> Standards#Column_limits>.
> NB due to how `git log' etc. indents descriptions I prefer to stay within
> 72 columns with my own changes for a better visual effect, though you are of
> course free to use your own judgement here as long as you're within 74 columns.

Actually, the hard limit is 80 columns.  I've been using that, so far, and from
looking at other files in gdb/ it looks like others were using the 80 columns limit,
as well.


>  Same with ChangeLog entries.  Also as per the the GNU Coding Standard:
> <https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs>
> long function lists, etc. use `)' as the closing character, so:
> 
> 	* target.h (target_enable_btrace, target_disable_btrace)
> 	(target_read_btrace, target_read_btrace_conf): Turn macro into
> 	inline function.  Throw error if target method not defined.
> 
>  You need to mark new error messages for translation, so:
> 
> +    error (_("Target does not support branch tracing."));

Thanks for pointing those out.  I fixed them locally.

  
> > >  Also did you verify that old-GDB/new-gdbserver and
> > > new-GDB/old-gdbserver combinations work correctly?
> >
> > That was discussed here:
> > https://sourceware.org/ml/gdb-patches/2018-02/msg00117.html
> 
>  Thanks for confirming.  Is there going to be any difference here for
> non-x86 targets that needs to be verified?

They're supposed to work the same way.  I.e. older gdbservers won't
advertise the packets and newer GDB's hence won't use them.  And
newer gdbservers would advertise the packets and, with this patch,
fail on every request with "E.Target does not support branch tracing".

Let me check that once for both directions just to be sure.

Regards,
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

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Maciej W. Rozycki]

Hi Markus,

> >  With the change description you have also overrun our 74-column limit:
> > <https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-
> > Standards#Column_limits>.
> > NB due to how `git log' etc. indents descriptions I prefer to stay within
> > 72 columns with my own changes for a better visual effect, though you are of
> > course free to use your own judgement here as long as you're within 74 columns.
> 
> Actually, the hard limit is 80 columns.  I've been using that, so far, and from
> looking at other files in gdb/ it looks like others were using the 80 columns limit,
> as well.

 Well other people doing it wrong is not an excuse I am afraid.  With 
source files it is at least bearable, however as I wrote `git log' etc. 
indent descriptions by 4 characters, which means that as soon as you are 
beyond 76 columns lines get wrapped making text really hard to follow.

 Take your recent commit de65820cd69a as an example.  Its description 
renders like this on my screen:

    In gdb.btrace/buffer-size.exp we explicitly ask for the BTS recording format
.
    This may lead to spurious fails on systems where PT is being used by some ot
her
    process at the same time.

    Set both PT and BTS buffer sizes to 1 and check that whatever recording form
at
    is used will use a 4KB buffer.

-- is this how you wanted it to look like?

 If you disagree, then please discuss and get a consensus on an update to 
the rules set in our wiki.  They are there for a reason and please look 
for past discussions as to why they have been set like they are now.

> >  Thanks for confirming.  Is there going to be any difference here for
> > non-x86 targets that needs to be verified?
> 
> They're supposed to work the same way.  I.e. older gdbservers won't
> advertise the packets and newer GDB's hence won't use them.  And
> newer gdbservers would advertise the packets and, with this patch,
> fail on every request with "E.Target does not support branch tracing".
> 
> Let me check that once for both directions just to be sure.

 Great, thanks!

  Maciej

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Maciej W. Rozycki]

Hi Markus,

On Wed, 28 Feb 2018, Maciej W. Rozycki wrote:

>  Well other people doing it wrong is not an excuse I am afraid.  With 
> source files it is at least bearable, however as I wrote `git log' etc. 
> indent descriptions by 4 characters, which means that as soon as you are 
> beyond 76 columns lines get wrapped making text really hard to follow.

 A follow-up to myself just for avoidance of doubt: you are indeed allowed 
to exceed the soft limit in certain circumstances, but you still need to 
have "a reasonable reason", you can't just do this on a regular basis with 
no apparent reason whatsoever.

 I wouldn't care for example if you had a full stop in column 75 in one 
line in an otherwise well formatted description.  Otherwise you need to 
justify yourself.

  Maciej

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Metzger, Markus T]

Hello Maciej,

I added

	(setq fill-column 74)

for COMMIT_EDITMSG and for *.[hc] files.  This excludes .exp and .texinfo
(and others), which are still at 80 columns.

Also for command help-text strings I believe it makes sense to stay at 80
columns.

Are you OK with that?

Regards,
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

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Metzger, Markus T]

Hello Maciej,

I wondered why my fill-column auto-mode settings didn't seem to be effective and found
that they were overwritten by gdb/.dir-locals.el.

If you want to enforce the 74 columns limit, you may want to submit the below patch.

I will leave the fill-column 74 for the commit-message but fall back to 80 columns for the rest.
IIUC you were mostly concerned about the additional 4-columns indentation by 'git log'.
Are you OK with that?

Regards,
Markus.

---

diff --git a/gdb/.dir-locals.el b/gdb/.dir-locals.el
index 7e2b0cc..abfd167 100644
--- a/gdb/.dir-locals.el
+++ b/gdb/.dir-locals.el
@@ -21,6 +21,7 @@
  (nil . ((bug-reference-url-format . "http://sourceware.org/bugzilla/show_bug.cgi?id=%s")))
  (c-mode . ((c-file-style . "GNU")
            (mode . c++)
+           (fill-column . 74)
            (indent-tabs-mode . t)
            (tab-width . 8)
            (c-basic-offset . 2)



> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Metzger, Markus T
> Sent: 28 February 2018 12:24
> To: Maciej W. Rozycki <macro@mips.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace
> target method
> 
> Hello Maciej,
> 
> I added
> 
> 	(setq fill-column 74)
> 
> for COMMIT_EDITMSG and for *.[hc] files.  This excludes .exp and .texinfo (and
> others), which are still at 80 columns.
> 
> Also for command help-text strings I believe it makes sense to stay at 80 columns.
> 
> Are you OK with that?
> 
> Regards,
> 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

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

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Maciej W. Rozycki]

Hi Markus,

> I wondered why my fill-column auto-mode settings didn't seem to be effective and found
> that they were overwritten by gdb/.dir-locals.el.
> 
> If you want to enforce the 74 columns limit, you may want to submit the below patch.

 I don't use Emacs so I can't comment on technical details of your change, 
although at the high level it does look reasonable to me.

> I will leave the fill-column 74 for the commit-message but fall back to 80 columns for the rest.
> IIUC you were mostly concerned about the additional 4-columns indentation by 'git log'.
> Are you OK with that?

 As I say we have the formatting rules set as documented in our wiki, so 
please do follow them, or if you want to get them changed, then you need 
to reach consensus about your proposal among GDB maintainers.

  Maciej

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Eli Zaretskii]

> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Wed, 28 Feb 2018 12:53:15 +0000
> 
> I wondered why my fill-column auto-mode settings didn't seem to be effective and found
> that they were overwritten by gdb/.dir-locals.el.
> 
> If you want to enforce the 74 columns limit, you may want to submit the below patch.
> 
> I will leave the fill-column 74 for the commit-message but fall back to 80 columns for the rest.
> IIUC you were mostly concerned about the additional 4-columns indentation by 'git log'.
> Are you OK with that?

I believe this value is so we could some day produce ChangeLog files
from Git log.

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Yao Qi]

"Maciej W. Rozycki" <macro@mips.com> writes:

>  Hmm, v3 of this change (apparently never posted), that is specifically 
> commit de6242d30757 ("btrace, gdbserver: remove the to_supports_btrace 
> target method"), has broken remote `mips-linux' target debugging 
> completely, that is an attempt to make a remote connection fails in the 
> initial handshake, e.g.:
>
> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 25519
> Listening on port 2346
> target remote 1.2.3.4:2346
> Remote debugging using 1.2.3.4:2346
> Reading symbols from .../lib/ld.so.1...done.
> 0x77fc8de0 in __start () from .../lib/ld.so.1
> Protocol error: qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.
> (gdb) continue
> The program is not being run.
> (gdb) FAIL: gdb.base/advance.exp: can't run to main

For the record, since non-x86 gdbserver is broken, it takes much longer
to run gdb tests on non-x86 gdbserver buildbot builders,
On Ubuntu-AArch64-native-gdbserver-m64, the time is changed from 17 mins
to 5 hrs 50 mins; 
https://gdb-build.sergiodj.net/builders/Ubuntu-AArch64-native-gdbserver-m64/builds/4141
On CentOS-ppc64be-native-gdbserver-m64, the time is changed from 48 mins
to 6 hrs 34 mins;
https://gdb-build.sergiodj.net/builders/CentOS-ppc64be-native-gdbserver-m64/builds/155

CentOS-ppc64le-native-gdbserver-m64, Fedora-ppc64le-native-gdbserver-m64 and
Debian-s390x-native-gdbserver-m64 are affected as well.

By the time Markus's fix is pushed in, these builders can run tests for
only 4 commits every day, and there are already 184 commits pushed after
commit de6242d30757.  It takes at least 46 days to build and test every
commit.  If we take "try" jobs submitted in the last several days into
account, it takes more time to clear the queue.

Sergio, in short, non-x86 gdbserver is broken, it takes several hours to
run gdb tests with gdbserver builders.  As a result, many builds are
pending there, and it still also takes much time to clean them up.
After Markus commit his fix, can we do something to let non-x86
gdbserver builders to skip these pending builds, and "jump" to Markus's fix?
Or can we temporarily disable non-x86 builders, restart them after the
fix is committed, and make sure builders pick the most recent commit
rather than resuming from the pending builds.

-- 
Yao (齐尧)

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Sergio Durigan Junior]

On Wednesday, February 28 2018, Yao Qi wrote:

> "Maciej W. Rozycki" <macro@mips.com> writes:
>
>>  Hmm, v3 of this change (apparently never posted), that is specifically 
>> commit de6242d30757 ("btrace, gdbserver: remove the to_supports_btrace 
>> target method"), has broken remote `mips-linux' target debugging 
>> completely, that is an attempt to make a remote connection fails in the 
>> initial handshake, e.g.:
>>
>> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 25519
>> Listening on port 2346
>> target remote 1.2.3.4:2346
>> Remote debugging using 1.2.3.4:2346
>> Reading symbols from .../lib/ld.so.1...done.
>> 0x77fc8de0 in __start () from .../lib/ld.so.1
>> Protocol error: qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.
>> (gdb) continue
>> The program is not being run.
>> (gdb) FAIL: gdb.base/advance.exp: can't run to main
>
> For the record, since non-x86 gdbserver is broken, it takes much longer
> to run gdb tests on non-x86 gdbserver buildbot builders,
> On Ubuntu-AArch64-native-gdbserver-m64, the time is changed from 17 mins
> to 5 hrs 50 mins; 
> https://gdb-build.sergiodj.net/builders/Ubuntu-AArch64-native-gdbserver-m64/builds/4141
> On CentOS-ppc64be-native-gdbserver-m64, the time is changed from 48 mins
> to 6 hrs 34 mins;
> https://gdb-build.sergiodj.net/builders/CentOS-ppc64be-native-gdbserver-m64/builds/155
>
> CentOS-ppc64le-native-gdbserver-m64, Fedora-ppc64le-native-gdbserver-m64 and
> Debian-s390x-native-gdbserver-m64 are affected as well.

Ah, that explains the failures I am seeing.

> By the time Markus's fix is pushed in, these builders can run tests for
> only 4 commits every day, and there are already 184 commits pushed after
> commit de6242d30757.  It takes at least 46 days to build and test every
> commit.  If we take "try" jobs submitted in the last several days into
> account, it takes more time to clear the queue.
>
> Sergio, in short, non-x86 gdbserver is broken, it takes several hours to
> run gdb tests with gdbserver builders.  As a result, many builds are
> pending there, and it still also takes much time to clean them up.
> After Markus commit his fix, can we do something to let non-x86
> gdbserver builders to skip these pending builds, and "jump" to Markus's fix?
> Or can we temporarily disable non-x86 builders, restart them after the
> fix is committed, and make sure builders pick the most recent commit
> rather than resuming from the pending builds.

What I can do is manually cancel all the build until Markus's commit.
I've done that before for other builders, so it is possible.

I'll keep an eye and do that when the commit is pushed.  Thanks for
keeping me in the loop, Yao.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Metzger, Markus T]

Hello Sergio,

> What I can do is manually cancel all the build until Markus's commit.
> I've done that before for other builders, so it is possible.

I pushed the fix.

I'm sorry about the breakage this caused and the lost test coverage for
the commits for which we will have to cancel testing.

Thanks for helping with buildbot, Sergio.

Regards,
Markus.


> -----Original Message-----
> From: Sergio Durigan Junior [mailto:sergiodj@redhat.com]
> Sent: 28 February 2018 17:27
> To: Yao Qi <qiyaoltc@gmail.com>
> Cc: Maciej W. Rozycki <macro@mips.com>; Metzger, Markus T
> <markus.t.metzger@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace
> target method
> 
> On Wednesday, February 28 2018, Yao Qi wrote:
> 
> > "Maciej W. Rozycki" <macro@mips.com> writes:
> >
> >>  Hmm, v3 of this change (apparently never posted), that is
> >> specifically commit de6242d30757 ("btrace, gdbserver: remove the
> >> to_supports_btrace target method"), has broken remote `mips-linux'
> >> target debugging completely, that is an attempt to make a remote
> >> connection fails in the initial handshake, e.g.:
> >>
> >> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created;
> >> pid = 25519 Listening on port 2346 target remote 1.2.3.4:2346 Remote
> >> debugging using 1.2.3.4:2346 Reading symbols from
> >> .../lib/ld.so.1...done.
> >> 0x77fc8de0 in __start () from .../lib/ld.so.1 Protocol error:
> >> qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.
> >> (gdb) continue
> >> The program is not being run.
> >> (gdb) FAIL: gdb.base/advance.exp: can't run to main
> >
> > For the record, since non-x86 gdbserver is broken, it takes much
> > longer to run gdb tests on non-x86 gdbserver buildbot builders, On
> > Ubuntu-AArch64-native-gdbserver-m64, the time is changed from 17 mins
> > to 5 hrs 50 mins;
> > https://gdb-build.sergiodj.net/builders/Ubuntu-AArch64-native-gdbserve
> > r-m64/builds/4141 On CentOS-ppc64be-native-gdbserver-m64, the time is
> > changed from 48 mins to 6 hrs 34 mins;
> > https://gdb-build.sergiodj.net/builders/CentOS-ppc64be-native-gdbserve
> > r-m64/builds/155
> >
> > CentOS-ppc64le-native-gdbserver-m64,
> > Fedora-ppc64le-native-gdbserver-m64 and
> > Debian-s390x-native-gdbserver-m64 are affected as well.
> 
> Ah, that explains the failures I am seeing.
> 
> > By the time Markus's fix is pushed in, these builders can run tests
> > for only 4 commits every day, and there are already 184 commits pushed
> > after commit de6242d30757.  It takes at least 46 days to build and
> > test every commit.  If we take "try" jobs submitted in the last
> > several days into account, it takes more time to clear the queue.
> >
> > Sergio, in short, non-x86 gdbserver is broken, it takes several hours
> > to run gdb tests with gdbserver builders.  As a result, many builds
> > are pending there, and it still also takes much time to clean them up.
> > After Markus commit his fix, can we do something to let non-x86
> > gdbserver builders to skip these pending builds, and "jump" to Markus's fix?
> > Or can we temporarily disable non-x86 builders, restart them after the
> > fix is committed, and make sure builders pick the most recent commit
> > rather than resuming from the pending builds.
> 
> What I can do is manually cancel all the build until Markus's commit.
> I've done that before for other builders, so it is possible.
> 
> I'll keep an eye and do that when the commit is pushed.  Thanks for keeping me
> in the loop, Yao.
> 
> --
> Sergio
> GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36 Please send
> encrypted e-mail if possible http://sergiodj.net/
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

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Sergio Durigan Junior]

On Thursday, March 01 2018, Markus T. Metzger wrote:

> Hello Sergio,
>
>> What I can do is manually cancel all the build until Markus's commit.
>> I've done that before for other builders, so it is possible.
>
> I pushed the fix.
>
> I'm sorry about the breakage this caused and the lost test coverage for
> the commits for which we will have to cancel testing.
>
> Thanks for helping with buildbot, Sergio.

Hey Markus,

Thanks for letting me know.  I'm cancelling the pending builds on a few
builders that were affected by this issue.  Let's keep an eye and see if
there's anything else I should do :-).

Cheers,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Yao Qi]

Sergio Durigan Junior <sergiodj@redhat.com> writes:

> I'm cancelling the pending builds on a few
> builders that were affected by this issue.  Let's keep an eye and see if
> there's anything else I should do :-).

Hi Sergio,
Did you cancel pending builds?  There are still many pending builds on
some non-x86 gdbserver builders.

-- 
Yao (齐尧)

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Sergio Durigan Junior]

On Monday, March 05 2018, Yao Qi wrote:

> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
>> I'm cancelling the pending builds on a few
>> builders that were affected by this issue.  Let's keep an eye and see if
>> there's anything else I should do :-).
>
> Hi Sergio,
> Did you cancel pending builds?  There are still many pending builds on
> some non-x86 gdbserver builders.

Hi Yao,

I didn't cancel the pending builds of the ppc{be,le} builders, because I
remember seeing them even before this regression.  I will do that now.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Yao Qi]

On Mon, Mar 5, 2018 at 4:13 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
>
> I didn't cancel the pending builds of the ppc{be,le} builders, because I
> remember seeing them even before this regression.  I will do that now.
>

Can you also cancel the pending builds on Ubuntu-AArch64-native-gdbserver-m64?

-- 
Yao (齐尧)

Re: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Sergio Durigan Junior]

On Tuesday, March 06 2018, Yao Qi wrote:

> On Mon, Mar 5, 2018 at 4:13 PM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>>
>> I didn't cancel the pending builds of the ppc{be,le} builders, because I
>> remember seeing them even before this regression.  I will do that now.
>>
>
> Can you also cancel the pending builds on Ubuntu-AArch64-native-gdbserver-m64?

Done.  I'm monitoring Ubuntu-AArch64-m64 to see if it will be able to
catch up, otherwise I'll cancel its pending builds as well.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

RE: [PATCH v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method

[Maciej W. Rozycki]

Hi Markus,

> > What I can do is manually cancel all the build until Markus's commit.
> > I've done that before for other builders, so it is possible.
> 
> I pushed the fix.

 For the record, my test board has been restored to normal operation and I 
have now finished regression-testing with the `mips-mti-linux-gnu' remote 
target and its three ABIs, with no problems observed compared to results 
as at commit de6242d30757^.

 Thank you for getting the fix completed so quickly.

  Maciej

[PATCH v2 7/7] btrace: check perf_event_paranoid

[Markus Metzger]

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-26  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 4425539..c0b031f 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -409,6 +409,34 @@ cpu_supports_bts (void)
     }
 }
 
+/* The perf_event_open syscall failed.  Try to print a helpful error
+   message.  */
+
+static void
+diagnose_perf_event_open_fail ()
+{
+  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 *
@@ -448,7 +476,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
   errno = 0;
   scoped_fd fd (syscall (SYS_perf_event_open, &bts->attr, pid, -1, -1, 0));
   if (fd.get () < 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
@@ -578,7 +606,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   errno = 0;
   scoped_fd fd (syscall (SYS_perf_event_open, &pt->attr, pid, -1, -1, 0));
   if (fd.get () < 0)
-    error (_("Failed to start recording: %s"), safe_strerror (errno));
+    diagnose_perf_event_open_fail ();
 
   /* Allocate the configuration page. */
   scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
-- 
1.8.3.1

[PATCH v2 4/7] btrace, gdbserver: use exceptions to convey btrace enable/disable errors

[Markus Metzger]

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-26  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, &current_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, &current_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 354094f..2b37e41 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -909,6 +909,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

[PATCH v2 3/7] btrace: prepare for throwing exceptions when enabling btrace

[Markus Metzger]

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-26  Markus Metzger  <markus.t.metzger@intel.com>

gdb/
	* nat/linux-btrace.c: Include scoped_fd.h and scoped_mmap.h.
	(perf_event_pt_event_type): Use gdb_file_up.
	(linux_enable_bts, linux_enable_pt): Use gdb::unique_xmalloc_ptr,
	scoped_fd, and scoped_mmap.
---
 gdb/nat/linux-btrace.c | 118 +++++++++++++++++++++----------------------------
 1 file changed, 51 insertions(+), 67 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index bbd0fe6..354094f 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -25,6 +25,8 @@
 #include "gdb_wait.h"
 #include "x86-cpuid.h"
 #include "filestuff.h"
+#include "common/scoped_fd.h"
+#include "common/scoped_mmap.h"
 
 #include <inttypes.h>
 
@@ -179,17 +181,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 == nullptr)
     return -1;
 
-  found = fscanf (file, "%d", type);
-
-  fclose (file);
-
+  int found = fscanf (file.get (), "%d", type);
   if (found == 1)
     return 0;
   return -1;
@@ -662,14 +659,13 @@ linux_supports_btrace (struct target_ops *ops, enum btrace_format 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 +688,9 @@ 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_fd fd (syscall (SYS_perf_event_open, &bts->attr, pid, -1, -1, 0));
+  if (fd.get () < 0)
+    return nullptr;
 
   /* Convert the requested size in bytes to pages (rounding up).  */
   pages = ((size_t) conf->size / PAGE_SIZE
@@ -711,6 +707,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 data;
   for (; pages > 0; pages >>= 1)
     {
       size_t length;
@@ -730,15 +727,16 @@ 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));
-      if (header != MAP_FAILED)
+      data.reset (nullptr, length, PROT_READ, MAP_SHARED, fd.get (), 0);
+      if (data.get () != MAP_FAILED)
 	break;
     }
 
   if (pages == 0)
-    goto err_file;
+    return nullptr;
 
+  struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
+    data.get ();
   data_offset = PAGE_SIZE;
 
 #if defined (PERF_ATTR_SIZE_VER5)
@@ -753,29 +751,21 @@ 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 = (const uint8_t *) data.get () + data_offset;
   bts->bts.last_head = 0ull;
+  bts->header = header;
+  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);
+  data.release ();
 
- err_out:
-  xfree (tinfo);
-  return NULL;
+  tinfo->conf.bts.size = (unsigned int) size;
+  return tinfo.release ();
 }
 
 #if defined (PERF_ATTR_SIZE_VER5)
@@ -785,10 +775,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 +790,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 +805,18 @@ 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_fd fd (syscall (SYS_perf_event_open, &pt->attr, pid, -1, -1, 0));
+  if (fd.get () < 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));
-  if (header == MAP_FAILED)
-    goto err_file;
+  scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+		    fd.get (), 0);
+  if (data.get () == MAP_FAILED)
+    return nullptr;
+
+  struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
+    data.get ();
 
   header->aux_offset = header->data_offset + header->data_size;
 
@@ -844,6 +835,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 aux;
   for (; pages > 0; pages >>= 1)
     {
       size_t length;
@@ -855,41 +847,33 @@ 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 (nullptr, length, PROT_READ, MAP_SHARED, fd.get (),
+		 header->aux_offset);
+      if (aux.get () != 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 = (const uint8_t *) aux.release ();
   pt->pt.data_head = &header->aux_head;
+  pt->header = header;
+  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);
+  data.release ();
 
- 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

Re: [PATCH v2 0/7] improve btrace enable error reporting

[Pedro Alves]

Hi Markus,

On 01/26/2018 02:14 PM, Markus Metzger wrote:
> Recording may fail for a variety of reasons.  Improve the error
> messages by stating more clearly what operation failed and try to give
> a reason why it failed.
> 
> Further align the error messages for native and remote debugging.
> 
> Changes to v1:
>   - move helper classes into gdb/common/
>   - add unit tests for helpers
>   - simplify helpers
> 
> Markus Metzger (7):
>   common: add scoped_fd
>   common: add scoped_mmap
>   btrace: prepare for throwing exceptions when enabling btrace
>   btrace, gdbserver: use exceptions to convey btrace enable/disable
>     errors
>   btrace, gdbserver: remove the to_supports_btrace target method
>   btrace: improve enable error messages
>   btrace: check perf_event_paranoid

This LGTM, though I have a couple questions, and a nit.

#1 - Where does this leave up wrt to:

  'old gdb' x 'new gdbserver'
and
  'new gdb' x 'old gdbserver'
 
?


#2 - Where we now say

  +  error (_("GDB does not support Intel PT."));

(and similarly for BTS)

shouldn't that say something like "_This_ GDB does not",
so that the user can tell that it's a matter of that
particular build of gdb, not that GDB-the-project is lacking
support for PT?


#3 - in patch 7:

Instead of:

  const char *filename = "/proc/sys/kernel/perf_event_paranoid";

write:

  static const char filename[] = ...;

Thanks,
Pedro Alves

RE: [PATCH v2 0/7] improve btrace enable error reporting

[Metzger, Markus T]

Hello Pedro,

> This LGTM, though I have a couple questions, and a nit.
> 
> #1 - Where does this leave up wrt to:
> 
>   'old gdb' x 'new gdbserver'
> and
>   'new gdb' x 'old gdbserver'
> 
> ?

An old gdbserver would still produce the old error responses.  They would be turned
into errors on the GDB side in remote.c both for old and new GDB.  No change.

Removing remote_supports_btrace in a new GDB will defer the packet availability check
to the individual functions.  The "Target does not support branch tracing" error will now
come from record_enable_btrace() instead of btrace_enble().  The old gdbserver still does
the initial availability check and will not announce the respective enabling packets.  No
user-visible change.

A new gdbserver will produce the new error messages and will always announce btrace
packets in qSupported.  An old GDB will hence always ask to enable branch tracing and
the new gdbserver will always try and report the reason using the new error messages.
This will look like a new GDB.

 
> #2 - Where we now say
> 
>   +  error (_("GDB does not support Intel PT."));
> 
> (and similarly for BTS)
> 
> shouldn't that say something like "_This_ GDB does not", so that the user can tell
> that it's a matter of that particular build of gdb, not that GDB-the-project is lacking
> support for PT?

I thought we meant GDB in errors to always refer to this instance of GDB.  There would
hardly be an error about missing PT support if the GDB project didn't know about it.

I don't mind changing the error string.  Would "This GDB" be the correct wording?  Or
should we refer to the configuration and say something like "GDB has not been configured
to support ..." or "GDB has been built without support for ..."?

Both are substantially longer and not more helpful IMHO, even though they describe
more accurately what's wrong.  The term "This GDB" would refer to this particular GDB
executable more clearly but I'm not sure whether this would be more helpful, either.

 
> #3 - in patch 7:
> 
> Instead of:
> 
>   const char *filename = "/proc/sys/kernel/perf_event_paranoid";
> 
> write:
> 
>   static const char filename[] = ...;

Regarding that last patch, it is checking a Debian-specific kernel feature.  The upstream
kernel only knows levels -1, 0, 1, and 2.  Even setting the perf_event_paranoid level to 3
wouldn't cause any issues.

What is our policy regarding this?  Do we accept such distribution-specific checks into
upstream GDB or do we expect the Debian maintainers to maintain this patch on top
of upstream GDB?


Regards,
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

Re: [PATCH v2 0/7] improve btrace enable error reporting

[Pedro Alves]

On 02/07/2018 10:41 AM, Metzger, Markus T wrote:
> Hello Pedro,
> 
>> This LGTM, though I have a couple questions, and a nit.
>>
>> #1 - Where does this leave up wrt to:
>>
>>   'old gdb' x 'new gdbserver'
>> and
>>   'new gdb' x 'old gdbserver'
>>
>> ?
> 
> An old gdbserver would still produce the old error responses.  They would be turned
> into errors on the GDB side in remote.c both for old and new GDB.  No change.
> 
> Removing remote_supports_btrace in a new GDB will defer the packet availability check
> to the individual functions.  The "Target does not support branch tracing" error will now
> come from record_enable_btrace() instead of btrace_enble().  The old gdbserver still does
> the initial availability check and will not announce the respective enabling packets.  No
> user-visible change.
> 
> A new gdbserver will produce the new error messages and will always announce btrace
> packets in qSupported.  An old GDB will hence always ask to enable branch tracing and
> the new gdbserver will always try and report the reason using the new error messages.
> This will look like a new GDB.

Great, thanks.  I'd think it a good idea to copy/paste that to the relevant
patch's git log.

> 
>  
>> #2 - Where we now say
>>
>>   +  error (_("GDB does not support Intel PT."));
>>
>> (and similarly for BTS)
>>
>> shouldn't that say something like "_This_ GDB does not", so that the user can tell
>> that it's a matter of that particular build of gdb, not that GDB-the-project is lacking
>> support for PT?
> 
> I thought we meant GDB in errors to always refer to this instance of GDB.  There would
> hardly be an error about missing PT support if the GDB project didn't know about it.

GDB can know about PT but not have support implemented, because no one wrote
the support.  The error message "GDB does not support Intel PT" to me sounds
like what we'd say if we had written some initial support in form of
commands that do nothing.   How can the user tell the difference to
"gdb supports PT, but you need to build it against a newer version of some library,
or something like that."

> 
> I don't mind changing the error string.  Would "This GDB" be the correct wording?  Or
> should we refer to the configuration and say something like "GDB has not been configured
> to support ..." or "GDB has been built without support for ..."?
> 
> Both are substantially longer and not more helpful IMHO, even though they describe
> more accurately what's wrong.  The term "This GDB" would refer to this particular GDB
> executable more clearly but I'm not sure whether this would be more helpful, either.

Yeah "This GDB" was a straw man proposal, something to get the discussion started
to maybe find some better warning.

Off hand, I recall that in the no-expat cases, we say things like this:

      warning (_("Can not parse XML trace frame info; XML support "
                 "was disabled at compile time"));

      warning (_("Can not parse XML memory map; XML support was disabled "
                 "at compile time"));

      warning (_("Can not parse XML OS data; XML support was disabled "
                "at compile time"));

So maybe something around

  "Cannot do X; Intel PT support disabled at compile time."


> 
>  
>> #3 - in patch 7:
>>
>> Instead of:
>>
>>   const char *filename = "/proc/sys/kernel/perf_event_paranoid";
>>
>> write:
>>
>>   static const char filename[] = ...;
> 
> Regarding that last patch, it is checking a Debian-specific kernel feature.  The upstream
> kernel only knows levels -1, 0, 1, and 2.  Even setting the perf_event_paranoid level to 3
> wouldn't cause any issues.
> 
> What is our policy regarding this?  Do we accept such distribution-specific checks into
> upstream GDB or do we expect the Debian maintainers to maintain this patch on top
> of upstream GDB?

I don't think we have a written down policy.  

I don't mind having it in master.  It helps users/developers running
Debian and derivatives, and is not invasive.

Thanks,
Pedro Alves

RE: [PATCH v2 0/7] improve btrace enable error reporting

[Metzger, Markus T]

Hello Pedro,

Thanks for your comments.

> Great, thanks.  I'd think it a good idea to copy/paste that to the relevant patch's
> git log.


commit 41df50232209c74f3465556c9cfa690e16ee63ca
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Fri Jan 19 09:41:42 2018 +0100

    btrace, gdbserver: use exceptions to convey btrace enable/disable errors
    
    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.
    
    We're still using error strings on the RSP level.  This patch does not affect
    the interoperability of older/newer GDB/gdbserver.


commit 54ab396195eee25b4045a2fc5226e73c508dc5a6
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Fri Jan 19 14:32:09 2018 +0100

    btrace, gdbserver: remove the to_supports_btrace target method
    
    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.
    
    For remote debugging, gdbserver will now always advertise branch tracing related
    packets.  When talking to an older GDB, this will cause GDB to try to enable
    branch tracing and gdbserver to report a suitable error message every time.
    
    An older gdbserver will not advertise branch tracing related packets if the
    one-time check failed, so a newer GDB with this patch will fail to enable branch
    tracing at remote_enable_btrace() rather than at btrace_enable().  The error
    message is the same in both cases so there should be no user-visible change.

 
> Off hand, I recall that in the no-expat cases, we say things like this:
> 
>       warning (_("Can not parse XML trace frame info; XML support "
>                  "was disabled at compile time"));
> 
>       warning (_("Can not parse XML memory map; XML support was disabled "
>                  "at compile time"));
> 
>       warning (_("Can not parse XML OS data; XML support was disabled "
>                 "at compile time"));
> 
> So maybe something around
> 
>   "Cannot do X; Intel PT support disabled at compile time."

X is always 'enable branch tracing' in our case.  But the reason for the error may
be different:
- libipt missing at build time
- kernel headers missing or too old at build time

Those reasons are not relevant for the user, though.  There are already config
warnings for those so if GDB is configured with --with-intel-pt=yes, the build
should fail with an appropriate diagnostic.  I'd omit it in the error message.

So you're suggesting to change

    "GDB does not support Intel Processor Trace."

into

    "Intel Processor Trace support was disabled at compile time."

Correct?

We're already using the above in a warning in remote.c.  I'd change that, as well,
to be consistent with the new wording.

OK?


> >> #3 - in patch 7:
> >>
> >> Instead of:
> >>
> >>   const char *filename = "/proc/sys/kernel/perf_event_paranoid";
> >>
> >> write:
> >>
> >>   static const char filename[] = ...;
> >
> > Regarding that last patch, it is checking a Debian-specific kernel
> > feature.  The upstream kernel only knows levels -1, 0, 1, and 2.  Even
> > setting the perf_event_paranoid level to 3 wouldn't cause any issues.
> >
> > What is our policy regarding this?  Do we accept such
> > distribution-specific checks into upstream GDB or do we expect the
> > Debian maintainers to maintain this patch on top of upstream GDB?
> 
> I don't think we have a written down policy.
> 
> I don't mind having it in master.  It helps users/developers running Debian and
> derivatives, and is not invasive.

The error message might be misleading on other systems but it is not very likely
that perf_event_paranoid is set to another value than those supported by that
system.

If upstream introduces a level 3, we may need to remove this diagnostic again.

So I'll leave the patch in and correct the declaration of FILENAME.  I found one
more occurrence in this series.

Regards,
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

Re: [PATCH v2 0/7] improve btrace enable error reporting

[Pedro Alves]

On 02/08/2018 10:40 AM, Metzger, Markus T wrote:

>> Great, thanks.  I'd think it a good idea to copy/paste that to the relevant patch's
>> git log.
> 
> [snip new logs]

Thanks.

> So you're suggesting to change
> 
>     "GDB does not support Intel Processor Trace."
> 
> into
> 
>     "Intel Processor Trace support was disabled at compile time."
> 
> Correct?

Correct.

> 
> We're already using the above in a warning in remote.c.  I'd change that, as well,
> to be consistent with the new wording.
> 
> OK?

OK.

Thanks,
Pedro Alves

RE: [PATCH v2 0/7] improve btrace enable error reporting

[Metzger, Markus T]

Hello Pedro, Eli,

Here's the follow-up patch to change a few existing error messages.

> > So you're suggesting to change
> >
> >     "GDB does not support Intel Processor Trace."
> >
> > into
> >
> >     "Intel Processor Trace support was disabled at compile time."
> >
> > Correct?
> 
> Correct.
> 
> >
> > We're already using the above in a warning in remote.c.  I'd change
> > that, as well, to be consistent with the new wording.
> >

commit d635c0d842f95f9f76562a132dcabd9330ff6455
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Thu Feb 8 14:35:44 2018 +0100

    btrace: reword error messages
    
    Reword some btrace error messages to align with the format discussed in
    https://sourceware.org/ml/gdb-patches/2018-02/msg00135.html.
    
    2018-02-08  Markus Metzger  <markus.t.metzger@intel.com>
    
    gdb/
        * remote.c (remote_btrace_maybe_reopen): Change error message.
        * btrace.c (btrace_enable): Likewise.
        (parse_xml_btrace): Likewise.
        (parse_xml_btrace_conf): Likewise.
    
diff --git a/gdb/btrace.c b/gdb/btrace.c
index 2b031a4..158d03c 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1579,7 +1579,7 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
 
 #if !defined (HAVE_LIBIPT)
   if (conf->format == BTRACE_FORMAT_PT)
-    error (_("GDB does not support Intel Processor Trace."));
+    error (_("Intel Processor Trace support was disabled at compile time."));
 #endif /* !defined (HAVE_LIBIPT) */
 
   DEBUG ("enable thread %s (%s)", print_thread_id (tp),
@@ -2218,7 +2218,8 @@ parse_xml_btrace (struct btrace_data *btrace, const char *buffer)
 
 #else  /* !defined (HAVE_LIBEXPAT) */
 
-  error (_("Cannot process branch trace.  XML parsing is not supported."));
+  error (_("Cannot process branch trace.  XML support was disabled at "
+          "compile time."));
 
 #endif  /* !defined (HAVE_LIBEXPAT) */
 }
@@ -2312,7 +2313,8 @@ parse_xml_btrace_conf (struct btrace_config *conf, const char *xml)
 
 #else  /* !defined (HAVE_LIBEXPAT) */
 
-  error (_("XML parsing is not supported."));
+  error (_("Cannot process the branch trace configuration.  XML support "
+          "was disabled at compile time."));
 
 #endif  /* !defined (HAVE_LIBEXPAT) */
 }
diff --git a/gdb/remote.c b/gdb/remote.c
index e5680f0..15d6c5b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -13193,8 +13193,8 @@ remote_btrace_maybe_reopen (void)
          if (!warned)
            {
              warned = 1;
-             warning (_("GDB does not support Intel Processor Trace. "
-                        "\"record\" will not work in this session."));
+             warning (_("Target is recording using Intel Processor Trace "
+                        "but support was disabled at compile time."));
            }
 
          continue;

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

Re: [PATCH v2 0/7] improve btrace enable error reporting

[Pedro Alves]

On 02/08/2018 01:49 PM, Metzger, Markus T wrote:
> Hello Pedro, Eli,
> 
> Here's the follow-up patch to change a few existing error messages.
> 
>>> So you're suggesting to change
>>>
>>>     "GDB does not support Intel Processor Trace."
>>>
>>> into
>>>
>>>     "Intel Processor Trace support was disabled at compile time."
>>>
>>> Correct?
>>
>> Correct.
>>
>>>
>>> We're already using the above in a warning in remote.c.  I'd change
>>> that, as well, to be consistent with the new wording.
>>>
> 

Very nice.  Thanks!

Pedro Alves