[PATCH] bsd-kvm: Fix build after recent changes to path handling functions.

[PATCH] bsd-kvm: Fix build after recent changes to path handling functions.

[John Baldwin]

Convert bsd_kvm_corefile and the local filename in bsd_kvm_open to
std::string rather than simple char * pointers freed by xfree.
---
 gdb/bsd-kvm.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/gdb/bsd-kvm.c b/gdb/bsd-kvm.c
index 45c7f116472..87b79606b4f 100644
--- a/gdb/bsd-kvm.c
+++ b/gdb/bsd-kvm.c
@@ -21,6 +21,7 @@
 #include "defs.h"
 #include "cli/cli-cmds.h"
 #include "command.h"
+#include "filenames.h"
 #include "frame.h"
 #include "regcache.h"
 #include "target.h"
@@ -30,6 +31,7 @@
 #include "inferior.h"          /* for get_exec_file */
 #include "gdbthread.h"
 #include "gdbsupport/pathstuff.h"
+#include "gdbsupport/gdb_tilde_expand.h"
 
 #include <fcntl.h>
 #include <kvm.h>
@@ -47,7 +49,7 @@
 #include "bsd-kvm.h"
 
 /* Kernel memory device file.  */
-static const char *bsd_kvm_corefile;
+static std::string bsd_kvm_corefile;
 
 /* Kernel memory interface descriptor.  */
 static kvm_t *core_kd;
@@ -109,24 +111,19 @@ bsd_kvm_target_open (const char *arg, int from_tty)
   char errbuf[_POSIX2_LINE_MAX];
   const char *execfile = NULL;
   kvm_t *temp_kd;
-  char *filename = NULL;
+  std::string filename;
 
   target_preopen (from_tty);
 
   if (arg)
     {
-      filename = tilde_expand (arg);
-      if (filename[0] != '/')
-	{
-	  gdb::unique_xmalloc_ptr<char> temp (gdb_abspath (filename));
-
-	  xfree (filename);
-	  filename = temp.release ();
-	}
+      filename = gdb_tilde_expand (arg);
+      if (!IS_ABSOLUTE_PATH (filename))
+	filename = gdb_abspath (filename.c_str ());
     }
 
   execfile = get_exec_file (0);
-  temp_kd = kvm_openfiles (execfile, filename, NULL,
+  temp_kd = kvm_openfiles (execfile, filename.c_str (), NULL,
 			   write_files ? O_RDWR : O_RDONLY, errbuf);
   if (temp_kd == NULL)
     error (("%s"), errbuf);
@@ -155,6 +152,7 @@ bsd_kvm_target::close ()
       core_kd = NULL;
     }
 
+  bsd_kvm_corefile.clear ();
   switch_to_no_thread ();
   exit_inferior_silent (current_inferior ());
 }
@@ -203,9 +201,10 @@ bsd_kvm_target::xfer_partial (enum target_object object,
 void
 bsd_kvm_target::files_info ()
 {
-  if (bsd_kvm_corefile && strcmp (bsd_kvm_corefile, _PATH_MEM) != 0)
+  if (!bsd_kvm_corefile.empty ()
+      && strcmp (bsd_kvm_corefile.c_str (), _PATH_MEM) != 0)
     gdb_printf (_("\tUsing the kernel crash dump %s.\n"),
-		bsd_kvm_corefile);
+		bsd_kvm_corefile.c_str ());
   else
     gdb_printf (_("\tUsing the currently running kernel.\n"));
 }
-- 
2.34.1

Re: [PATCH] bsd-kvm: Fix build after recent changes to path handling functions.

[Pedro Alves]

On 2022-04-26 01:23, John Baldwin wrote:
> @@ -203,9 +201,10 @@ bsd_kvm_target::xfer_partial (enum target_object object,
>  void
>  bsd_kvm_target::files_info ()
>  {
> -  if (bsd_kvm_corefile && strcmp (bsd_kvm_corefile, _PATH_MEM) != 0)
> +  if (!bsd_kvm_corefile.empty ()
> +      && strcmp (bsd_kvm_corefile.c_str (), _PATH_MEM) != 0)

It looks like these two lines could be just "if (bsd_kvm_corefile != _PATH_MEM)" instead ?

>      gdb_printf (_("\tUsing the kernel crash dump %s.\n"),
> -		bsd_kvm_corefile);
> +		bsd_kvm_corefile.c_str ());
>    else
>      gdb_printf (_("\tUsing the currently running kernel.\n"));
>  }

Re: [PATCH] bsd-kvm: Fix build after recent changes to path handling functions.

[Simon Marchi]

> Convert bsd_kvm_corefile and the local filename in bsd_kvm_open to
> std::string rather than simple char * pointers freed by xfree.

Woops, sorry for missing this.

