[RFA] Don't kill inferior if there's a typo in the specified port.

[RFA] Don't kill inferior if there's a typo in the specified port.

[Doug Evans]

Hi.

If there's a typo in the port gdbserver will kill the inferior.
This fixes that by calling detach_or_kill_inferior_callback
instead of kill_inferior_callback.

It also moves the status message to the callback;
it's only called when exiting gdb (and if that changes one can split
the function into silent/verbose versions).
An alternative is something like this:

-      fprintf (stderr, "Killing all inferiors\n");
+      fprintf (stderr, "Detaching or killing all inferiors\n");

but will that be confusing?
OTOH, if gdbserver ever gets used with 10's or 100's of processes,
exiting with one line per process may be a bit much.
OTOOH, the user might like to know what got detached and what got killed.
I don't know, but I can change the patch to do whatever y'all prefer.

Ok to check in?

2009-04-10  Doug Evans  <dje@google.com>

	Don't kill inferior if there's a typo in the specified port.
	* server.c (detach_or_kill_inferior_callback): Print whether we're
	detaching or killing.
	(main): Don't print "Killing all inferiors", leave message to
	detach_or_kill_inferior_callback, which we now call instead of
	kill_inferior_callback.

Index: gdbserver/server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.95
diff -u -p -u -p -r1.95 server.c]
--- gdbserver/server.c	1 Apr 2009 22:50:24 -0000	1.95
+++ gdbserver/server.c	10 Apr 2009 23:07:55 -0000
@@ -1808,6 +1808,11 @@ kill_inferior_callback (struct inferior_
   discard_queued_stop_replies (pid);
 }
 
+/* Callback for for_each_inferior to detach or kill the inferior,
+   depending on whether we attached to it or not.
+   We inform the user whether we're detaching or killing the process
+   as this is only called when gdbserver is about to exit.  */
+
 static void
 detach_or_kill_inferior_callback (struct inferior_list_entry *entry)
 {
@@ -1815,9 +1820,15 @@ detach_or_kill_inferior_callback (struct
   int pid = ptid_get_pid (process->head.id);
 
   if (process->attached)
-    detach_inferior (pid);
+    {
+      fprintf (stderr, "Detaching from %d\n", pid);
+      detach_inferior (pid);
+    }
   else
-    kill_inferior (pid);
+    {
+      fprintf (stderr, "Killing %d\n", pid);
+      kill_inferior (pid);
+    }
 
   discard_queued_stop_replies (pid);
 }
@@ -2015,9 +2026,8 @@ main (int argc, char *argv[])
 
   if (setjmp (toplevel))
     {
-      fprintf (stderr, "Killing all inferiors\n");
       for_each_inferior (&all_processes,
-			 kill_inferior_callback);
+			 detach_or_kill_inferior_callback);
       exit (1);
     }
 

Re: [RFA] Don't kill inferior if there's a typo in the specified port.

[Pedro Alves]

On Saturday 11 April 2009 00:18:14, Doug Evans wrote:
> Hi.
> 
> If there's a typo in the port gdbserver will kill the inferior.

To be clear to everyone else following this at home, this is for
"gdbserver --attach :badport PID", because the startup sequence is,

 1) create or attach to inferior
 2) open socket and wait for GDB connection

So, if #2 fails, we have to unwind 1.  Previously, we simply
killed the inferior.  That's bad when we have just attached to
to a running process, not so bad if we have created a new inferior.

> This fixes that by calling detach_or_kill_inferior_callback
> instead of kill_inferior_callback.

