[patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[Jan Kratochvil]

Hi,

There is a common question on #gdb and also already described:

http://sourceware.org/gdb/wiki/FAQ
16. Getting an internal error or other error while attaching to processes on
    GNU/Linux
->
    Try setenforce 0 (SELinux) or echo 0 >/proc/sys/kernel/yama/ptrace_scope
    (ptrace scope) to disable system security protections. 

and here is a patch to give some explanations.

More reasons can be given later, this is a container for them and it contains
some useful ones already.

No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu and with
non-extended gdbserver.

The testcase does not test gdbserver, somehow it is a bit difficult without
having shell on target.

Attaching to process 27480
ptrace: Operation not permitted.
(gdb) _
->
Attaching to process 27480
warning: process 27480 is already traced by process 29011
ptrace: Operation not permitted.
(gdb) _


Thanks,
Jan


gdb/
2012-03-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* common/linux-procfs.c (linux_proc_get_int): New, from
	linux_proc_get_tgid.
	(linux_proc_get_tgid): Only call linux_proc_get_int.
	(linux_proc_get_tracerpid): New.
	(linux_proc_pid_has_state): New, from linux_proc_pid_is_zombie.
	(linux_proc_pid_is_stopped, linux_proc_pid_is_zombie): Only call
	linux_proc_pid_has_state.
	* common/linux-procfs.h (linux_proc_get_tracerpid): New declaration.
	* common/linux-ptrace.c: Include linux-procfs.h.
	(linux_ptrace_attach_warnings): New.
	* common/linux-ptrace.h (linux_ptrace_attach_warnings): New declaration.
	* linux-nat.c: Include exceptions.h and linux-ptrace.h.
	(linux_nat_attach): New variable ex.  Wrap to_attach by TRY_CATCH and
	call linux_ptrace_attach_warnings.

gdb/gdbserver/
2012-03-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* linux-low.c (linux_attach_lwp_1): Call linux_ptrace_attach_warnings.

gdb/testsuite/
2012-03-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/attach-twice.c: New files.
	* gdb.base/attach-twice.exp: New files.

--- a/gdb/common/linux-procfs.c
+++ b/gdb/common/linux-procfs.c
@@ -28,67 +28,54 @@
 /* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
    found.  */
 
-int
-linux_proc_get_tgid (int lwpid)
+static int
+linux_proc_get_int (int lwpid, const char *field)
 {
+  size_t field_len = strlen (field);
   FILE *status_file;
   char buf[100];
-  int tgid = -1;
+  int retval = -1;
 
   snprintf (buf, sizeof (buf), "/proc/%d/status", (int) lwpid);
   status_file = fopen (buf, "r");
-  if (status_file != NULL)
+  if (status_file == NULL)
     {
-      while (fgets (buf, sizeof (buf), status_file))
-	{
-	  if (strncmp (buf, "Tgid:", 5) == 0)
-	    {
-	      tgid = strtoul (buf + strlen ("Tgid:"), NULL, 10);
-	      break;
-	    }
-	}
-
-      fclose (status_file);
+      warning (_("unable to open /proc file '%s'"), buf);
+      return -1;
     }
 
-  return tgid;
+  while (fgets (buf, sizeof (buf), status_file))
+    if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':')
+      {
+	retval = strtol (&buf[field_len + 1], NULL, 10);
+	break;
+      }
+
+  fclose (status_file);
+  return retval;
 }
 
-/* Detect `T (stopped)' in `/proc/PID/status'.
-   Other states including `T (tracing stop)' are reported as false.  */
+/* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
+   found.  */
 
 int
-linux_proc_pid_is_stopped (pid_t pid)
+linux_proc_get_tgid (int lwpid)
 {
-  FILE *status_file;
-  char buf[100];
-  int retval = 0;
+  return linux_proc_get_int (lwpid, "Tgid");
+}
 
-  snprintf (buf, sizeof (buf), "/proc/%d/status", (int) pid);
-  status_file = fopen (buf, "r");
-  if (status_file != NULL)
-    {
-      int have_state = 0;
-
-      while (fgets (buf, sizeof (buf), status_file))
-	{
-	  if (strncmp (buf, "State:", 6) == 0)
-	    {
-	      have_state = 1;
-	      break;
-	    }
-	}
-      if (have_state && strstr (buf, "T (stopped)") != NULL)
-	retval = 1;
-      fclose (status_file);
-    }
-  return retval;
+/* See linux-procfs.h.  */
+
+pid_t
+linux_proc_get_tracerpid (int lwpid)
+{
+  return linux_proc_get_int (lwpid, "TracerPid");
 }
 
-/* See linux-procfs.h declaration.  */
+/* Return non-zero if 'State' of /proc/PID/status contains STATE.  */
 
-int
-linux_proc_pid_is_zombie (pid_t pid)
+static int
+linux_proc_pid_has_state (pid_t pid, const char *state)
 {
   char buffer[100];
   FILE *procfile;
@@ -110,8 +97,24 @@ linux_proc_pid_is_zombie (pid_t pid)
 	have_state = 1;
 	break;
       }
-  retval = (have_state
-	    && strcmp (buffer, "State:\tZ (zombie)\n") == 0);
+  retval = (have_state && strstr (buffer, state) != NULL);
   fclose (procfile);
   return retval;
 }
+
+/* Detect `T (stopped)' in `/proc/PID/status'.
+   Other states including `T (tracing stop)' are reported as false.  */
+
+int
+linux_proc_pid_is_stopped (pid_t pid)
+{
+  return linux_proc_pid_has_state (pid, "T (stopped)");
+}
+
+/* See linux-procfs.h declaration.  */
+
+int
+linux_proc_pid_is_zombie (pid_t pid)
+{
+  return linux_proc_pid_has_state (pid, "Z (zombie)");
+}
--- a/gdb/common/linux-procfs.h
+++ b/gdb/common/linux-procfs.h
@@ -26,6 +26,11 @@
 
 extern int linux_proc_get_tgid (int lwpid);
 
