[PATCH 0/5] Remove support for old FreeBSD hosts

[PATCH 0/5] Remove support for old FreeBSD hosts

[John Baldwin]

This patch series aims to remove some cruft from the FreeBSD native
target that isn't needed on modern systems.  Most of the changes
remove support for systems released more than 10 years ago.  The last
change is closer to 9 years ago, but any such systems already fail to
compile the native target today (and for several recent releases).
I've yet to receive a single complaint either here or on FreeBSD lists
about such breakage.

The kinfo_get_file change could perhaps use further refinement.  We
need a configure check to pick up the correct library (libutil on
FreeBSD and libutil-freebsd on GNU/kFreeBSD).  However, the
kinfo_get_file and kinfo_get_vmmap functions were added to this
library at the same time (same SVN commit in FreeBSD), so checking for
both is redundant.  Right now we check for kinfo_get_file in
gdbsupport since filestuff.c uses it if present.  We could remove the
check for kinfo_get_vmmap entirely from gdb/configure.ac if we think
the kinfo_get_file check in gdbsupport/common.m4 is sufficient.  If
so, I might need to at least move the comment from configure.ac over
to common.m4 with some modifications.

John Baldwin (5): Assume that PT_LWPINFO is always defined on FreeBSD
  hosts.  Assume KERN_PROC_PATHNAME is present on FreeBSD hosts.
  Assume FreeBSD hosts include support for fetching signal
  information.  Require kinfo_get_file and kinfo_get_vmmap for FreeBSD
  hosts.  Assume FreeBSD kernels always report exec events.

 gdb/ChangeLog  |  31 +++++++++++
 gdb/fbsd-nat.c | 139 +------------------------------------------------
 gdb/fbsd-nat.h |   4 --
 3 files changed, 32 insertions(+), 142 deletions(-)

-- 
2.25.1

[PATCH 1/5] Assume that PT_LWPINFO is always defined on FreeBSD hosts.

[John Baldwin]

FreeBSD kernels have included support for this since 5.0 release.
The most recent release without support is 4.11 which was released
in January of 2005.

gdb/ChangeLog:

	* fbsd-nat.c: Assume PT_LWPINFO is always defined.
	* fbsd-nat.h: Likewise.
---
 gdb/ChangeLog  | 5 +++++
 gdb/fbsd-nat.c | 4 ----
 gdb/fbsd-nat.h | 2 --
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fad4608002..687d9aede5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-07-20  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c: Assume PT_LWPINFO is always defined.
+	* fbsd-nat.h: Likewise.
+
 2020-07-20  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-tdep.c (fbsd_skip_solib_resolver): New function.
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index a355ebe438..fc7136a97c 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -831,7 +831,6 @@ fbsd_nat_target::xfer_partial (enum target_object object,
     }
 }
 
-#ifdef PT_LWPINFO
 static bool debug_fbsd_lwp;
 static bool debug_fbsd_nat;
 
@@ -1667,7 +1666,6 @@ fbsd_nat_target::set_syscall_catchpoint (int pid, bool needed,
   return 0;
 }
 #endif
-#endif
 
 bool
 fbsd_nat_target::supports_multi_process ()
@@ -1679,7 +1677,6 @@ void _initialize_fbsd_nat ();
 void
 _initialize_fbsd_nat ()
 {
-#ifdef PT_LWPINFO
   add_setshow_boolean_cmd ("fbsd-lwp", class_maintenance,
 			   &debug_fbsd_lwp, _("\
 Set debugging of FreeBSD lwp module."), _("\
@@ -1696,5 +1693,4 @@ Enables printf debugging output."),
 			   NULL,
 			   &show_fbsd_nat_debug,
 			   &setdebuglist, &showdebuglist);
-#endif
 }
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index b5a62b9212..b49bf8cbaf 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -49,7 +49,6 @@ class fbsd_nat_target : public inf_ptrace_target
 					ULONGEST offset, ULONGEST len,
 					ULONGEST *xfered_len) override;
 
