RFC: fix problems with errors during quitting (killed.exp)

RFC: fix problems with errors during quitting (killed.exp)

[J. Johnston]

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

The following patch fixes the problem that occurs when an error happens while
quitting.  Often an error() call gets triggered and the poor user is thrown
back to the gdb command line where they try to quit again.  I made an attempt
to fix a similar problem by handling ptrace errors in quitting, however, thread-db.c gets
into multiple precarious situations because it has to rely on libthread_db
for information.  When the thread has been killed/maimed, it gets errors when
they are not expected and quitting becomes impossible.

My new strategy is to intercept error() calls during quitting.  Basically,
a simple get/set function is set up to denote when we the user has confirmed
a quit.  If we get into top.c:throw_exception() and quit has been confirmed, it
exits the program.  This handles any number of messy kill scenarios and causes
no regressions in the current testsuite.  It fixes gdb.threads/killed.exp.  I made the
functions external so other parts of gdb can register a confirmed quit if necessary.  I could
make these routines static if this feature is not needed.  I chose to exit with the
code being passed on the jump.  There may be a better constant choice.

If anyone can think of scenario that this code is not going to handle correctly that might
not be exercised by the testsuite, please let me know.

Tested on x86 and ia64.

Comments?

2003-09-04  Jeff Johnston  <jjohnstn@redhat.com>

	* top.h (get_quit_confirmed, set_quit_confirmed): New prototypes.
	* top.c (get_quit_confirmed, set_quit_confirmed): New functions.
	(throw_exception): If quit confirmed, exit instead of jumping.

[-- Attachment #2: kill-error.patch --]
[-- Type: text/plain, Size: 2489 bytes --]

Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.75
diff -u -r1.75 top.c
--- top.c	16 Aug 2003 18:38:46 -0000	1.75
+++ top.c	4 Sep 2003 21:34:25 -0000
@@ -341,6 +341,9 @@
      to that call via setjmp's return value.  Note that REASON can't
      be zero, by definition in defs.h. */
 
+  if (get_quit_confirmed ())
+    exit ((int) reason);
+
   (NORETURN void) SIGLONGJMP (*catch_return, (int) reason);
 }
 
@@ -546,6 +549,21 @@
   return catch_errors (do_captured_command, &args, "", mask);
 }
 
+/* Flag to indicate if quit has been performed.  */
+static int quit_confirmed;
+
+/* Verify if end-user has performed a quit operation.  */
+int
+get_quit_confirmed (void)
+{
+  return quit_confirmed;
+}
+
+void
+set_quit_confirmed (int val)
+{
+  quit_confirmed = val;
+}
 
 /* Handler for SIGHUP.  */
 
@@ -560,6 +578,7 @@
 {
   caution = 0;			/* Throw caution to the wind -- we're exiting.
 				   This prevents asking the user dumb questions.  */
+  set_quit_confirmed (1);
   quit_command ((char *) 0, 0);
   return 0;
 }
@@ -771,6 +790,7 @@
 	(*window_hook) (instream, get_prompt ());
 
       quit_flag = 0;
+      set_quit_confirmed (0);
       if (instream == stdin && stdin_is_tty)
 	reinitialize_more_filter ();
       old_chain = make_cleanup (null_cleanup, 0);
@@ -836,6 +856,7 @@
   while (instream && !feof (instream))
     {
       quit_flag = 0;
+      set_quit_confirmed (0);
       if (instream == stdin && stdin_is_tty)
 	reinitialize_more_filter ();
       old_chain = make_cleanup (null_cleanup, 0);
@@ -1422,6 +1443,8 @@
     {
       char *s;
 
+      set_quit_confirmed (0);
+
       /* This is something of a hack.  But there's no reliable way to
          see if a GUI is running.  The `use_windows' variable doesn't
          cut it.  */
@@ -1436,6 +1459,7 @@
 	return 0;
     }
 
+  set_quit_confirmed (1);
   return 1;
 }
 
