[PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[Rainer Orth]

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

While looking through gdb.log, I found that two tests FAIL like this:

warning: No executable has been specified and target does not support
determining executable automatically.  Try using the "file" command.
0x00400dc4 in ?? ()
(gdb) FAIL: gdb.base/attach.exp: attach2, with no file

The other is gdb.base/quit-live.exp.  I've implemented the following
patch that fixes both failures, only then detecting that I'd previously
reported the issue as PR tdep/17903.

Tested on amd64-pc-solaris2.10 and amd64-pc-solaris2.11, ok for master?

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2018-06-13  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	PR tdep/17903
	* procfs.c (procfs_target): Declare pid_to_exec_file.
	(procfs_target::pid_to_exec_file): New.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sol2-procfs-pid_to_exec_file.patch --]
[-- Type: text/x-patch, Size: 1353 bytes --]

# HG changeset patch
# Parent  a7848e82a4d1e81ba23d3a82820e881924b84fdd
Provide pid_to_exec_file on Solaris

diff --git a/gdb/procfs.c b/gdb/procfs.c
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -128,6 +128,8 @@ public:
 
   const char *pid_to_str (ptid_t) override;
 
+  char *pid_to_exec_file (int pid) override;
+
   thread_control_capabilities get_thread_control_capabilities () override
   { return tc_schedlock; }
 
@@ -3135,6 +3137,35 @@ procfs_target::pid_to_str (ptid_t ptid)
   return buf;
 }
 
+/* Accepts an integer PID; Returns a string representing a file that
+   can be opened to get the symbols for the child process.  */
+
+char *
+procfs_target::pid_to_exec_file (int pid)
+{
+  static char buf[PATH_MAX];
+  char name[PATH_MAX];
+
+  /* Solaris 11 introduced /proc/<proc-id>/execname.  */
+  xsnprintf (name, PATH_MAX, "/proc/%d/execname", pid);
+  scoped_fd fd (open (name, O_RDONLY));
+  if (fd.get () < 0 || read (fd.get (), buf, PATH_MAX - 1) < 0)
+    {
+      /* If that fails, fall back to /proc/<proc-id>/path/a.out introduced in
+	 Solaris 10.  */
+      ssize_t len;
+
+      xsnprintf (name, PATH_MAX, "/proc/%d/path/a.out", pid);
+      len = readlink (name, buf, PATH_MAX - 1);
+      if (len <= 0)
+	strcpy (buf, name);
+      else
+	buf[len] = '\0';
+    }
+
+  return buf;
+}
+
 /* Insert a watchpoint.  */
 
 static int

Re: [PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[Joel Brobecker]

Hi Rainer,

On Wed, Sep 19, 2018 at 04:17:57PM +0200, Rainer Orth wrote:
> While looking through gdb.log, I found that two tests FAIL like this:
> 
> warning: No executable has been specified and target does not support
> determining executable automatically.  Try using the "file" command.
> 0x00400dc4 in ?? ()
> (gdb) FAIL: gdb.base/attach.exp: attach2, with no file
> 
> The other is gdb.base/quit-live.exp.  I've implemented the following
> patch that fixes both failures, only then detecting that I'd previously
> reported the issue as PR tdep/17903.
> 
> Tested on amd64-pc-solaris2.10 and amd64-pc-solaris2.11, ok for master?
> 
> 	Rainer
> 
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
> 
> 
> 2018-06-13  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> 
> 	PR tdep/17903
> 	* procfs.c (procfs_target): Declare pid_to_exec_file.
> 	(procfs_target::pid_to_exec_file): New.

Nice :). This is OK for me as is; one question: Have you considered
the use of an std::string for the variable "name"? I thought about it,
and I'm not sure it would make the code all that better, but thought
I'd mention it again, in case you or someone else sees something
I don't see.

> diff --git a/gdb/procfs.c b/gdb/procfs.c
> --- a/gdb/procfs.c
> +++ b/gdb/procfs.c
> @@ -128,6 +128,8 @@ public:
>  
>    const char *pid_to_str (ptid_t) override;
>  
> +  char *pid_to_exec_file (int pid) override;
> +
>    thread_control_capabilities get_thread_control_capabilities () override
>    { return tc_schedlock; }
>  
> @@ -3135,6 +3137,35 @@ procfs_target::pid_to_str (ptid_t ptid)
>    return buf;
>  }
>  
> +/* Accepts an integer PID; Returns a string representing a file that
> +   can be opened to get the symbols for the child process.  */
> +
> +char *
> +procfs_target::pid_to_exec_file (int pid)
> +{
> +  static char buf[PATH_MAX];
> +  char name[PATH_MAX];
> +
> +  /* Solaris 11 introduced /proc/<proc-id>/execname.  */
> +  xsnprintf (name, PATH_MAX, "/proc/%d/execname", pid);
> +  scoped_fd fd (open (name, O_RDONLY));
> +  if (fd.get () < 0 || read (fd.get (), buf, PATH_MAX - 1) < 0)
> +    {
> +      /* If that fails, fall back to /proc/<proc-id>/path/a.out introduced in
> +	 Solaris 10.  */
> +      ssize_t len;
> +
> +      xsnprintf (name, PATH_MAX, "/proc/%d/path/a.out", pid);
> +      len = readlink (name, buf, PATH_MAX - 1);
> +      if (len <= 0)
> +	strcpy (buf, name);
> +      else
> +	buf[len] = '\0';
> +    }
> +
> +  return buf;
> +}
> +
>  /* Insert a watchpoint.  */
>  
>  static int

-- 
Joel