-#ifdef PT_LWPINFO
   bool thread_alive (ptid_t ptid) override;
   std::string pid_to_str (ptid_t) override;
 
@@ -93,7 +92,6 @@ class fbsd_nat_target : public inf_ptrace_target
   int set_syscall_catchpoint (int, bool, int, gdb::array_view<const int>)
     override;
 #endif
-#endif /* PT_LWPINFO */
 
   bool supports_multi_process () override;
 };
-- 
2.25.1

[PATCH 2/5] Assume KERN_PROC_PATHNAME is present on FreeBSD hosts.

[John Baldwin]

FreeBSD kernels have included this sysctl since 6.0 release.  The most
recent release without support is 5.5 which was released in May of
2006.

gdb/ChangeLog:

	* fbsd-nat.c (fbsd_nat_target::pid_to_exec_file): Always use
	sysctl and remove procfs fallback.
---
 gdb/ChangeLog  |  5 +++++
 gdb/fbsd-nat.c | 13 -------------
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 687d9aede5..0339083f1a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-07-20  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c (fbsd_nat_target::pid_to_exec_file): Always use
+	sysctl and remove procfs fallback.
+
 2020-07-20  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c: Assume PT_LWPINFO is always defined.
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index fc7136a97c..6193e0fbde 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -53,11 +53,7 @@
 char *
 fbsd_nat_target::pid_to_exec_file (int pid)
 {
-  ssize_t len;
   static char buf[PATH_MAX];
-  char name[PATH_MAX];
-
-#ifdef KERN_PROC_PATHNAME
   size_t buflen;
   int mib[4];
 
@@ -71,15 +67,6 @@ fbsd_nat_target::pid_to_exec_file (int pid)
        for processes without an associated executable such as kernel
        processes.  */
     return buflen == 0 ? NULL : buf;
-#endif
-
-  xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
-  len = readlink (name, buf, PATH_MAX - 1);
-  if (len != -1)
-    {
-      buf[len] = '\0';
-      return buf;
-    }
 
   return NULL;
 }
-- 
2.25.1

[PATCH 3/5] Assume FreeBSD hosts include support for fetching signal information.

[John Baldwin]

The current layout of siginfo_t and support for fetching it has been
included in FreeBSD kernels since 7.0 release.  The most recent
release without support is 6.4 released in November of 2008.

gdb/ChangeLog:

	* fbsd-nat.c: Always include support for
	TARGET_OBJECT_SIGNAL_INFO.
---
 gdb/ChangeLog  |  5 +++++
 gdb/fbsd-nat.c | 14 --------------
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0339083f1a..da6406a1f7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-07-20  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c: Always include support for
+	TARGET_OBJECT_SIGNAL_INFO.
+
 2020-07-20  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c (fbsd_nat_target::pid_to_exec_file): Always use
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 6193e0fbde..aa5d9ccd12 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -527,17 +527,6 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
   return true;
 }
 
-/*
- * The current layout of siginfo_t on FreeBSD was adopted in SVN
- * revision 153154 which shipped in FreeBSD versions 7.0 and later.
- * Don't bother supporting the older layout on older kernels.  The
- * older format was also never used in core dump notes.
- */
-#if __FreeBSD_version >= 700009
-#define USE_SIGINFO
-#endif
-
-#ifdef USE_SIGINFO
 /* Return the size of siginfo for the current inferior.  */
 
 #ifdef __LP64__
@@ -664,7 +653,6 @@ fbsd_convert_siginfo (siginfo_t *si)
   memcpy(si, &si32, sizeof (si32));
 #endif
 }
-#endif
 
 /* Implement the "xfer_partial" target_ops method.  */
 