> @@ -203,9 +201,10 @@ bsd_kvm_target::xfer_partial (enum target_object object,
>  void
>  bsd_kvm_target::files_info ()
>  {
> -  if (bsd_kvm_corefile && strcmp (bsd_kvm_corefile, _PATH_MEM) != 0)
> +  if (!bsd_kvm_corefile.empty ()
> +      && strcmp (bsd_kvm_corefile.c_str (), _PATH_MEM) != 0)

The strcmp could probably become

  bsd_kvm_corefile != _PATH_MEM

Otherwise, LGTM.

Simon

Re: [PATCH] bsd-kvm: Fix build after recent changes to path handling functions.

[John Baldwin]

On 4/26/22 5:13 AM, Pedro Alves wrote:
> On 2022-04-26 01:23, John Baldwin wrote:
>> @@ -203,9 +201,10 @@ bsd_kvm_target::xfer_partial (enum target_object object,
>>   void
>>   bsd_kvm_target::files_info ()
>>   {
>> -  if (bsd_kvm_corefile && strcmp (bsd_kvm_corefile, _PATH_MEM) != 0)
>> +  if (!bsd_kvm_corefile.empty ()
>> +      && strcmp (bsd_kvm_corefile.c_str (), _PATH_MEM) != 0)
> 
> It looks like these two lines could be just "if (bsd_kvm_corefile != _PATH_MEM)" instead ?

Oh, yes.

>>       gdb_printf (_("\tUsing the kernel crash dump %s.\n"),
>> -		bsd_kvm_corefile);
>> +		bsd_kvm_corefile.c_str ());
>>     else
>>       gdb_printf (_("\tUsing the currently running kernel.\n"));
>>   }


-- 
John Baldwin

Re: [PATCH] bsd-kvm: Fix build after recent changes to path handling functions.

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John>  /* Kernel memory device file.  */
John> -static const char *bsd_kvm_corefile;
John> +static std::string bsd_kvm_corefile;

I never looked at this file before, but it seems like it could be
converted to multi-target without too much trouble.

Tom

Re: [PATCH] bsd-kvm: Fix build after recent changes to path handling functions.

[John Baldwin]

On 4/26/22 11:41 AM, Tom Tromey wrote:
>>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
> 
> John>  /* Kernel memory device file.  */
> John> -static const char *bsd_kvm_corefile;
> John> +static std::string bsd_kvm_corefile;
> 
> I never looked at this file before, but it seems like it could be
> converted to multi-target without too much trouble.

So I had looked at this a bit more when Pedro first did the multi-target
series at least in terms of moving more of the globals into the
bsd_kvm_target class.  However, on FreeBSD we don't use bsd-kvm.c
(and haven't in a long time).  I maintain a variant of it downstream
that uses different hooks (and supports multiple kthreads, etc.).   That
one I will likely convert to multi-target at some point.  I may still
fix this one just because I still use bsd-kvm.c as a kind of
reference for tracking API changes in my downstream version.

-- 
John Baldwin

Re: [PATCH] bsd-kvm: Fix build after recent changes to path handling functions.

[Tom Tromey]

John> However, on FreeBSD we don't use bsd-kvm.c
John> (and haven't in a long time).  I maintain a variant of it downstream
John> that uses different hooks (and supports multiple kthreads, etc.).   That
John> one I will likely convert to multi-target at some point.  I may still
John> fix this one just because I still use bsd-kvm.c as a kind of
John> reference for tracking API changes in my downstream version.

It sounds reasonable to me.  It's fine also not to do it.  Also, is
anyone using this code any more?

Tom

Re: [PATCH] bsd-kvm: Fix build after recent changes to path handling functions.

[John Baldwin]

On 4/26/22 12:37 PM, Tom Tromey wrote:
> John> However, on FreeBSD we don't use bsd-kvm.c
> John> (and haven't in a long time).  I maintain a variant of it downstream
> John> that uses different hooks (and supports multiple kthreads, etc.).   That
> John> one I will likely convert to multi-target at some point.  I may still
> John> fix this one just because I still use bsd-kvm.c as a kind of
> John> reference for tracking API changes in my downstream version.
> 
> It sounds reasonable to me.  It's fine also not to do it.  Also, is
> anyone using this code any more?

I have no idea if anyone is using it.  For me it is useful to see what API
changes I need to make downstream.  However, aside from that it is unused
on FreeBSD.  I don't know if anyone is using it on other *BSD's.

(I would eventually like to upstream some of the FreeBSD kernel debugging
bits I maintain, but they aren't really in a suitable shape for upstreaming
currently, though I might at some point try to start extracting at least
some of them for upstreaming.  The replacement for bsd-kvm.c would perhaps
be one of the last to upstream though.)

-- 
John Baldwin