I think this makes sense.  Even if we change the startup sequence
to:

  1) open and bind listen socket.
  2) create/attach inferior
  3) wait for GDB connection (`accept').
  4) go debug

... which would also fix your problem, between #3 and #4, an error
can happen (although very rare), so we should detach from
any attachee from point #2.

The only thing that crosses my mind that could be strange, is
that if we detached from an inferior due to a gdbserver error/fatal
exit, while breakpoints are still installed, there's a risk that
the inferior will trip on them much later, causing inferior
SIGTRAP kamikaze.  By killing the inferior immediately, the
cause/effect was very evident.  :-)  There's no best behavior
to chose, tough --- it's a compromise.  I assume gdbserver will
be gaining Z0 (memory breakpoints) support in the near future,
which will make this point mostly moot.  Plus, as you found
out yesterday, we're already broken, in the case of something failing
while debugging, and we error out while reopening the
connection --- meaning that this is all theory that won't
happen much in practice.

All this giberish to say I'm all for detaching instead
of killing.  :-)

> It also moves the status message to the callback;
> it's only called when exiting gdb (and if that changes one can split
> the function into silent/verbose versions).
> An alternative is something like this:
> 
> -      fprintf (stderr, "Killing all inferiors\n");
> +      fprintf (stderr, "Detaching or killing all inferiors\n");
> 
> but will that be confusing?

Perhaps:

 "Detaching from inferiors we had attached to, and killing all others\n"

dunno, don't want to start a bikeshed-ish discussion on that.

> OTOH, if gdbserver ever gets used with 10's or 100's of processes,
> exiting with one line per process may be a bit much.
> OTOOH, the user might like to know what got detached and what got killed.
> I don't know, but I can change the patch to do whatever y'all prefer.

Yeah.  If this is a real problem, then we could output something like,

  Killing process(es) PID1, PID2, PID3, PID4, PIDnn.
  Detaching process(es) PID1, PID2, PID3, PID4, PIDnn.

This should mitigate the problem, although if you're really attached to
100's of processes, it will still print a lot.

The other thing that crossed my mind was that if you had a bunch
of processes, the real error message that let to gdbserver bailing out
would be buried in the output many lines before all those
"Killing/detaching process X".  The only version that doesn't
have this problem is the one that doesn't include any detail, like
the first option.

Anyway, I've no real inclination for any of these possible
outputs.

> Ok to check in?

This is fine with me.  Let's give it a few days in case Daniel or
others want to comment.

Thanks for documenting detach_or_kill_inferior_callback, btw ;-).

-- 
Pedro Alves

Re: [RFA] Don't kill inferior if there's a typo in the specified port.

[Doug Evans]

On Sat, Apr 11, 2009 at 8:04 AM, Pedro Alves <pedro@codesourcery.com> wrote:
>> It also moves the status message to the callback;
>> it's only called when exiting gdb (and if that changes one can split
>> the function into silent/verbose versions).
>> An alternative is something like this:
>>
>> -      fprintf (stderr, "Killing all inferiors\n");
>> +      fprintf (stderr, "Detaching or killing all inferiors\n");
>>
>> but will that be confusing?
>
> Perhaps:
>
>  "Detaching from inferiors we had attached to, and killing all others\n"
>
> dunno, don't want to start a bikeshed-ish discussion on that.
>
>> OTOH, if gdbserver ever gets used with 10's or 100's of processes,
>> exiting with one line per process may be a bit much.
>> OTOOH, the user might like to know what got detached and what got killed.
>> I don't know, but I can change the patch to do whatever y'all prefer.
>
> Yeah.  If this is a real problem, then we could output something like,
>
>  Killing process(es) PID1, PID2, PID3, PID4, PIDnn.
>  Detaching process(es) PID1, PID2, PID3, PID4, PIDnn.
>
> This should mitigate the problem, although if you're really attached to
> 100's of processes, it will still print a lot.
>
> The other thing that crossed my mind was that if you had a bunch
> of processes, the real error message that let to gdbserver bailing out
> would be buried in the output many lines before all those
> "Killing/detaching process X".  The only version that doesn't
> have this problem is the one that doesn't include any detail, like
> the first option.

One reason why I like printing which processes were detached from and
which were killed is: What happens if the process is gone shortly
after gdbserver exits?  Did gdbserver kill it, or did the detach screw
up, or did the program crash on its own shortly after gdbserver
detached?  Tracking this down will be a pain without this info.  One
could add a verbose option to print such info I guess, but it might
cut down on support costs if this info was always printed.

>
> Anyway, I've no real inclination for any of these possible
> outputs.
>
>> Ok to check in?
>
> This is fine with me.  Let's give it a few days in case Daniel or
> others want to comment.
>
> Thanks for documenting detach_or_kill_inferior_callback, btw ;-).
>
> --
> Pedro Alves
>

Re: [RFA] Don't kill inferior if there's a typo in the specified port.

[Doug Evans]

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

On Mon, Apr 13, 2009 at 11:49 AM, Doug Evans <dje@google.com> wrote:
> On Sat, Apr 11, 2009 at 8:04 AM, Pedro Alves <pedro@codesourcery.com> wrote:
>>> It also moves the status message to the callback;
>>> it's only called when exiting gdb (and if that changes one can split
>>> the function into silent/verbose versions).
>>> An alternative is something like this:
>>>
>>> -      fprintf (stderr, "Killing all inferiors\n");
>>> +      fprintf (stderr, "Detaching or killing all inferiors\n");
>>>
>>> but will that be confusing?
>>
>> Perhaps:
>>
>>  "Detaching from inferiors we had attached to, and killing all others\n"
>>
>> dunno, don't want to start a bikeshed-ish discussion on that.
>>
>>> OTOH, if gdbserver ever gets used with 10's or 100's of processes,
>>> exiting with one line per process may be a bit much.
>>> OTOOH, the user might like to know what got detached and what got killed.
>>> I don't know, but I can change the patch to do whatever y'all prefer.
>>
>> Yeah.  If this is a real problem, then we could output something like,
>>
>>  Killing process(es) PID1, PID2, PID3, PID4, PIDnn.
>>  Detaching process(es) PID1, PID2, PID3, PID4, PIDnn.
>>
>> This should mitigate the problem, although if you're really attached to
>> 100's of processes, it will still print a lot.
>>
>> The other thing that crossed my mind was that if you had a bunch
>> of processes, the real error message that let to gdbserver bailing out
>> would be buried in the output many lines before all those
>> "Killing/detaching process X".  The only version that doesn't
>> have this problem is the one that doesn't include any detail, like
>> the first option.
>
> One reason why I like printing which processes were detached from and
> which were killed is: What happens if the process is gone shortly
> after gdbserver exits?  Did gdbserver kill it, or did the detach screw
> up, or did the program crash on its own shortly after gdbserver
> detached?  Tracking this down will be a pain without this info.  One
> could add a verbose option to print such info I guess, but it might
> cut down on support costs if this info was always printed.
>
>>
>> Anyway, I've no real inclination for any of these possible
>> outputs.
>>
>>> Ok to check in?
>>
>> This is fine with me.  Let's give it a few days in case Daniel or
>> others want to comment.
>>
>> Thanks for documenting detach_or_kill_inferior_callback, btw ;-).
>>
>> --
>> Pedro Alves
>>
>

Ok to check this in?
It does

Killing process(es) PID1, PID2, PID3, PID4, PIDnn.
Detaching process(es) PID1, PID2, PID3, PID4, PIDnn.

though I removed the commas for simplicity.
[I'm proactively avoiding some of the issues being raised. :-)]

[-- Attachment #2: gdb-090430-gdbserver-attached-2.patch.txt --]
[-- Type: text/plain, Size: 5992 bytes --]

2009-04-30  Doug Evans  <dje@google.com>

	* inferiors.c (started_inferior_callback): New function.
	(attached_inferior_callback): New function.
	(have_started_inferiors_p, have_attached_inferiors_p): New functions.
	* server.c (print_started_pid, print_attached_pid): New functions.
	(detach_or_kill_for_exit): New function.
	(main): Call it instead of for_each_inferior (kill_inferior_callback).
	* server.h (have_started_inferiors_p): Declare.
	(have_attached_inferiors_p): Declare.

Index: inferiors.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/inferiors.c,v
retrieving revision 1.21
diff -u -p -r1.21 inferiors.c
--- inferiors.c	30 Apr 2009 18:35:55 -0000	1.21
+++ inferiors.c	30 Apr 2009 21:03:31 -0000
@@ -127,6 +127,8 @@ add_inferior_to_list (struct inferior_li
   list->tail = new_inferior;
 }
 
+/* Invoke ACTION for each inferior in LIST.  */
+
 void
 for_each_inferior (struct inferior_list *list,
 		   void (*action) (struct inferior_list_entry *))
@@ -447,6 +449,46 @@ find_process_pid (int pid)
     find_inferior_id (&all_processes, pid_to_ptid (pid));
 }
 
+/* Return non-zero if INF, a struct process_info, was started by us,
+   i.e. not attached to.  */
+
+static int
+started_inferior_callback (struct inferior_list_entry *entry, void *args)
+{
+  struct process_info *process = (struct process_info *) entry;
+
+  return ! process->attached;
+}
+
+/* Return non-zero if there are any inferiors that we have created
+   (as opposed to attached-to).  */
+
+int
+have_started_inferiors_p (void)
+{
+  return (find_inferior (&all_processes, started_inferior_callback, NULL)
+	  != NULL);
+}
+
+/* Return non-zero if INF, a struct process_info, was attached to.  */
+
+static int
+attached_inferior_callback (struct inferior_list_entry *entry, void *args)
+{
+  struct process_info *process = (struct process_info *) entry;
+
+  return process->attached;
+}
+
+/* Return non-zero if there are any inferiors that we have attached to.  */
+
+int
+have_attached_inferiors_p (void)
+{
+  return (find_inferior (&all_processes, attached_inferior_callback, NULL)
+	  != NULL);
+}
+
 struct process_info *
 get_thread_process (struct thread_info *thread)
 {
Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.95
diff -u -p -r1.95 server.c
--- server.c	1 Apr 2009 22:50:24 -0000	1.95
+++ server.c	30 Apr 2009 21:03:31 -0000
@@ -1808,6 +1808,11 @@ kill_inferior_callback (struct inferior_
   discard_queued_stop_replies (pid);
 }
 
+/* Callback for for_each_inferior to detach or kill the inferior,
+   depending on whether we attached to it or not.
+   We inform the user whether we're detaching or killing the process
+   as this is only called when gdbserver is about to exit.  */
+
 static void
 detach_or_kill_inferior_callback (struct inferior_list_entry *entry)
 {
@@ -1822,6 +1827,65 @@ detach_or_kill_inferior_callback (struct
   discard_queued_stop_replies (pid);
 }
 
+/* for_each_inferior callback for detach_or_kill_for_exit to print
+   the pids of started inferiors.  */
+
+static void
+print_started_pid (struct inferior_list_entry *entry)
+{
+  struct process_info *process = (struct process_info *) entry;
+
+  if (! process->attached)
+    {
+      int pid = ptid_get_pid (process->head.id);
+      fprintf (stderr, " %d", pid);
+    }
+}
+
+/* for_each_inferior callback for detach_or_kill_for_exit to print
+   the pids of attached inferiors.  */
+
+static void
+print_attached_pid (struct inferior_list_entry *entry)
+{
+  struct process_info *process = (struct process_info *) entry;
+
+  if (process->attached)
+    {
+      int pid = ptid_get_pid (process->head.id);
+      fprintf (stderr, " %d", pid);
+    }
+}
+
+/* Call this when exiting gdbserver with possible inferiors that need
+   to be killed or detached from.  */
+
+static void
+detach_or_kill_for_exit (void)
+{
+  /* First print a list of the inferiors we will be killing/detaching.
+     This is to assist the user, for example, in case the inferior unexpectedly
+     dies after we exit: did we screw up or did the inferior exit on its own?
+     Having this info will save some head-scratching.  */
+
+  if (have_started_inferiors_p ())
+    {
+      fprintf (stderr, "Killing process(es):");
+      for_each_inferior (&all_processes, print_started_pid);
+      fprintf (stderr, "\n");
+    }
+  if (have_attached_inferiors_p ())
+    {
+      fprintf (stderr, "Detaching process(es):");
+      for_each_inferior (&all_processes, print_attached_pid);
+      fprintf (stderr, "\n");
+    }
+
+  /* Now we can kill or detach the inferiors.  */
+
+  for_each_inferior (&all_processes, detach_or_kill_inferior_callback);
+}
+
 static void
 join_inferiors_callback (struct inferior_list_entry *entry)
 {
@@ -2015,9 +2079,7 @@ main (int argc, char *argv[])
 
   if (setjmp (toplevel))
     {
-      fprintf (stderr, "Killing all inferiors\n");
-      for_each_inferior (&all_processes,
-			 kill_inferior_callback);
+      detach_or_kill_for_exit ();
       exit (1);
     }
 
@@ -2062,8 +2124,7 @@ main (int argc, char *argv[])
 
       if (exit_requested)
 	{
-	  for_each_inferior (&all_processes,
-			     detach_or_kill_inferior_callback);
+	  detach_or_kill_for_exit ();
 	  exit (0);
 	}
       else
Index: server.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
retrieving revision 1.56
diff -u -p -r1.56 server.h
--- server.h	3 Apr 2009 20:15:51 -0000	1.56
+++ server.h	30 Apr 2009 21:03:31 -0000
@@ -236,6 +236,8 @@ void add_thread (ptid_t ptid, void *targ
 struct process_info *add_process (int pid, int attached);
 void remove_process (struct process_info *process);
 struct process_info *find_process_pid (int pid);
+int have_started_inferiors_p (void);
+int have_attached_inferiors_p (void);
 
 struct thread_info *find_thread_pid (ptid_t ptid);
 

Re: [RFA] Don't kill inferior if there's a typo in the specified port.

[Pedro Alves]

On Thursday 30 April 2009 22:51:27, Doug Evans wrote:

> >>> Ok to check in?
> >>
> >> This is fine with me.  Let's give it a few days in case Daniel or
> >> others want to comment.
> >>
> >> Thanks for documenting detach_or_kill_inferior_callback, btw ;-).
> >>
> >> --
> >> Pedro Alves
> >>
> >

> Ok to check this in?

Eh, I thought we had already settled.  :-)

> It does
> 
> Killing process(es) PID1, PID2, PID3, PID4, PIDnn.
> Detaching process(es) PID1, PID2, PID3, PID4, PIDnn.
> 
> though I removed the commas for simplicity.
> [I'm proactively avoiding some of the issues being raised. :-)]

It looks fine to me.  Please go ahead before someone has a
different idea.  :-)  Thanks!

-- 
Pedro Alves