@@ -679,7 +667,6 @@ fbsd_nat_target::xfer_partial (enum target_object object,
 
   switch (object)
     {
-#ifdef USE_SIGINFO
     case TARGET_OBJECT_SIGNAL_INFO:
       {
 	struct ptrace_lwpinfo pl;
@@ -710,7 +697,6 @@ fbsd_nat_target::xfer_partial (enum target_object object,
 	*xfered_len = len;
 	return TARGET_XFER_OK;
       }
-#endif
 #ifdef KERN_PROC_AUXV
     case TARGET_OBJECT_AUXV:
       {
-- 
2.25.1

[PATCH 4/5] Require kinfo_get_file and kinfo_get_vmmap for FreeBSD hosts.

[John Baldwin]

FreeBSD systems have provided these functions in libutil since 7.1
release.  The most recent release without support is 6.4 released in
November of 2008.

This also requires libutil-freebsd on GNU/kFreeBSD systems.  I assume
that those systems have supported kinfo_get_file and kinfo_get_vmmap
over a similar timeframe.

gdb/ChangeLog:

	* fbsd-nat.c (fbsd_read_mapping): Remove
	(fbsd_nat_target::find_memory_regions): Remove the procfs version.
	(fbsd_nat_target::info_proc): Assume kinfo_getfile() and
	kinfo_get_vmmap() are always present.
---
 gdb/ChangeLog  |   7 ++++
 gdb/fbsd-nat.c | 101 -------------------------------------------------
 2 files changed, 7 insertions(+), 101 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index da6406a1f7..8201b357a5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2020-07-20  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c (fbsd_read_mapping): Remove
+	(fbsd_nat_target::find_memory_regions): Remove the procfs version.
+	(fbsd_nat_target::info_proc): Assume kinfo_getfile() and
+	kinfo_get_vmmap() are always present.
+
 2020-07-20  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c: Always include support for
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index aa5d9ccd12..ac88fbc87f 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -34,12 +34,7 @@
 #include <sys/signal.h>
 #include <sys/sysctl.h>
 #include <sys/user.h>
-#if defined(HAVE_KINFO_GETFILE) || defined(HAVE_KINFO_GETVMMAP)
 #include <libutil.h>
-#endif
-#if !defined(HAVE_KINFO_GETVMMAP)
-#include "gdbsupport/filestuff.h"
-#endif
 
 #include "elf-bfd.h"
 #include "fbsd-nat.h"
@@ -71,7 +66,6 @@ fbsd_nat_target::pid_to_exec_file (int pid)
   return NULL;
 }
 
-#ifdef HAVE_KINFO_GETVMMAP
 /* Iterate over all the memory regions in the current inferior,
    calling FUNC for each memory region.  DATA is passed as the last
    argument to FUNC.  */
@@ -124,77 +118,6 @@ fbsd_nat_target::find_memory_regions (find_memory_region_ftype func,
     }
   return 0;
 }
-#else
-static int
-fbsd_read_mapping (FILE *mapfile, unsigned long *start, unsigned long *end,
-		   char *protection)
-{
-  /* FreeBSD 5.1-RELEASE uses a 256-byte buffer.  */
-  char buf[256];
-  int resident, privateresident;
-  unsigned long obj;
-  int ret = EOF;
-
-  /* As of FreeBSD 5.0-RELEASE, the layout is described in
-     /usr/src/sys/fs/procfs/procfs_map.c.  Somewhere in 5.1-CURRENT a
-     new column was added to the procfs map.  Therefore we can't use
-     fscanf since we need to support older releases too.  */
-  if (fgets (buf, sizeof buf, mapfile) != NULL)
-    ret = sscanf (buf, "%lx %lx %d %d %lx %s", start, end,
-		  &resident, &privateresident, &obj, protection);
-
-  return (ret != 0 && ret != EOF);
-}
-
-/* Iterate over all the memory regions in the current inferior,
-   calling FUNC for each memory region.  DATA is passed as the last
-   argument to FUNC.  */
-
-int
-fbsd_nat_target::find_memory_regions (find_memory_region_ftype func,
-				      void *data)
-{
-  pid_t pid = inferior_ptid.pid ();
-  unsigned long start, end, size;
-  char protection[4];
-  int read, write, exec;
-
-  std::string mapfilename = string_printf ("/proc/%ld/map", (long) pid);
-  gdb_file_up mapfile (fopen (mapfilename.c_str (), "r"));
-  if (mapfile == NULL)
-    error (_("Couldn't open %s."), mapfilename.c_str ());
-
-  if (info_verbose)
-    fprintf_filtered (gdb_stdout, 
-		      "Reading memory regions from %s\n", mapfilename.c_str ());
-
-  /* Now iterate until end-of-file.  */
-  while (fbsd_read_mapping (mapfile.get (), &start, &end, &protection[0]))
-    {
-      size = end - start;
-
-      read = (strchr (protection, 'r') != 0);
-      write = (strchr (protection, 'w') != 0);
-      exec = (strchr (protection, 'x') != 0);
-
-      if (info_verbose)
-	{
-	  fprintf_filtered (gdb_stdout, 
-			    "Save segment, %ld bytes at %s (%c%c%c)\n",
-			    size, paddress (target_gdbarch (), start),
-			    read ? 'r' : '-',
-			    write ? 'w' : '-',
-			    exec ? 'x' : '-');
-	}
-
-      /* Invoke the callback function to create the corefile segment.
-	 Pass MODIFIED as true, we do not know the real modification state.  */
-      func (start, size, read, write, exec, 1, data);
-    }
-
-  return 0;
-}
-#endif
 
 /* Fetch the command line for a running process.  */
 
@@ -251,21 +174,15 @@ fbsd_fetch_kinfo_proc (pid_t pid, struct kinfo_proc *kp)
 bool
 fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
 {
-#ifdef HAVE_KINFO_GETFILE
   gdb::unique_xmalloc_ptr<struct kinfo_file> fdtbl;
   int nfd = 0;
-#endif
   struct kinfo_proc kp;
   pid_t pid;
   bool do_cmdline = false;
   bool do_cwd = false;
   bool do_exe = false;
-#ifdef HAVE_KINFO_GETFILE
   bool do_files = false;
-#endif
-#ifdef HAVE_KINFO_GETVMMAP
   bool do_mappings = false;
-#endif
   bool do_status = false;
 
   switch (what)
@@ -275,11 +192,9 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
       do_cwd = true;
       do_exe = true;
       break;
-#ifdef HAVE_KINFO_GETVMMAP
     case IP_MAPPINGS:
       do_mappings = true;
       break;
-#endif
     case IP_STATUS:
     case IP_STAT:
       do_status = true;
@@ -293,21 +208,15 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
     case IP_CWD:
       do_cwd = true;
       break;
-#ifdef HAVE_KINFO_GETFILE
     case IP_FILES:
       do_files = true;
       break;
-#endif
     case IP_ALL:
       do_cmdline = true;
       do_cwd = true;
       do_exe = true;
-#ifdef HAVE_KINFO_GETFILE
       do_files = true;
-#endif
-#ifdef HAVE_KINFO_GETVMMAP
       do_mappings = true;
-#endif
       do_status = true;
       break;
     default:
@@ -327,10 +236,8 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
     error (_("Invalid arguments."));
 
   printf_filtered (_("process %d\n"), pid);
-#ifdef HAVE_KINFO_GETFILE
   if (do_cwd || do_exe || do_files)
     fdtbl.reset (kinfo_getfile (pid, &nfd));
-#endif
 
   if (do_cmdline)
     {
@@ -343,7 +250,6 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
   if (do_cwd)
     {
       const char *cwd = NULL;
-#ifdef HAVE_KINFO_GETFILE
       struct kinfo_file *kf = fdtbl.get ();
       for (int i = 0; i < nfd; i++, kf++)
 	{
@@ -353,7 +259,6 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
 	      break;
 	    }
 	}
-#endif
       if (cwd != NULL)
 	printf_filtered ("cwd = '%s'\n", cwd);
       else
@@ -362,7 +267,6 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
   if (do_exe)
     {
       const char *exe = NULL;
-#ifdef HAVE_KINFO_GETFILE
       struct kinfo_file *kf = fdtbl.get ();
       for (int i = 0; i < nfd; i++, kf++)
 	{
@@ -372,7 +276,6 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
 	      break;
 	    }
 	}
-#endif
       if (exe == NULL)
 	exe = pid_to_exec_file (pid);
       if (exe != NULL)
@@ -380,7 +283,6 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
       else
 	warning (_("unable to fetch executable path name"));
     }
-#ifdef HAVE_KINFO_GETFILE
   if (do_files)
     {
       struct kinfo_file *kf = fdtbl.get ();
@@ -398,8 +300,6 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
       else
 	warning (_("unable to fetch list of open files"));
     }
-#endif
-#ifdef HAVE_KINFO_GETVMMAP
   if (do_mappings)
     {
       int nvment;
@@ -421,7 +321,6 @@ fbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
       else
 	warning (_("unable to fetch virtual memory map"));
     }
-#endif
   if (do_status)
     {
       if (!fbsd_fetch_kinfo_proc (pid, &kp))
-- 
2.25.1

[PATCH 5/5] Assume FreeBSD kernels always report exec events.

[John Baldwin]

FreeBSD kernels have reported exec events via the PL_FLAG_EXEC flag
since 8.2 release.  The most recent release that did not support this
flag is 7.4 released in November of 2011.

Note that the FreeBSD native target already assumed that PL_FLAG_SCE
and PL_FLAG_SCX were always supported on systems supporting
PT_LWPINFO, but those flags were added at the same time as
PL_FLAG_EXEC.  Building the native target on a system without
PL_FLAG_EXEC would have failed to build before this change already as
a result.

gdb/ChangeLog:

	* fbsd-nat.c (fbsd_nat_target::wait): Always check for
	PL_FLAG_EXEC.
	(fbsd_nat_target::insert_exec_catchpoint)
	(fbsd_nat_target::remove_exec_catchpoint): Always define.
	* fbsd-nat.h (fbsd_nat_target::insert_exec_catchpoint)
	(fbsd_nat_target::remove_exec_catchpoint): Always declare.
---
 gdb/ChangeLog  | 9 +++++++++
 gdb/fbsd-nat.c | 7 +------
 gdb/fbsd-nat.h | 2 --
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8201b357a5..a5fb48f091 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2020-07-20  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c (fbsd_nat_target::wait): Always check for
+	PL_FLAG_EXEC.
+	(fbsd_nat_target::insert_exec_catchpoint)
+	(fbsd_nat_target::remove_exec_catchpoint): Always define.
+	* fbsd-nat.h (fbsd_nat_target::insert_exec_catchpoint)
+	(fbsd_nat_target::remove_exec_catchpoint): Always declare.
+
 2020-07-20  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c (fbsd_read_mapping): Remove
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index ac88fbc87f..0ab7f83d68 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1335,7 +1335,6 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 #endif
 #endif
 
-#ifdef PL_FLAG_EXEC
 	  if (pl.pl_flags & PL_FLAG_EXEC)
 	    {
 	      ourstatus->kind = TARGET_WAITKIND_EXECD;
@@ -1343,7 +1342,6 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 		= xstrdup (pid_to_exec_file (pid));
 	      return wptid;
 	    }
-#endif
 
 #ifdef USE_SIGTRAP_SIGINFO
 	  if (fbsd_handle_debug_trap (this, wptid, pl))
@@ -1508,9 +1506,7 @@ fbsd_nat_target::post_attach (int pid)
   fbsd_add_threads (this, pid);
 }
 
-#ifdef PL_FLAG_EXEC
-/* If the FreeBSD kernel supports PL_FLAG_EXEC, then traced processes
-   will always stop after exec.  */
+/* Traced processes always stop after exec.  */
 
 int
 fbsd_nat_target::insert_exec_catchpoint (int pid)
@@ -1523,7 +1519,6 @@ fbsd_nat_target::remove_exec_catchpoint (int pid)
 {
   return 0;
 }
-#endif
 
 #ifdef HAVE_STRUCT_PTRACE_LWPINFO_PL_SYSCALL_CODE
 int
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index b49bf8cbaf..c3c5cccf7e 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -83,10 +83,8 @@ class fbsd_nat_target : public inf_ptrace_target
   int remove_vfork_catchpoint (int) override;
 #endif
 
-#ifdef PL_FLAG_EXEC
   int insert_exec_catchpoint (int) override;
   int remove_exec_catchpoint (int) override;
-#endif
 
 #ifdef HAVE_STRUCT_PTRACE_LWPINFO_PL_SYSCALL_CODE
   int set_syscall_catchpoint (int, bool, int, gdb::array_view<const int>)
-- 
2.25.1

Re: [PATCH 2/5] Assume KERN_PROC_PATHNAME is present on FreeBSD hosts.

[Simon Marchi]

On 2020-07-20 1:31 p.m., John Baldwin wrote:
> FreeBSD kernels have included this sysctl since 6.0 release.  The most
> recent release without support is 5.5 which was released in May of
> 2006.
> 
> gdb/ChangeLog:
> 
> 	* fbsd-nat.c (fbsd_nat_target::pid_to_exec_file): Always use
> 	sysctl and remove procfs fallback.
> ---
>  gdb/ChangeLog  |  5 +++++
>  gdb/fbsd-nat.c | 13 -------------
>  2 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 687d9aede5..0339083f1a 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2020-07-20  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* fbsd-nat.c (fbsd_nat_target::pid_to_exec_file): Always use
> +	sysctl and remove procfs fallback.
> +
>  2020-07-20  John Baldwin  <jhb@FreeBSD.org>
>  
>  	* fbsd-nat.c: Assume PT_LWPINFO is always defined.
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index fc7136a97c..6193e0fbde 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -53,11 +53,7 @@
>  char *
>  fbsd_nat_target::pid_to_exec_file (int pid)
>  {
> -  ssize_t len;
>    static char buf[PATH_MAX];
> -  char name[PATH_MAX];
> -
> -#ifdef KERN_PROC_PATHNAME
>    size_t buflen;
>    int mib[4];
>  
> @@ -71,15 +67,6 @@ fbsd_nat_target::pid_to_exec_file (int pid)
>         for processes without an associated executable such as kernel
>         processes.  */
>      return buflen == 0 ? NULL : buf;
> -#endif
> -
> -  xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
> -  len = readlink (name, buf, PATH_MAX - 1);
> -  if (len != -1)
> -    {
> -      buf[len] = '\0';
> -      return buf;
> -    }
>  
>    return NULL;
>  }
> -- 
> 2.25.1
> 

Note that the way the code is currently written allows the readlink method to be used
if the sysctl method fails at runtime.  We probably don't care, but I thought I'd raise
it just in case.

Simon

Re: [PATCH 2/5] Assume KERN_PROC_PATHNAME is present on FreeBSD hosts.

[John Baldwin]

On 7/20/20 7:16 PM, Simon Marchi wrote:
> On 2020-07-20 1:31 p.m., John Baldwin wrote:
>> FreeBSD kernels have included this sysctl since 6.0 release.  The most
>> recent release without support is 5.5 which was released in May of
>> 2006.
>>
>> gdb/ChangeLog:
>>
>> 	* fbsd-nat.c (fbsd_nat_target::pid_to_exec_file): Always use
>> 	sysctl and remove procfs fallback.
>> ---
>>  gdb/ChangeLog  |  5 +++++
>>  gdb/fbsd-nat.c | 13 -------------
>>  2 files changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 687d9aede5..0339083f1a 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2020-07-20  John Baldwin  <jhb@FreeBSD.org>
>> +
>> +	* fbsd-nat.c (fbsd_nat_target::pid_to_exec_file): Always use
>> +	sysctl and remove procfs fallback.
>> +
>>  2020-07-20  John Baldwin  <jhb@FreeBSD.org>
>>  
>>  	* fbsd-nat.c: Assume PT_LWPINFO is always defined.
>> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
>> index fc7136a97c..6193e0fbde 100644
>> --- a/gdb/fbsd-nat.c
>> +++ b/gdb/fbsd-nat.c
>> @@ -53,11 +53,7 @@
>>  char *
>>  fbsd_nat_target::pid_to_exec_file (int pid)
>>  {
>> -  ssize_t len;
>>    static char buf[PATH_MAX];
>> -  char name[PATH_MAX];
>> -
>> -#ifdef KERN_PROC_PATHNAME
>>    size_t buflen;
>>    int mib[4];
>>  
>> @@ -71,15 +67,6 @@ fbsd_nat_target::pid_to_exec_file (int pid)
>>         for processes without an associated executable such as kernel
>>         processes.  */
>>      return buflen == 0 ? NULL : buf;
>> -#endif
>> -
>> -  xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
>> -  len = readlink (name, buf, PATH_MAX - 1);
>> -  if (len != -1)
>> -    {
>> -      buf[len] = '\0';
>> -      return buf;
>> -    }
>>  
>>    return NULL;
>>  }
>> -- 
>> 2.25.1
>>
> 
> Note that the way the code is currently written allows the readlink method to be used
> if the sysctl method fails at runtime.  We probably don't care, but I thought I'd raise
> it just in case.

Yes, we don't care in this case.  Both the sysctl and the /proc/<pid>/exe file end
up resolving to the same function in the kernel (vn_fullpath).  If it fails for the
sysctl, the readlink() call will also fail.  I will add that to the commit message
though so it is clear to future readers.

-- 
John Baldwin

Re: [PATCH 0/5] Remove support for old FreeBSD hosts

[John Baldwin]

On 7/20/20 10:31 AM, John Baldwin wrote:
> The kinfo_get_file change could perhaps use further refinement.  We
> need a configure check to pick up the correct library (libutil on
> FreeBSD and libutil-freebsd on GNU/kFreeBSD).  However, the
> kinfo_get_file and kinfo_get_vmmap functions were added to this
> library at the same time (same SVN commit in FreeBSD), so checking for
> both is redundant.  Right now we check for kinfo_get_file in
> gdbsupport since filestuff.c uses it if present.  We could remove the
> check for kinfo_get_vmmap entirely from gdb/configure.ac if we think
> the kinfo_get_file check in gdbsupport/common.m4 is sufficient.  If
> so, I might need to at least move the comment from configure.ac over
> to common.m4 with some modifications.

Tom, I am curious for your input on this question at least.  You happened
to move the check for kinfo_get_file as part of the gdbsupport/ changes
earlier.  If the two checks were still on gdb's configure.ac I would
probably collapse them into one, and I would also probably not bother
defining a HAVE_FOO constant at all but just get configure to search
for the right library.  However, for filestuff.c the check in common.m4
including a HAVE_FOO is correct.  My question is if I should remove the
kinfo_get_vmmap from gdb's configure.ac entirely and depend on gdbsupport
to add the link dependency on the library or if I should keep a check in
gdb's configure.ac (but perhaps modify it to not set HAVE_FOO?).

-- 
John Baldwin

Re: [PATCH 0/5] Remove support for old FreeBSD hosts

[John Baldwin]

On 7/20/20 10:31 AM, John Baldwin wrote:
> This patch series aims to remove some cruft from the FreeBSD native
> target that isn't needed on modern systems.  Most of the changes
> remove support for systems released more than 10 years ago.  The last
> change is closer to 9 years ago, but any such systems already fail to
> compile the native target today (and for several recent releases).
> I've yet to receive a single complaint either here or on FreeBSD lists
> about such breakage.

I'd like to push this little series later this week, but since it's
been a while since I posted it I thought I'd give a heads up first.
I don't have any material differences aside from adding and additional
comment in the commit log for patch 2 I mentioned in an earlier
reply to Simon.

-- 
John Baldwin

Re: [PATCH 0/5] Remove support for old FreeBSD hosts

[Simon Marchi]

On 2020-08-03 12:19 p.m., John Baldwin wrote:
> On 7/20/20 10:31 AM, John Baldwin wrote:
>> The kinfo_get_file change could perhaps use further refinement.  We
>> need a configure check to pick up the correct library (libutil on
>> FreeBSD and libutil-freebsd on GNU/kFreeBSD).  However, the
>> kinfo_get_file and kinfo_get_vmmap functions were added to this
>> library at the same time (same SVN commit in FreeBSD), so checking for
>> both is redundant.  Right now we check for kinfo_get_file in
>> gdbsupport since filestuff.c uses it if present.  We could remove the
>> check for kinfo_get_vmmap entirely from gdb/configure.ac if we think
>> the kinfo_get_file check in gdbsupport/common.m4 is sufficient.  If
>> so, I might need to at least move the comment from configure.ac over
>> to common.m4 with some modifications.
> 
> Tom, I am curious for your input on this question at least.  You happened
> to move the check for kinfo_get_file as part of the gdbsupport/ changes
> earlier.  If the two checks were still on gdb's configure.ac I would
> probably collapse them into one, and I would also probably not bother
> defining a HAVE_FOO constant at all but just get configure to search
> for the right library.  However, for filestuff.c the check in common.m4
> including a HAVE_FOO is correct.  My question is if I should remove the
> kinfo_get_vmmap from gdb's configure.ac entirely and depend on gdbsupport
> to add the link dependency on the library or if I should keep a check in
> gdb's configure.ac (but perhaps modify it to not set HAVE_FOO?).

I think it would be ok to have checks only in gdbsupport/common.m4.  But I
think it would be clearer to separate the two concerns:

- which of -lutil and -lutil-freebsd should be added to LIBS?
- should HAVE_KINFO_GETFILE be defined?

So I would perhaps write it like this, even if it's a bit redundant:

  # On FreeBSD we may need libutil for the kinfo_get* functions.  On
  # GNU/kFreeBSD systems, FreeBSD libutil is renamed to libutil-freebsd.
  # Figure out which one to use.
  AC_SEARCH_LIBS(kinfo_getfile, util util-freebsd)

  # Define HAVE_KINFO_GETFILE if kinfo_getfile is available.
  AC_CHECK_FUNCS(kinfo_getfile)

But really I think any way would be fine, as long as it's properly
documented / commented.

Simon

Re: [PATCH 0/5] Remove support for old FreeBSD hosts

[John Baldwin]

On 9/10/20 5:24 AM, Simon Marchi wrote:
> I think it would be ok to have checks only in gdbsupport/common.m4.  But I
> think it would be clearer to separate the two concerns:
> 
> - which of -lutil and -lutil-freebsd should be added to LIBS?
> - should HAVE_KINFO_GETFILE be defined?
> 
> So I would perhaps write it like this, even if it's a bit redundant:
> 
>   # On FreeBSD we may need libutil for the kinfo_get* functions.  On
>   # GNU/kFreeBSD systems, FreeBSD libutil is renamed to libutil-freebsd.
>   # Figure out which one to use.
>   AC_SEARCH_LIBS(kinfo_getfile, util util-freebsd)
> 
>   # Define HAVE_KINFO_GETFILE if kinfo_getfile is available.
>   AC_CHECK_FUNCS(kinfo_getfile)
> 
> But really I think any way would be fine, as long as it's properly
> documented / commented.

I like this and I'll post a v2 with that in a bit once my test
build has finished.

-- 
John Baldwin