+/* Return the TracerPid of LWPID from /proc/pid/status.  Returns -1 if not
+   found.  */
+
+extern pid_t linux_proc_get_tracerpid (int lwpid);
+
 /* Detect `T (stopped)' in `/proc/PID/status'.
    Other states including `T (tracing stop)' are reported as false.  */
 
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -24,3 +24,21 @@
 #endif
 
 #include "linux-ptrace.h"
+#include "linux-procfs.h"
+
+/* Print all possible reasons we could fail to attach PID.  */
+
+void
+linux_ptrace_attach_warnings (pid_t pid)
+{
+  pid_t tracerpid;
+
+  tracerpid = linux_proc_get_tracerpid (pid);
+  if (tracerpid > 0)
+    warning (_("process %d is already traced by process %d"), (int) pid,
+	     (int) tracerpid);
+
+  if (linux_proc_pid_is_zombie (pid))
+    warning (_("process %d is a zombie - the process has already terminated"),
+	     (int) pid);
+}
--- a/gdb/common/linux-ptrace.h
+++ b/gdb/common/linux-ptrace.h
@@ -65,4 +65,6 @@
 #define __WALL          0x40000000 /* Wait for any child.  */
 #endif
 
+extern void linux_ptrace_attach_warnings (pid_t pid);
+
 #endif /* COMMON_LINUX_PTRACE_H */
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -662,6 +662,7 @@ linux_attach_lwp_1 (unsigned long lwpid, int initial)
 	}
 
       /* If we fail to attach to a process, report an error.  */
+      linux_ptrace_attach_warnings (lwpid);
       error ("Cannot attach to lwp %ld: %s (%d)\n", lwpid,
 	     strerror (errno), errno);
     }
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -62,6 +62,8 @@
 #include "symfile.h"
 #include "agent.h"
 #include "tracepoint.h"
+#include "exceptions.h"
+#include "linux-ptrace.h"
 
 #ifndef SPUFS_MAGIC
 #define SPUFS_MAGIC 0x23c9b64e
@@ -1612,11 +1614,22 @@ linux_nat_attach (struct target_ops *ops, char *args, int from_tty)
   struct lwp_info *lp;
   int status;
   ptid_t ptid;
+  volatile struct gdb_exception ex;
 
   /* Make sure we report all signals during attach.  */
   linux_nat_pass_signals (0, NULL);
 
-  linux_ops->to_attach (ops, args, from_tty);
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      linux_ops->to_attach (ops, args, from_tty);
+    }
+  if (ex.reason < 0)
+    {
+      pid_t pid = parse_pid_to_attach (args);
+
+      linux_ptrace_attach_warnings (pid);
+      throw_exception (ex);
+    }
 
   /* The ptrace base target adds the main thread with (pid,0,0)
      format.  Decorate it with lwp info.  */
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-twice.c
@@ -0,0 +1,42 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011-2012 Free Software Foundation, Inc.
+
+   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 <stdlib.h>
+#include <unistd.h>
+#include <sys/ptrace.h>
+#include <errno.h>
+
+int
+main (void)
+{
+  long l;
+
+  switch (fork ())
+  {
+    case -1:
+      perror ("fork");
+      exit (1);
+    case 0:
+      errno = 0;
+      ptrace (PTRACE_ATTACH, getppid (), NULL, NULL);
+      if (errno != 0)
+	perror ("PTRACE_ATTACH");
+      break;
+  }
+  sleep (600);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-twice.exp
@@ -0,0 +1,52 @@
+# Copyright (C) 2012 Free Software Foundation, Inc.
+#
+# 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/>.
+
+# Manipulation with PID on target is not supported.
+if [is_remote target] then {
+    return 0
+}
+
+set testfile attach-twice
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [prepare_for_testing ${testfile}.exp $executable] } {
+    return -1
+}
+
+set testpid [eval exec $binfile &]
+exec sleep 2
+
+set parentpid 0
+
+set test "attach"
+gdb_test_multiple "attach $testpid" $test {
+    -re "Attaching to program: \[^\r\n\]*, process $testpid\r\n.*warning: process $testpid is already traced by process (\[0-9\]+)\r\n.*ptrace: Operation not permitted\\.\r\n$gdb_prompt $" {
+	set parentpid $expect_out(1,string)
+	pass $test
+    }
+    -re "Attaching to program: \[^\r\n\]*, process $testpid\r\n.*ptrace: Operation not permitted\\.\r\n$gdb_prompt $" {
+	fail $test
+    }
+    -re "\r\n$gdb_prompt $" {
+	xfail $test
+    }
+}
+
+eval exec ps xfw
+if {$parentpid != 0} {
+  eval exec kill -9 $parentpid
+}
+eval exec kill -9 $testpid