Index: top.h
===================================================================
RCS file: /cvs/src/src/gdb/top.h,v
retrieving revision 1.9
diff -u -r1.9 top.h
--- top.h	8 Jun 2003 18:27:14 -0000	1.9
+++ top.h	4 Sep 2003 21:34:25 -0000
@@ -42,6 +42,8 @@
 				     void (*execute_command_func) (char *,
 								   int));
 extern int quit_confirm (void);
+extern int get_quit_confirmed (void);
+extern void set_quit_confirmed (int);
 extern void quit_force (char *, int);
 extern void quit_command (char *, int);
 extern int quit_cover (void *);

Re: RFC: fix problems with errors during quitting (killed.exp)

[Daniel Jacobowitz]

On Fri, Sep 05, 2003 at 11:53:55AM -0400, J. Johnston wrote:
> The following patch fixes the problem that occurs when an error happens 
> while
> quitting.  Often an error() call gets triggered and the poor user is thrown
> back to the gdb command line where they try to quit again.  I made an 
> attempt
> to fix a similar problem by handling ptrace errors in quitting, however, 
> thread-db.c gets
> into multiple precarious situations because it has to rely on libthread_db
> for information.  When the thread has been killed/maimed, it gets errors 
> when
> they are not expected and quitting becomes impossible.
> 
> My new strategy is to intercept error() calls during quitting.  Basically,
> a simple get/set function is set up to denote when we the user has confirmed
> a quit.  If we get into top.c:throw_exception() and quit has been 
> confirmed, it
> exits the program.  This handles any number of messy kill scenarios and 
> causes
> no regressions in the current testsuite.  It fixes gdb.threads/killed.exp.  
> I made the
> functions external so other parts of gdb can register a confirmed quit if 
> necessary.  I could
> make these routines static if this feature is not needed.  I chose to exit 
> with the
> code being passed on the jump.  There may be a better constant choice.
> 
> If anyone can think of scenario that this code is not going to handle 
> correctly that might
> not be exercised by the testsuite, please let me know.
> 
> Tested on x86 and ia64.
> 
> Comments?

My immediate concern is, does killed.exp leave a stopped binary around?

Other than that I like it.

> 2003-09-04  Jeff Johnston  <jjohnstn@redhat.com>
> 
> 	* top.h (get_quit_confirmed, set_quit_confirmed): New prototypes.
> 	* top.c (get_quit_confirmed, set_quit_confirmed): New functions.
> 	(throw_exception): If quit confirmed, exit instead of jumping.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: fix problems with errors during quitting (killed.exp)

[J. Johnston]

Daniel Jacobowitz wrote:
> On Fri, Sep 05, 2003 at 11:53:55AM -0400, J. Johnston wrote:
> 
>>The following patch fixes the problem that occurs when an error happens 
>>while
>>quitting.  Often an error() call gets triggered and the poor user is thrown
>>back to the gdb command line where they try to quit again.  I made an 
>>attempt
>>to fix a similar problem by handling ptrace errors in quitting, however, 
>>thread-db.c gets
>>into multiple precarious situations because it has to rely on libthread_db
>>for information.  When the thread has been killed/maimed, it gets errors 
>>when
>>they are not expected and quitting becomes impossible.
>>
>>My new strategy is to intercept error() calls during quitting.  Basically,
>>a simple get/set function is set up to denote when we the user has confirmed
>>a quit.  If we get into top.c:throw_exception() and quit has been 
>>confirmed, it
>>exits the program.  This handles any number of messy kill scenarios and 
>>causes
>>no regressions in the current testsuite.  It fixes gdb.threads/killed.exp.  
>>I made the
>>functions external so other parts of gdb can register a confirmed quit if 
>>necessary.  I could
>>make these routines static if this feature is not needed.  I chose to exit 
>>with the
>>code being passed on the jump.  There may be a better constant choice.
>>
>>If anyone can think of scenario that this code is not going to handle 
>>correctly that might
>>not be exercised by the testsuite, please let me know.
>>
>>Tested on x86 and ia64.
>>
>>Comments?
> 
> 
> My immediate concern is, does killed.exp leave a stopped binary around?
> 

It doesn't, but the debugged process going away was the cause of the error in the
first place.  I don't know if there are can't-quit scenarios where a process
will be left around.  Some fork/exec nightmare perhaps??, but I'm not sure what
gdb could be expected to do in such a situation.