Re: [PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[Tom Tromey]

>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

Rainer> Tested on amd64-pc-solaris2.10 and amd64-pc-solaris2.11, ok for master?

Thank you for doing this.

Rainer> +  char *pid_to_exec_file (int pid) override;

Boy this is a bad API!
Not your problem but I'll make a note on my to-do list to change it to
return a std::string or something like that.

Rainer> +  scoped_fd fd (open (name, O_RDONLY));

You may wish to use gdb_open_cloexec.

procfs.c is one of the cloexec holdouts, because I could never compile it.
But I guess once the buildbot is up I can do it :)

Tom

Re: [PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[Simon Marchi]

On 2018-09-19 10:48, Tom Tromey wrote:
> procfs.c is one of the cloexec holdouts, because I could never compile 
> it.
> But I guess once the buildbot is up I can do it :)

I don't know if you are registered to the gcc compile farm announce 
mailing list, so just in case, there are now some Solaris machines 
(perhaps the same as Rainer is using for the buildbot?):

https://lists.tetaneutral.net/pipermail/cfarm-announces/2018-September/000002.html

Simon

Re: [PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[Rainer Orth]

Hi Joel,

>> 2018-06-13  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
>> 
>> 	PR tdep/17903
>> 	* procfs.c (procfs_target): Declare pid_to_exec_file.
>> 	(procfs_target::pid_to_exec_file): New.
>
> Nice :). This is OK for me as is; one question: Have you considered
> the use of an std::string for the variable "name"? I thought about it,
> and I'm not sure it would make the code all that better, but thought
> I'd mention it again, in case you or someone else sees something
> I don't see.

TBH, I know close to nothing about C++, so rely on others for
suggestions here.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[Rainer Orth]

Hi Tom,

> Rainer> +  scoped_fd fd (open (name, O_RDONLY));
>
> You may wish to use gdb_open_cloexec.

I'll give it a whirl and change the patch accordingly before committing
if it works out.

> procfs.c is one of the cloexec holdouts, because I could never compile it.
> But I guess once the buildbot is up I can do it :)

Certainly: my proposed buildbot patch adds them to the try_ssh scheduler
to allow just that.  In the meantime, you may well send procfs patches
my way if you like ;-)

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[Rainer Orth]

Hi Simon,

> On 2018-09-19 10:48, Tom Tromey wrote:
>> procfs.c is one of the cloexec holdouts, because I could never compile
>> it.
>> But I guess once the buildbot is up I can do it :)
>
> I don't know if you are registered to the gcc compile farm announce mailing
> list, so just in case, there are now some Solaris machines (perhaps the
> same as Rainer is using for the buildbot?):
>
> https://lists.tetaneutral.net/pipermail/cfarm-announces/2018-September/000002.html

they aren't: I've got two Solaris 11.4 kernel zones (sparc and x86)
intended to run both the buildbots and in principle fit for addition to
the compile farm.  However, a transparent integration probably won't be
possible because University policy requires prior acceptance (though
online) of the Acceptable Use Policy.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[Joel Brobecker]

> >> 	PR tdep/17903
> >> 	* procfs.c (procfs_target): Declare pid_to_exec_file.
> >> 	(procfs_target::pid_to_exec_file): New.
> >
> > Nice :). This is OK for me as is; one question: Have you considered
> > the use of an std::string for the variable "name"? I thought about it,
> > and I'm not sure it would make the code all that better, but thought
> > I'd mention it again, in case you or someone else sees something
> > I don't see.
> 
> TBH, I know close to nothing about C++, so rely on others for
> suggestions here.

In that case, I would go with what you have now considering the current
interface for this method, and possibly enhance it later as a followup.
It's not obvious to me that switching that part to C++ is a win.

-- 
Joel

Re: [PATCH] Provide pid_to_exec_file on Solaris (PR tdep/17903)

[Rainer Orth]

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

Hi Tom,

> Rainer> +  scoped_fd fd (open (name, O_RDONLY));
>
> You may wish to use gdb_open_cloexec.

this worked just as well, so after retesting on amd64-pc-solaris2.11,
I've committed the following patch.

Thanks.
        Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sol2-procfs-pid_to_exec_file.patch --]
[-- Type: text/x-patch, Size: 1368 bytes --]

# HG changeset patch
# Parent  020f5ee983bb3daaef32d4d8424e6f60d24111c2
Provide pid_to_exec_file on Solaris

diff --git a/gdb/procfs.c b/gdb/procfs.c
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -128,6 +128,8 @@ public:
 
   const char *pid_to_str (ptid_t) override;
 
+  char *pid_to_exec_file (int pid) override;
+
   thread_control_capabilities get_thread_control_capabilities () override
   { return tc_schedlock; }
 
@@ -3138,6 +3140,35 @@ procfs_target::pid_to_str (ptid_t ptid)
   return buf;
 }
 
+/* Accepts an integer PID; Returns a string representing a file that
+   can be opened to get the symbols for the child process.  */
+
+char *
+procfs_target::pid_to_exec_file (int pid)
+{
+  static char buf[PATH_MAX];
+  char name[PATH_MAX];
+
+  /* Solaris 11 introduced /proc/<proc-id>/execname.  */
+  xsnprintf (name, PATH_MAX, "/proc/%d/execname", pid);
+  scoped_fd fd (gdb_open_cloexec (name, O_RDONLY, 0));
+  if (fd.get () < 0 || read (fd.get (), buf, PATH_MAX - 1) < 0)
+    {
+      /* If that fails, fall back to /proc/<proc-id>/path/a.out introduced in
+	 Solaris 10.  */
+      ssize_t len;
+
+      xsnprintf (name, PATH_MAX, "/proc/%d/path/a.out", pid);
+      len = readlink (name, buf, PATH_MAX - 1);
+      if (len <= 0)
+	strcpy (buf, name);
+      else
+	buf[len] = '\0';
+    }
+
+  return buf;
+}
+
 /* Insert a watchpoint.  */
 
 static int