Re: [patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[Pedro Alves]

On 03/06/2012 06:17 AM, Jan Kratochvil wrote:

> +void
> +linux_ptrace_attach_warnings (pid_t pid)
> +{
> +  pid_t tracerpid;
> +
> +  tracerpid = linux_proc_get_tracerpid (pid);


...

> +extern pid_t linux_proc_get_tracerpid (int lwpid);
..

Would be neater with `pid_t lwpid'.

I agree this is a useful addition.

linux_ptrace_attach_warnings has a tiny race, in that by the time you open /proc/pid/... you
may be looking at a different process.  There's no way to fix that, I think, and this
will be right in the large majority of the cases, so we can just live with it, IMO.

You may have considered the following already, but I haven't seen it mentioned.  I think it
may be better if the new warning strings are actually part of the thrown error string:

  - for example, frontends that display the error to the user as a dialog
    box or similar show the whole warning+error automatically, instead of having the warning
    get lost in the console noise (if it's visible at all).
  - It's perhaps better for gdbserver as well, as we can forward the whole error string
    to GDB so that GDB shows it to the user (the `E.' thing) (although we could forward warnings as
    well with the O packet, though we don't do that presently).
  - If some code in GDB, or most likely a python script, wraps "attach", and catches the error,
    it presumably will also want to capture the warnings.  Having everything in the
    exception seems cleaner.

(Note this is not an objection.  Your patch is useful as it is.)

-- 
Pedro Alves

Re: [patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[Jan Kratochvil]

On Thu, 08 Mar 2012 14:55:22 +0100, Pedro Alves wrote:
> > +extern pid_t linux_proc_get_tracerpid (int lwpid);
> ..
> 
> Would be neater with `pid_t lwpid'.

I chose 'int' specifically for you as this is TID, not PID and specifically
you typically use 'int' instead of 'pid_t' in your patches / in gdbserver,
where pid_t is available.  I do not address this in reviews as I find it as
a real nitpick.

OK, so changed it to pid_t, thanks.


> linux_ptrace_attach_warnings has a tiny race, in that by the time you open /proc/pid/... you
> may be looking at a different process.  There's no way to fix that, I think, and this
> will be right in the large majority of the cases, so we can just live with it, IMO.

I agree, this is affecting all the /proc operations.  As the code examines
/proc only in the case the attachment failed so any races have only
informational impact.


> You may have considered the following already, but I haven't seen it mentioned.  I think it
> may be better if the new warning strings are actually part of the thrown error string:

I fully agree, IIRC I considered it a bit but:

(1) Plain C code dealing with dynamic strings in gdbserver has to be ugly.

(2) I did not want to bring in more of the gdb/ and libiberty/ framework into
    gdbserver as:

  (2a) All the work would be getting a bit out of the scope of this patch.

  (2b) If introducing a new framework to gdbserver code we should bring in STL
       and not the libiberty/gdb code.  But we cannot yet use STL.

So I chose the (1) way in this patch.

No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu and in
non-extended gdbserver mode.


Thanks,
Jan


gdb/
2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* common/linux-procfs.c (linux_proc_get_int): New, from
	linux_proc_get_tgid, change its LWPID type to pid_t, add parameter
	field.
	(linux_proc_get_tgid): Only call linux_proc_get_int.
	(linux_proc_get_tracerpid): New.
	(linux_proc_pid_has_state): New, from linux_proc_pid_is_zombie.
	(linux_proc_pid_is_stopped, linux_proc_pid_is_zombie): Only call
	linux_proc_pid_has_state.
	* common/linux-procfs.h (linux_proc_get_tracerpid): New declaration.
	* common/linux-ptrace.c: Include linux-procfs.h.
	(linux_ptrace_attach_warnings): New.
	* common/linux-ptrace.h (linux_ptrace_attach_warnings): New declaration.
	* linux-nat.c: Include exceptions.h and linux-ptrace.h.
	(linux_nat_attach): New variables ex and message.  Wrap to_attach by
	TRY_CATCH and call linux_ptrace_attach_warnings.

gdb/gdbserver/
2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* linux-low.c (linux_attach_lwp_1): New variable message.  Call
	linux_ptrace_attach_warnings.

gdb/testsuite/
2012-03-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/attach-twice.c: New files.
	* gdb.base/attach-twice.exp: New files.

--- a/gdb/common/linux-procfs.c
+++ b/gdb/common/linux-procfs.c
@@ -28,67 +28,54 @@
 /* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
    found.  */
 
-int
-linux_proc_get_tgid (int lwpid)
+static int
+linux_proc_get_int (pid_t lwpid, const char *field)
 {
+  size_t field_len = strlen (field);
   FILE *status_file;
   char buf[100];
-  int tgid = -1;
+  int retval = -1;
 
   snprintf (buf, sizeof (buf), "/proc/%d/status", (int) lwpid);
   status_file = fopen (buf, "r");
-  if (status_file != NULL)
+  if (status_file == NULL)
     {
-      while (fgets (buf, sizeof (buf), status_file))
-	{
-	  if (strncmp (buf, "Tgid:", 5) == 0)
-	    {
-	      tgid = strtoul (buf + strlen ("Tgid:"), NULL, 10);
-	      break;
-	    }
-	}
-
-      fclose (status_file);
+      warning (_("unable to open /proc file '%s'"), buf);
+      return -1;
     }
 
-  return tgid;
+  while (fgets (buf, sizeof (buf), status_file))
+    if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':')
+      {
+	retval = strtol (&buf[field_len + 1], NULL, 10);
+	break;
+      }
+
+  fclose (status_file);
+  return retval;
 }
 
-/* Detect `T (stopped)' in `/proc/PID/status'.
-   Other states including `T (tracing stop)' are reported as false.  */
+/* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
+   found.  */
 
 int
-linux_proc_pid_is_stopped (pid_t pid)
+linux_proc_get_tgid (pid_t lwpid)
 {
-  FILE *status_file;
-  char buf[100];
-  int retval = 0;
+  return linux_proc_get_int (lwpid, "Tgid");
+}
 
-  snprintf (buf, sizeof (buf), "/proc/%d/status", (int) pid);
-  status_file = fopen (buf, "r");
-  if (status_file != NULL)
-    {
-      int have_state = 0;
-
-      while (fgets (buf, sizeof (buf), status_file))
-	{
-	  if (strncmp (buf, "State:", 6) == 0)
-	    {
-	      have_state = 1;
-	      break;
-	    }
-	}
-      if (have_state && strstr (buf, "T (stopped)") != NULL)
-	retval = 1;
-      fclose (status_file);
-    }
-  return retval;
+/* See linux-procfs.h.  */
+
+pid_t
+linux_proc_get_tracerpid (pid_t lwpid)
+{
+  return linux_proc_get_int (lwpid, "TracerPid");
 }
 
-/* See linux-procfs.h declaration.  */
+/* Return non-zero if 'State' of /proc/PID/status contains STATE.  */
 
-int
-linux_proc_pid_is_zombie (pid_t pid)
+static int
+linux_proc_pid_has_state (pid_t pid, const char *state)
 {
   char buffer[100];
   FILE *procfile;
@@ -110,8 +97,24 @@ linux_proc_pid_is_zombie (pid_t pid)
 	have_state = 1;
 	break;
       }
-  retval = (have_state
-	    && strcmp (buffer, "State:\tZ (zombie)\n") == 0);
+  retval = (have_state && strstr (buffer, state) != NULL);
   fclose (procfile);
   return retval;
 }
+
+/* Detect `T (stopped)' in `/proc/PID/status'.
+   Other states including `T (tracing stop)' are reported as false.  */
+
+int
+linux_proc_pid_is_stopped (pid_t pid)
+{
+  return linux_proc_pid_has_state (pid, "T (stopped)");
+}
+
+/* See linux-procfs.h declaration.  */
+
+int
+linux_proc_pid_is_zombie (pid_t pid)
+{
+  return linux_proc_pid_has_state (pid, "Z (zombie)");
+}
--- a/gdb/common/linux-procfs.h
+++ b/gdb/common/linux-procfs.h
@@ -24,7 +24,12 @@
 /* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
    found.  */
 
-extern int linux_proc_get_tgid (int lwpid);
+extern int linux_proc_get_tgid (pid_t lwpid);
+
+/* Return the TracerPid of LWPID from /proc/pid/status.  Returns -1 if not
+   found.  */
+
+extern pid_t linux_proc_get_tracerpid (pid_t lwpid);
 
 /* Detect `T (stopped)' in `/proc/PID/status'.
    Other states including `T (tracing stop)' are reported as false.  */
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -24,3 +24,36 @@
 #endif
 
 #include "linux-ptrace.h"
+#include "linux-procfs.h"
+
+/* Find all possible reasons we could fail to attach PID.  Prepend strings for
+   those reasons to *MESSAGEP each one delimited by a newline.  *MESSAGEP
+   should be '\0'-terminated string allocated by xmalloc, its address may
+   change.  */
+
+void
+linux_ptrace_attach_warnings (pid_t pid, char **messagep)
+{
+  pid_t tracerpid;
+
+  tracerpid = linux_proc_get_tracerpid (pid);
+  if (tracerpid > 0)
+    {
+      char *s = xstrprintf (_("warning: "
+			      "process %d is already traced by process %d\n%s"),
+			    (int) pid, (int) tracerpid, *messagep);
+
+      xfree (*messagep);
+      *messagep = s;
+    }
+
+  if (linux_proc_pid_is_zombie (pid))
+    {
+      char *s = xstrprintf (_("warning: process %d is a zombie "
+			      "- the process has already terminated\n%s"),
+			    (int) pid, *messagep);
+
+      xfree (*messagep);
+      *messagep = s;
+    }
+}
--- a/gdb/common/linux-ptrace.h
+++ b/gdb/common/linux-ptrace.h
@@ -65,4 +65,6 @@
 #define __WALL          0x40000000 /* Wait for any child.  */
 #endif
 
+extern void linux_ptrace_attach_warnings (pid_t pid, char **messagep);
+
 #endif /* COMMON_LINUX_PTRACE_H */
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -652,6 +652,8 @@ linux_attach_lwp_1 (unsigned long lwpid, int initial)
 
   if (ptrace (PTRACE_ATTACH, lwpid, 0, 0) != 0)
     {
+      char *message;
+
       if (!initial)
 	{
 	  /* If we fail to attach to an LWP, just warn.  */
@@ -662,8 +664,10 @@ linux_attach_lwp_1 (unsigned long lwpid, int initial)
 	}
 
       /* If we fail to attach to a process, report an error.  */
-      error ("Cannot attach to lwp %ld: %s (%d)\n", lwpid,
-	     strerror (errno), errno);
+      message = xstrprintf ("Cannot attach to lwp %ld: %s (%d)", lwpid,
+			    strerror (errno), errno);
+      linux_ptrace_attach_warnings (lwpid, &message);
+      error ("%s\n", message);
     }
 
   if (initial)
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -62,6 +62,8 @@
 #include "symfile.h"
 #include "agent.h"
 #include "tracepoint.h"
+#include "exceptions.h"
+#include "linux-ptrace.h"
 
 #ifndef SPUFS_MAGIC
 #define SPUFS_MAGIC 0x23c9b64e
@@ -1612,11 +1614,26 @@ linux_nat_attach (struct target_ops *ops, char *args, int from_tty)
   struct lwp_info *lp;
   int status;
   ptid_t ptid;
+  volatile struct gdb_exception ex;
 
   /* Make sure we report all signals during attach.  */
   linux_nat_pass_signals (0, NULL);
 
-  linux_ops->to_attach (ops, args, from_tty);
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      linux_ops->to_attach (ops, args, from_tty);
+    }
+  if (ex.reason < 0)
+    {
+      pid_t pid = parse_pid_to_attach (args);
+      char *message;
+
+      message = xstrdup (ex.message);
+      make_cleanup (free_current_contents, &message);
+
+      linux_ptrace_attach_warnings (pid, &message);
+      throw_error (ex.error, "%s", message);
+    }
 
   /* The ptrace base target adds the main thread with (pid,0,0)
      format.  Decorate it with lwp info.  */
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-twice.c
@@ -0,0 +1,42 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011-2012 Free Software Foundation, Inc.
+
+   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 <stdlib.h>
+#include <unistd.h>
+#include <sys/ptrace.h>
+#include <errno.h>
+
+int
+main (void)
+{
+  long l;
+
+  switch (fork ())
+  {
+    case -1:
+      perror ("fork");
+      exit (1);
+    case 0:
+      errno = 0;
+      ptrace (PTRACE_ATTACH, getppid (), NULL, NULL);
+      if (errno != 0)
+	perror ("PTRACE_ATTACH");
+      break;
+  }
+  sleep (600);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-twice.exp
@@ -0,0 +1,52 @@
+# Copyright (C) 2012 Free Software Foundation, Inc.
+#
+# 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/>.
+
+# Manipulation with PID on target is not supported.
+if [is_remote target] then {
+    return 0
+}
+
+set testfile attach-twice
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [prepare_for_testing ${testfile}.exp $executable] } {
+    return -1
+}
+
+set testpid [eval exec $binfile &]
+exec sleep 2
+
+set parentpid 0
+
+set test "attach"
+gdb_test_multiple "attach $testpid" $test {
+    -re "Attaching to program: \[^\r\n\]*, process $testpid\r\n.*warning: process $testpid is already traced by process (\[0-9\]+)\r\n.*ptrace: Operation not permitted\\.\r\n$gdb_prompt $" {
+	set parentpid $expect_out(1,string)
+	pass $test
+    }
+    -re "Attaching to program: \[^\r\n\]*, process $testpid\r\n.*ptrace: Operation not permitted\\.\r\n$gdb_prompt $" {
+	fail $test
+    }
+    -re "\r\n$gdb_prompt $" {
+	xfail $test
+    }
+}
+
+eval exec ps xfw
+if {$parentpid != 0} {
+  eval exec kill -9 $parentpid
+}
+eval exec kill -9 $testpid

Re: [patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[Pedro Alves]

On 03/13/2012 09:43 AM, Jan Kratochvil wrote:

> On Thu, 08 Mar 2012 14:55:22 +0100, Pedro Alves wrote:
>>> +extern pid_t linux_proc_get_tracerpid (int lwpid);
>> ..
>>
>> Would be neater with `pid_t lwpid'.
> 
> I chose 'int' specifically for you as this is TID, not PID and specifically


That's flattering.  :-)

> you typically use 'int' instead of 'pid_t' in your patches / in gdbserver,


I use int in my patches where I'm touching code that is already using int everywhere.
If I'm touching code that is already using `int' everywhere, I'll make my code use
it too.  `int' is what the codebase uses and was already using before I ever
laid my hands on it.  Your original code had:

 +void
 +linux_ptrace_attach_warnings (pid_t pid)
                                ^^^^^^^^^
 +{
 +  pid_t tracerpid;
 +
 +  tracerpid = linux_proc_get_tracerpid (pid);
                                          ^^^


So the caller had a pid_t handy.


 +pid_t
 +linux_proc_get_tracerpid (int lwpid)
 +{

And this returns pid_t, but actually takes int, although the only caller
has a pid_t handy.  It just seems neater to be all around consistent
here.  That's all.

> where pid_t is available.  I do not address this in reviews as I find it as
> a real nitpick.


I'll appreciate the nits if they're pointing at a similar inconsistency.  I'd
never say anything if int's were being used in the surrounding code.  Anyway, lots of
words for a handful of characters.  :-)

> OK, so changed it to pid_t, thanks.

Thanks.

>> linux_ptrace_attach_warnings has a tiny race, in that by the time you open /proc/pid/... you
>> may be looking at a different process.  There's no way to fix that, I think, and this
>> will be right in the large majority of the cases, so we can just live with it, IMO.
> 
> I agree, this is affecting all the /proc operations.  


Yeah, although if you're already attached to the process, then the risk of /proc/..
hitting the wrong pid is much lower, though it does exist.  One way I've considered
to minimize that further down to practically zero, was to open /proc/PID/task/TID/...
instead of /proc/PID/... in most places.  It'd like the /proc equivalent of tgkill
(where Linux's tgkill was invented precisely to avoid such races).

> As the code examines

> /proc only in the case the attachment failed so any races have only
> informational impact.




> 
> 
>> You may have considered the following already, but I haven't seen it mentioned.  I think it
>> may be better if the new warning strings are actually part of the thrown error string:
> 
> I fully agree, IIRC I considered it a bit but:
> 
> (1) Plain C code dealing with dynamic strings in gdbserver has to be ugly.


There's common/buffer.[h|c] for that.  It's like a very simple obstack.  I wrote it
initially for gdbserver, exactly to avoid pulling in libiberty.  At some point, we'll
probably end up eliminating it, but it's there now.  I'm okay with your new version though.

>   (2b) If introducing a new framework to gdbserver code we should bring in STL

>        and not the libiberty/gdb code.  But we cannot yet use STL.


Although I'm a C++ supporter, I really don't think such C++ arguments hold
much weight for the current C based code base.

> gdb/
> 2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
> 	* common/linux-procfs.c (linux_proc_get_int): New, from
> 	linux_proc_get_tgid, change its LWPID type to pid_t, add parameter
> 	field.
> 	(linux_proc_get_tgid): Only call linux_proc_get_int.
> 	(linux_proc_get_tracerpid): New.
> 	(linux_proc_pid_has_state): New, from linux_proc_pid_is_zombie.
> 	(linux_proc_pid_is_stopped, linux_proc_pid_is_zombie): Only call
> 	linux_proc_pid_has_state.
> 	* common/linux-procfs.h (linux_proc_get_tracerpid): New declaration.
> 	* common/linux-ptrace.c: Include linux-procfs.h.
> 	(linux_ptrace_attach_warnings): New.
> 	* common/linux-ptrace.h (linux_ptrace_attach_warnings): New declaration.
> 	* linux-nat.c: Include exceptions.h and linux-ptrace.h.
> 	(linux_nat_attach): New variables ex and message.  Wrap to_attach by
> 	TRY_CATCH and call linux_ptrace_attach_warnings.
>
> gdb/gdbserver/
> 2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
> 	* linux-low.c (linux_attach_lwp_1): New variable message.  Call
> 	linux_ptrace_attach_warnings.
>
> gdb/testsuite/
> 2012-03-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
> 	* gdb.base/attach-twice.c: New files.
> 	* gdb.base/attach-twice.exp: New files.

Looks good to me.  Thanks.

-- 
Pedro Alves

Re: [patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[Jan Kratochvil]

On Tue, 13 Mar 2012 11:33:19 +0100, Pedro Alves wrote:
> On 03/13/2012 09:43 AM, Jan Kratochvil wrote:
> > I agree, this is affecting all the /proc operations.  

OK, so not completely all.


> Yeah, although if you're already attached to the process, then the risk of /proc/..
> hitting the wrong pid is much lower, though it does exist.

How?  It remains as 'Z (zombie)' with non-zero 'TracerPid' and even root
cannot reuse that PID without killing the tracer.

The problem is if ptrace fails, then there are no lifetime guarantees.


> There's common/buffer.[h|c] for that.

I did not know.  So GDB uses two different implementations of dynamic strings
(with mem_fileopen).


> >   (2b) If introducing a new framework to gdbserver code we should bring in STL
> >        and not the libiberty/gdb code.  But we cannot yet use STL.
> 
> Although I'm a C++ supporter, I really don't think such C++ arguments hold
> much weight for the current C based code base.

Offtopic here but I think this code was another argument for C++, wasn't it?

No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu and in
non-extended gdbserver mode.


Thanks,
Jan


gdb/
2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* common/linux-procfs.c (linux_proc_get_int): New, from
	linux_proc_get_tgid, change its LWPID type to pid_t, add parameter
	field.
	(linux_proc_get_tgid): Only call linux_proc_get_int.
	(linux_proc_get_tracerpid): New.
	(linux_proc_pid_has_state): New, from linux_proc_pid_is_zombie.
	(linux_proc_pid_is_stopped, linux_proc_pid_is_zombie): Only call
	linux_proc_pid_has_state.
	* common/linux-procfs.h (linux_proc_get_tracerpid): New declaration.
	* common/linux-ptrace.c: Include linux-procfs.h and buffer.h.
	(linux_ptrace_attach_warnings): New.
	* common/linux-ptrace.h (struct buffer, linux_ptrace_attach_warnings):
	New declaration.
	* linux-nat.c: Include exceptions.h, linux-ptrace.h and buffer.h.
	(linux_nat_attach): New variables ex, buffer, message and message_s.
	Wrap to_attach by TRY_CATCH and call linux_ptrace_attach_warnings.

gdb/gdbserver/
2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* linux-low.c (linux_attach_lwp_1): New variable buffer.  Call
	linux_ptrace_attach_warnings.

gdb/testsuite/
2012-03-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/attach-twice.c: New files.
	* gdb.base/attach-twice.exp: New files.

--- a/gdb/common/linux-procfs.c
+++ b/gdb/common/linux-procfs.c
@@ -28,67 +28,54 @@
 /* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
    found.  */
 
-int
-linux_proc_get_tgid (int lwpid)
+static int
+linux_proc_get_int (pid_t lwpid, const char *field)
 {
+  size_t field_len = strlen (field);
   FILE *status_file;
   char buf[100];
-  int tgid = -1;
+  int retval = -1;
 
   snprintf (buf, sizeof (buf), "/proc/%d/status", (int) lwpid);
   status_file = fopen (buf, "r");
-  if (status_file != NULL)
+  if (status_file == NULL)
     {
-      while (fgets (buf, sizeof (buf), status_file))
-	{
-	  if (strncmp (buf, "Tgid:", 5) == 0)
-	    {
-	      tgid = strtoul (buf + strlen ("Tgid:"), NULL, 10);
-	      break;
-	    }
-	}
-
-      fclose (status_file);
+      warning (_("unable to open /proc file '%s'"), buf);
+      return -1;
     }
 
-  return tgid;
+  while (fgets (buf, sizeof (buf), status_file))
+    if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':')
+      {
+	retval = strtol (&buf[field_len + 1], NULL, 10);
+	break;
+      }
+
+  fclose (status_file);
+  return retval;
 }
 
-/* Detect `T (stopped)' in `/proc/PID/status'.
-   Other states including `T (tracing stop)' are reported as false.  */
+/* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
+   found.  */
 
 int
-linux_proc_pid_is_stopped (pid_t pid)
+linux_proc_get_tgid (pid_t lwpid)
 {
-  FILE *status_file;
-  char buf[100];
-  int retval = 0;
+  return linux_proc_get_int (lwpid, "Tgid");
+}
 
-  snprintf (buf, sizeof (buf), "/proc/%d/status", (int) pid);
-  status_file = fopen (buf, "r");
-  if (status_file != NULL)
-    {
-      int have_state = 0;
-
-      while (fgets (buf, sizeof (buf), status_file))
-	{
-	  if (strncmp (buf, "State:", 6) == 0)
-	    {
-	      have_state = 1;
-	      break;
-	    }
-	}
-      if (have_state && strstr (buf, "T (stopped)") != NULL)
-	retval = 1;
-      fclose (status_file);
-    }
-  return retval;
+/* See linux-procfs.h.  */
+
+pid_t
+linux_proc_get_tracerpid (pid_t lwpid)
+{
+  return linux_proc_get_int (lwpid, "TracerPid");
 }
 
-/* See linux-procfs.h declaration.  */
+/* Return non-zero if 'State' of /proc/PID/status contains STATE.  */
 
-int
-linux_proc_pid_is_zombie (pid_t pid)
+static int
+linux_proc_pid_has_state (pid_t pid, const char *state)
 {
   char buffer[100];
   FILE *procfile;
@@ -110,8 +97,24 @@ linux_proc_pid_is_zombie (pid_t pid)
 	have_state = 1;
 	break;
       }
-  retval = (have_state
-	    && strcmp (buffer, "State:\tZ (zombie)\n") == 0);
+  retval = (have_state && strstr (buffer, state) != NULL);
   fclose (procfile);
   return retval;
 }
+
+/* Detect `T (stopped)' in `/proc/PID/status'.
+   Other states including `T (tracing stop)' are reported as false.  */
+
+int
+linux_proc_pid_is_stopped (pid_t pid)
+{
+  return linux_proc_pid_has_state (pid, "T (stopped)");
+}
+
+/* See linux-procfs.h declaration.  */
+
+int
+linux_proc_pid_is_zombie (pid_t pid)
+{
+  return linux_proc_pid_has_state (pid, "Z (zombie)");
+}
--- a/gdb/common/linux-procfs.h
+++ b/gdb/common/linux-procfs.h
@@ -24,7 +24,12 @@
 /* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
    found.  */
 
-extern int linux_proc_get_tgid (int lwpid);
+extern int linux_proc_get_tgid (pid_t lwpid);
+
+/* Return the TracerPid of LWPID from /proc/pid/status.  Returns -1 if not
+   found.  */
+
+extern pid_t linux_proc_get_tracerpid (pid_t lwpid);
 
 /* Detect `T (stopped)' in `/proc/PID/status'.
    Other states including `T (tracing stop)' are reported as false.  */
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -24,3 +24,26 @@
 #endif
 
 #include "linux-ptrace.h"
+#include "linux-procfs.h"
+#include "buffer.h"
+
+/* Find all possible reasons we could fail to attach PID and append these
+   newline terminated reason strings to initialized BUFFER.  '\0' termination
+   of BUFFER must be done by the caller.  */
+
+void
+linux_ptrace_attach_warnings (pid_t pid, struct buffer *buffer)
+{
+  pid_t tracerpid;
+
+  tracerpid = linux_proc_get_tracerpid (pid);
+  if (tracerpid > 0)
+    buffer_xml_printf (buffer, _("warning: process %d is already traced "
+				 "by process %d\n"),
+		       (int) pid, (int) tracerpid);
+
+  if (linux_proc_pid_is_zombie (pid))
+    buffer_xml_printf (buffer, _("warning: process %d is a zombie "
+				 "- the process has already terminated\n"),
+		       (int) pid);
+}
--- a/gdb/common/linux-ptrace.h
+++ b/gdb/common/linux-ptrace.h
@@ -18,6 +18,8 @@
 #ifndef COMMON_LINUX_PTRACE_H
 #define COMMON_LINUX_PTRACE_H
 
+struct buffer;
+
 #include <sys/ptrace.h>
 
 #ifndef PTRACE_GETSIGINFO
@@ -65,4 +67,6 @@
 #define __WALL          0x40000000 /* Wait for any child.  */
 #endif
 
+extern void linux_ptrace_attach_warnings (pid_t pid, struct buffer *buffer);
+
 #endif /* COMMON_LINUX_PTRACE_H */
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -652,6 +652,8 @@ linux_attach_lwp_1 (unsigned long lwpid, int initial)
 
   if (ptrace (PTRACE_ATTACH, lwpid, 0, 0) != 0)
     {
+      struct buffer buffer;
+
       if (!initial)
 	{
 	  /* If we fail to attach to an LWP, just warn.  */
@@ -662,8 +664,11 @@ linux_attach_lwp_1 (unsigned long lwpid, int initial)
 	}
 
       /* If we fail to attach to a process, report an error.  */
-      error ("Cannot attach to lwp %ld: %s (%d)\n", lwpid,
-	     strerror (errno), errno);
+      buffer_init (&buffer);
+      linux_ptrace_attach_warnings (lwpid, &buffer);
+      buffer_grow_str0 (&buffer, "");
+      error ("%sCannot attach to lwp %ld: %s (%d)", buffer_finish (&buffer),
+	     lwpid, strerror (errno), errno);
     }
 
   if (initial)
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -62,6 +62,9 @@
 #include "symfile.h"
 #include "agent.h"
 #include "tracepoint.h"
+#include "exceptions.h"
+#include "linux-ptrace.h"
+#include "buffer.h"
 
 #ifndef SPUFS_MAGIC
 #define SPUFS_MAGIC 0x23c9b64e
@@ -1612,11 +1615,33 @@ linux_nat_attach (struct target_ops *ops, char *args, int from_tty)
   struct lwp_info *lp;
   int status;
   ptid_t ptid;
+  volatile struct gdb_exception ex;
 
   /* Make sure we report all signals during attach.  */
   linux_nat_pass_signals (0, NULL);
 
-  linux_ops->to_attach (ops, args, from_tty);
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      linux_ops->to_attach (ops, args, from_tty);
+    }
+  if (ex.reason < 0)
+    {
+      pid_t pid = parse_pid_to_attach (args);
+      struct buffer buffer;
+      char *message, *buffer_s;
+
+      message = xstrdup (ex.message);
+      make_cleanup (xfree, message);
+
+      buffer_init (&buffer);
+      linux_ptrace_attach_warnings (pid, &buffer);
+
+      buffer_grow_str0 (&buffer, "");
+      buffer_s = buffer_finish (&buffer);
+      make_cleanup (xfree, buffer_s);
+
+      throw_error (ex.error, "%s%s", buffer_s, message);
+    }
 
   /* The ptrace base target adds the main thread with (pid,0,0)
      format.  Decorate it with lwp info.  */
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-twice.c
@@ -0,0 +1,42 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011-2012 Free Software Foundation, Inc.
+
+   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 <stdlib.h>
+#include <unistd.h>
+#include <sys/ptrace.h>
+#include <errno.h>
+
+int
+main (void)
+{
+  long l;
+
+  switch (fork ())
+  {
+    case -1:
+      perror ("fork");
+      exit (1);
+    case 0:
+      errno = 0;
+      ptrace (PTRACE_ATTACH, getppid (), NULL, NULL);
+      if (errno != 0)
+	perror ("PTRACE_ATTACH");
+      break;
+  }
+  sleep (600);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-twice.exp
@@ -0,0 +1,52 @@
+# Copyright (C) 2012 Free Software Foundation, Inc.
+#
+# 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/>.
+
+# Manipulation with PID on target is not supported.
+if [is_remote target] then {
+    return 0
+}
+
+set testfile attach-twice
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [prepare_for_testing ${testfile}.exp $executable] } {
+    return -1
+}
+
+set testpid [eval exec $binfile &]
+exec sleep 2
+
+set parentpid 0
+
+set test "attach"
+gdb_test_multiple "attach $testpid" $test {
+    -re "Attaching to program: \[^\r\n\]*, process $testpid\r\n.*warning: process $testpid is already traced by process (\[0-9\]+)\r\n.*ptrace: Operation not permitted\\.\r\n$gdb_prompt $" {
+	set parentpid $expect_out(1,string)
+	pass $test
+    }
+    -re "Attaching to program: \[^\r\n\]*, process $testpid\r\n.*ptrace: Operation not permitted\\.\r\n$gdb_prompt $" {
+	fail $test
+    }
+    -re "\r\n$gdb_prompt $" {
+	xfail $test
+    }
+}
+
+eval exec ps xfw
+if {$parentpid != 0} {
+  eval exec kill -9 $parentpid
+}
+eval exec kill -9 $testpid

Re: [patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[Pedro Alves]

On 03/13/2012 01:54 PM, Jan Kratochvil wrote:

> On Tue, 13 Mar 2012 11:33:19 +0100, Pedro Alves wrote:
>> On 03/13/2012 09:43 AM, Jan Kratochvil wrote:
>>> I agree, this is affecting all the /proc operations.  
> 
> OK, so not completely all.
> 
> 
>> Yeah, although if you're already attached to the process, then the risk of /proc/..
>> hitting the wrong pid is much lower, though it does exist.
> 
> How?  It remains as 'Z (zombie)' with non-zero 'TracerPid' and even root
> cannot reuse that PID without killing the tracer.


E.g., with a multi-threaded exec, the execing thread just disappears
from /proc/TID as in reality the kernel changed the execing thread's TID, so
it's possible we end trying to open the non-existing /proc/TID/... before handling
the exec event.  I think there were other ways for a tracee to be completely
killed/removed behind the tracer's back, but maybe that was only with older kernels,
if it ever was.

>>>   (2b) If introducing a new framework to gdbserver code we should bring in STL
>>>        and not the libiberty/gdb code.  But we cannot yet use STL.
>>
>> Although I'm a C++ supporter, I really don't think such C++ arguments hold
>> much weight for the current C based code base.
> 
> Offtopic here but I think this code was another argument for C++, wasn't it?


It was like saying that if you had posted this code written in C++ doing this
dynamic string growth with delete[]/new[] to reallocate (instead of std::string, not
knowing its existence), and justified it by claiming the right solution is to
switch to Java.   Eventually.  And only maybe.

> gdb/
> 2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* common/linux-procfs.c (linux_proc_get_int): New, from
> 	linux_proc_get_tgid, change its LWPID type to pid_t, add parameter
> 	field.
> 	(linux_proc_get_tgid): Only call linux_proc_get_int.
> 	(linux_proc_get_tracerpid): New.
> 	(linux_proc_pid_has_state): New, from linux_proc_pid_is_zombie.
> 	(linux_proc_pid_is_stopped, linux_proc_pid_is_zombie): Only call
> 	linux_proc_pid_has_state.
> 	* common/linux-procfs.h (linux_proc_get_tracerpid): New declaration.
> 	* common/linux-ptrace.c: Include linux-procfs.h and buffer.h.
> 	(linux_ptrace_attach_warnings): New.
> 	* common/linux-ptrace.h (struct buffer, linux_ptrace_attach_warnings):
> 	New declaration.
> 	* linux-nat.c: Include exceptions.h, linux-ptrace.h and buffer.h.
> 	(linux_nat_attach): New variables ex, buffer, message and message_s.
> 	Wrap to_attach by TRY_CATCH and call linux_ptrace_attach_warnings.
> 
> gdb/gdbserver/
> 2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* linux-low.c (linux_attach_lwp_1): New variable buffer.  Call
> 	linux_ptrace_attach_warnings.
> 
> gdb/testsuite/
> 2012-03-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/attach-twice.c: New files.
> 	* gdb.base/attach-twice.exp: New files.
> 


Looks good too.

-- 
Pedro Alves

Re: [patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[Jan Kratochvil]

On Tue, 13 Mar 2012 15:34:55 +0100, Pedro Alves wrote:
> E.g., with a multi-threaded exec, the execing thread just disappears
> from /proc/TID as in reality the kernel changed the execing thread's TID, so
> it's possible we end trying to open the non-existing /proc/TID/... before handling
> the exec event.  I think there were other ways for a tracee to be completely
> killed/removed behind the tracer's back, but maybe that was only with older kernels,
> if it ever was.

I do not see when exactly this race may happen.

Still I always thought /proc is racy by design, it does not have to be (not
sure if it currently is or is not).


> dynamic string growth with delete[]/new[] to reallocate (instead of std::string, not
> knowing its existence),

The difference is statistically everyone knows STL but nobody knows
GDB-specific (or Sourceware-specific) libraries/functions.  This makes
learning curve of possible contributors needlessly steep.


Thanks,
Jan

[commit] [patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[Jan Kratochvil]

On Tue, 13 Mar 2012 15:34:55 +0100, Pedro Alves wrote:
> > gdb/
> > 2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* common/linux-procfs.c (linux_proc_get_int): New, from
> > 	linux_proc_get_tgid, change its LWPID type to pid_t, add parameter
> > 	field.
> > 	(linux_proc_get_tgid): Only call linux_proc_get_int.
> > 	(linux_proc_get_tracerpid): New.
> > 	(linux_proc_pid_has_state): New, from linux_proc_pid_is_zombie.
> > 	(linux_proc_pid_is_stopped, linux_proc_pid_is_zombie): Only call
> > 	linux_proc_pid_has_state.
> > 	* common/linux-procfs.h (linux_proc_get_tracerpid): New declaration.
> > 	* common/linux-ptrace.c: Include linux-procfs.h and buffer.h.
> > 	(linux_ptrace_attach_warnings): New.
> > 	* common/linux-ptrace.h (struct buffer, linux_ptrace_attach_warnings):
> > 	New declaration.
> > 	* linux-nat.c: Include exceptions.h, linux-ptrace.h and buffer.h.
> > 	(linux_nat_attach): New variables ex, buffer, message and message_s.
> > 	Wrap to_attach by TRY_CATCH and call linux_ptrace_attach_warnings.
> > 
> > gdb/gdbserver/
> > 2012-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* linux-low.c (linux_attach_lwp_1): New variable buffer.  Call
> > 	linux_ptrace_attach_warnings.
> > 
> > gdb/testsuite/
> > 2012-03-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* gdb.base/attach-twice.c: New files.
> > 	* gdb.base/attach-twice.exp: New files.
> 
> Looks good too.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2012-03/msg00169.html


Thanks,
Jan

Re: [patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Offtopic here but I think this code was another argument for C++,
Jan> wasn't it?

Yeah; but my sense was there was also general agreement that, even if we
switch gdb, we won't switch gdbserver.

Tom