-- Jeff J.

> Other than that I like it.
> 
> 
>>2003-09-04  Jeff Johnston  <jjohnstn@redhat.com>
>>
>>	* top.h (get_quit_confirmed, set_quit_confirmed): New prototypes.
>>	* top.c (get_quit_confirmed, set_quit_confirmed): New functions.
>>	(throw_exception): If quit confirmed, exit instead of jumping.
> 
> 
> 

Re: RFC: fix problems with errors during quitting (killed.exp)

[Andrew Cagney]

>> My new strategy is to intercept error() calls during quitting.  Basically,
>> a simple get/set function is set up to denote when we the user has confirmed
>> a quit.
> 
> 
> My immediate concern is, does killed.exp leave a stopped binary around?
> 
> Other than that I like it.

I feel ill.

The first bug is in quit_force, so modifying throw_exeption to work 
around it is a hack.  quit_force needs to catch any errors thrown by the 
target vector.

Looking at the function's body.  I think the first part:

>   /* An optional expression may be used to cause gdb to terminate with the 
>      value of that expression. */
>   if (args)
>     {
>       struct value *val = parse_and_eval (args);
> 
>       exit_code = (int) value_as_long (val);
>     }

should still be allowed to error out.  Otherwize something like:

	(gdb) exit bogus operand

will quit gdb denying the user of an oportunity to enter the correct 
command.  The target calls and (I guess) the write_history should be 
captured:

>   if (! ptid_equal (inferior_ptid, null_ptid) && target_has_execution)
>     {
>       if (attach_flag)
>         target_detach (args, from_tty);
>       else
>         target_kill ();
>     }
> 
>   /* UDI wants this, to kill the TIP.  */
>   target_close (1);
> 
>   /* Save the history information if it is appropriate to do so.  */
>   if (write_history_p && history_filename)
>     write_history (history_filename);

Also putting the do_final_cleanups inside the safety net would also 
probably be for the best.

>   do_final_cleanups (ALL_CLEANUPS);     /* Do any final cleanups before exiting 




The second less urgent bug is in the target code.  The target code 
should catch the error and then force itself to be popped (then 
rethrowing the error I guess).  That would have ensured that forward 
progress was made and that second quit attemt worked.

Andrew

Re: RFC: fix problems with errors during quitting (killed.exp)

[J. Johnston]

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



Andrew Cagney wrote:
>>> My new strategy is to intercept error() calls during quitting.  
>>> Basically,
>>> a simple get/set function is set up to denote when we the user has 
>>> confirmed
>>> a quit.
>>
>>
>>
>> My immediate concern is, does killed.exp leave a stopped binary around?
>>
>> Other than that I like it.
> 
> 
> I feel ill.
> 

I'm sorry to hear that.

> The first bug is in quit_force, so modifying throw_exeption to work 
> around it is a hack.  quit_force needs to catch any errors thrown by the 
> target vector.
> 
> Looking at the function's body.  I think the first part:
> 
>>   /* An optional expression may be used to cause gdb to terminate with 
>> the      value of that expression. */
>>   if (args)
>>     {
>>       struct value *val = parse_and_eval (args);
>>
>>       exit_code = (int) value_as_long (val);
>>     }
> 
> 
> should still be allowed to error out.  Otherwize something like:
> 
>     (gdb) exit bogus operand
> 
> will quit gdb denying the user of an oportunity to enter the correct 
> command.  The target calls and (I guess) the write_history should be 
> captured:
> 
>>   if (! ptid_equal (inferior_ptid, null_ptid) && target_has_execution)
>>     {
>>       if (attach_flag)
>>         target_detach (args, from_tty);
>>       else
>>         target_kill ();
>>     }
>>
>>   /* UDI wants this, to kill the TIP.  */
>>   target_close (1);
>>
>>   /* Save the history information if it is appropriate to do so.  */
>>   if (write_history_p && history_filename)
>>     write_history (history_filename);
> 
> 
> Also putting the do_final_cleanups inside the safety net would also 
> probably be for the best.
> 
>>   do_final_cleanups (ALL_CLEANUPS);     /* Do any final cleanups 
>> before exiting 
> 
> 

Ok, your idea is much better.  I have reworked the patch to use catch_errors().  I prepended the
message "Quitting: " so users will know that even though there is an error message
appended, it is still quitting.  I could optionally have put "Error quitting: "
but I thought the former was a better choice.

New patch attached.

2003-09-11  Jeff Johnston  <jjohnstn@redhat.com>

	* top.c (quit_target): New static helper function.
	(quit_force): Moved code to quit_target().  Call quit_target()
	via catch_errors() to catch errors during quit.

Ok to commit?

-- Jeff J.

> 
> 
> 
> The second less urgent bug is in the target code.  The target code 
> should catch the error and then force itself to be popped (then 
> rethrowing the error I guess).  That would have ensured that forward 
> progress was made and that second quit attemt worked.
> 
> Andrew
> 
> 

[-- Attachment #2: killed-patch2 --]
[-- Type: text/plain, Size: 1834 bytes --]

Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.75
diff -u -r1.75 top.c
--- top.c	16 Aug 2003 18:38:46 -0000	1.75
+++ top.c	11 Sep 2003 19:42:03 -0000
@@ -1439,28 +1439,25 @@
   return 1;
 }
 
-/* Quit without asking for confirmation.  */
+/* Helper routine for quit_force that requires error handling.  */
 
-void
-quit_force (char *args, int from_tty)
+struct qt_args
 {
-  int exit_code = 0;
-
-  /* An optional expression may be used to cause gdb to terminate with the 
-     value of that expression. */
-  if (args)
-    {
-      struct value *val = parse_and_eval (args);
+  char *args;
+  int from_tty;
+};
 
-      exit_code = (int) value_as_long (val);
-    }
+static int
+quit_target (void *arg)
+{
+  struct qt_args *qt = (struct qt_args *)arg;
 
   if (! ptid_equal (inferior_ptid, null_ptid) && target_has_execution)
     {
       if (attach_flag)
-	target_detach (args, from_tty);
+        target_detach (qt->args, qt->from_tty);
       else
-	target_kill ();
+        target_kill ();
     }
 
   /* UDI wants this, to kill the TIP.  */
@@ -1471,6 +1468,29 @@
     write_history (history_filename);
 
   do_final_cleanups (ALL_CLEANUPS);	/* Do any final cleanups before exiting */
+
+  return 0;
+}
+
+/* Quit without asking for confirmation.  */
+
+void
+quit_force (char *args, int from_tty)
+{
+  int exit_code = 0;
+
+  /* An optional expression may be used to cause gdb to terminate with the 
+     value of that expression. */
+  if (args)
+    {
+      struct value *val = parse_and_eval (args);
+
+      exit_code = (int) value_as_long (val);
+    }
+
+  /* We want to handle any quit errors and exit regardless.  */
+  catch_errors (quit_target, args,
+	        "Quitting: ", RETURN_MASK_ALL);
 
   exit (exit_code);
 }

Re: RFC: fix problems with errors during quitting (killed.exp)

[Andrew Cagney]

> New patch attached.
> 
> 2003-09-11  Jeff Johnston  <jjohnstn@redhat.com>
> 
>     * top.c (quit_target): New static helper function.
>     (quit_force): Moved code to quit_target().  Call quit_target()
>     via catch_errors() to catch errors during quit.
> 
> Ok to commit?

Yes, ok for 6.0 and mainline.  Thanks!

Anderw

Re: RFC: fix problems with errors during quitting (killed.exp)

[J. Johnston]

Patch checked into mainline and 6.0.

-- Jeff J.

Andrew Cagney wrote:
> 
>> New patch attached.
>>
>> 2003-09-11  Jeff Johnston  <jjohnstn@redhat.com>
>>
>>     * top.c (quit_target): New static helper function.
>>     (quit_force): Moved code to quit_target().  Call quit_target()
>>     via catch_errors() to catch errors during quit.
>>
>> Ok to commit?
> 
> 
> Yes, ok for 6.0 and mainline.  Thanks!
> 
> Anderw
> 
> 
>