[RFC] Patch for QUIT macro support

[RFC] Patch for QUIT macro support

[Fred Fish]

If you use gdb to debug a large executable, turn off pagination, and
use "rbreak ." or just "rbreak" with no argument, you can't interrupt
the rbreak command until it is done, which can take a very long time
depending upon the circumstances.  You can reproduce this somewhat by
using gdb to debug itself.

To test a possible fix for this, I made the following change:

--- symtab.c	27 Sep 2006 23:02:50 -0000	1.1.1.5.6.1
+++ symtab.c	13 Oct 2006 04:48:15 -0000
@@ -3306,6 +3306,7 @@ rbreak_command (char *regexp, int from_t
 
   for (p = ss; p != NULL; p = p->next)
     {
+      QUIT;	/* Allow user to ^C in case ss list is huge (I.E. "rbreak .") */
       if (p->msymbol == NULL)
 	{
 	  char *string = alloca (strlen (p->symtab->filename)

but to my surprise, this had no affect.  Further inveswtigation lead
me to the conclusion that the QUIT macro is no longer useful because
of changes in when the quit_flag is set.  Without getting to deep into
the details, when a user types ^C now, the function that catches the
signal arranges for a handler to be called, but that handler isn't
actually called until the event loop checks to see if there are any
handlers in the ready to run state.  And that doesn't happen during
the running of internal gdb commands like rbreak_command.

The patch attached below is pretty minimal and does fix this problem,
but it has the following issues:

(1) It exposes a previously internal entry point of the event handler
to other parts of gdb.

(2) It potential allows signal handlers unrelated to SIGINT to be
run at every point in the gdb code where QUIT is used.  Perhaps what
is needed is a way to restrict invoke_async_signal_handler to only
test for one particular handler being ready.

(3) If in fact QUIT is broken, and perhaps has been for a while,
it's possible that it's no longer safe in some places where it's
been used and reenabling it without more extensive testing could
cause other problems.

-Fred

+2006-10-12  Fred Fish  <fnf@specifix.com>
+
+	* defs.h (QUIT): Call invoke_async_signal_handler() if
+	quit_flag is set.
+	* event-loop.c (invoke_async_signal_handler): Remove static
+	qualifier so it can be called from other parts of gdb.
+	* event-top.c (handle_sigint): Set quit_flag so we can check
+	at convenient points if we want to abandon some long running
+	processing, via use of the "QUIT" macro.
+	(async_request_quit): Remove useless setting of quit_flag,
+	done too late to be helpful.
+	* symtab.c (rbreak_command): Call QUIT to test for ^C before
+	setting each breakpoint.

Index: defs.h
===================================================================
RCS file: /cvsroots/specifix/gnusense/gdb/gdb/defs.h,v
retrieving revision 1.1.1.5
diff -u -p -r1.1.1.5 defs.h
--- defs.h	22 Dec 2005 01:56:10 -0000	1.1.1.5
+++ defs.h	13 Oct 2006 04:45:03 -0000
@@ -174,8 +174,8 @@ extern void quit (void);
 #define QUIT_FIXME "ignoring redefinition of QUIT"
 #else
 #define QUIT { \
-  if (quit_flag) quit (); \
-  if (deprecated_interactive_hook) deprecated_interactive_hook (); \
+  extern void invoke_async_signal_handler (void); \
+  if (quit_flag) invoke_async_signal_handler (); \
 }
 #endif
 
Index: event-loop.c
===================================================================
RCS file: /cvsroots/specifix/gnusense/gdb/gdb/event-loop.c,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 event-loop.c
--- event-loop.c	22 Dec 2005 01:56:31 -0000	1.1.1.2
+++ event-loop.c	13 Oct 2006 04:46:26 -0000
@@ -214,7 +214,7 @@ sighandler_list;
 static int async_handler_ready = 0;
 
 static void create_file_handler (int fd, int mask, handler_func * proc, gdb_client_data client_data);
-static void invoke_async_signal_handler (void);
+void invoke_async_signal_handler (void);
 static void handle_file_event (int event_file_desc);
 static int gdb_wait_for_event (void);
 static int check_async_ready (void);
@@ -982,7 +982,7 @@ mark_async_signal_handler (async_signal_
 }
 
 /* Call all the handlers that are ready. */
-static void
+void
 invoke_async_signal_handler (void)
 {
   async_signal_handler *async_handler_ptr;
Index: event-top.c
===================================================================
RCS file: /cvsroots/specifix/gnusense/gdb/gdb/event-top.c,v
retrieving revision 1.1.1.4.6.1
diff -u -p -r1.1.1.4.6.1 event-top.c
--- event-top.c	27 Sep 2006 23:02:49 -0000	1.1.1.4.6.1
+++ event-top.c	13 Oct 2006 04:47:23 -0000
@@ -969,6 +969,14 @@ handle_sigint (int sig)
 {
   signal (sig, handle_sigint);
 
+  /* This used to be in async_request_quit() but for the normal case
+     when immediate_quit isn't set, mark_async_signal_handler_wrapper
+     arranges to the signal handlers to eventually be checked, and
+     only at that point would async_request_quit get called, which is
+     too late for most things checking quit_flag with the QUIT
+     macro. */
+  quit_flag = 1;
+
   /* If immediate_quit is set, we go ahead and process the SIGINT right
      away, even if we usually would defer this to the event loop. The
      assumption here is that it is safe to process ^C immediately if
@@ -988,7 +996,6 @@ handle_sigint (int sig)
 void
 async_request_quit (gdb_client_data arg)
 {
-  quit_flag = 1;
   quit ();
 }
 

Re: [RFC] Patch for QUIT macro support

[Fred Fish]

On Friday 13 October 2006 05:43, Fred Fish wrote:
> To test a possible fix for this, I made the following change:

OK, guess I should keep up with the gdb-patches list more, particularly
before posting.  :-)

I see Nick Roberts just posted a patch a few hours earlier that
addresses the issue of where quit_flag is set.  It also occured to
me after posting my quick and dirty patch that it probably wasn't
necessary to jump all the way back into the event loop signal
handling code, just simply continue to check for the quit_flag
and call quit when it's set.

-Fred

Re: [RFC] Patch for QUIT macro support

[Daniel Jacobowitz]

On Fri, Oct 13, 2006 at 06:22:39AM -0700, Fred Fish wrote:
> On Friday 13 October 2006 05:43, Fred Fish wrote:
> > To test a possible fix for this, I made the following change:
> 
> OK, guess I should keep up with the gdb-patches list more, particularly
> before posting.  :-)
> 
> I see Nick Roberts just posted a patch a few hours earlier that
> addresses the issue of where quit_flag is set.  It also occured to
> me after posting my quick and dirty patch that it probably wasn't
> necessary to jump all the way back into the event loop signal
> handling code, just simply continue to check for the quit_flag
> and call quit when it's set.

Do you mean the big async patch?  If so it's still worth fixing this in
isolation.  Otherwise, I must have missed a patch.

I may not have time to look at this for a while - I find the event loop
so confusing - but in the meantime, it's come up before:

  http://sourceware.org/ml/gdb-patches/2003-08/msg00213.html
  http://sourceware.org/ml/gdb-patches/2004-02/msg00530.html
  http://sourceware.org/ml/gdb-patches/2004-05/msg00234.html

Maybe one of those threads will be enlightening on how it's "supposed"
to work.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Patch for QUIT macro support

[Fred Fish]

On Friday 13 October 2006 06:30, Daniel Jacobowitz wrote:
> Do you mean the big async patch?  If so it's still worth fixing this in
> isolation.

Yes, just search for the couple of hunks that make changes to "quit_flag".

The key difference between my patch and Nick's is the test in async_request_quit
to avoid call quit twice (once from the QUIT macro and once when the event
loop gets around to checking the signal handlers).

Here's the patch I'm now testing.

-Fred

Index: gdb/event-top.c
===================================================================
RCS file: /cvs/src/src/gdb/event-top.c,v
retrieving revision 1.46
diff -c -r1.46 event-top.c
*** gdb/event-top.c	21 Jul 2006 14:46:53 -0000	1.46
--- gdb/event-top.c	13 Oct 2006 07:49:52 -0000
***************
*** 958,963 ****
--- 1002,1017 ----
  {
    signal (sig, handle_sigint);
  
+   /* We used to set the quit flag in async_request_quit, which is either
+      called when immediate_quit is 1, or when we get back to the event
+      loop.  This is wrong, because you could be running in a loop reading
+      in symfiles or something, and it could be quite a while before you 
+      get to the event loop.  Instead, set quit_flag to 1 here, then mark
+      the sigint handler as ready.  Then if somebody calls QUIT before you
+      get to the event loop, they will unwind as expected.  */
+ 
+   quit_flag = 1;
+ 
    /* If immediate_quit is set, we go ahead and process the SIGINT right
       away, even if we usually would defer this to the event loop. The
       assumption here is that it is safe to process ^C immediately if
***************
*** 986,992 ****
  void
  async_request_quit (gdb_client_data arg)
  {
!   quit_flag = 1;
    quit ();
  }
  
--- 1040,1053 ----
  void
  async_request_quit (gdb_client_data arg)
  {
!   /* If the quit_flag has gotten reset back to 0 by the time we get
!      back here, that means that an exception was thrown to unwind
!      the current command before we got back to the event loop.  So
!      there is no reason to call quit again here. */
! 
!   if (quit_flag == 0)
!     return;
! 
    quit ();
  }
  

Re: [RFC] Patch for QUIT macro support

[Daniel Jacobowitz]

On Fri, Oct 13, 2006 at 06:48:31AM -0700, Fred Fish wrote:
> On Friday 13 October 2006 06:30, Daniel Jacobowitz wrote:
> > Do you mean the big async patch?  If so it's still worth fixing this in
> > isolation.
> 
> Yes, just search for the couple of hunks that make changes to "quit_flag".
> 
> The key difference between my patch and Nick's is the test in async_request_quit
> to avoid call quit twice (once from the QUIT macro and once when the event
> loop gets around to checking the signal handlers).
> 
> Here's the patch I'm now testing.

Hey, Fred, did anything ever come of this?


-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Patch for QUIT macro support

[Fred Fish]

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

On Sunday 21 January 2007 10:44, Daniel Jacobowitz wrote:
> > Here's the patch I'm now testing.
> 
> Hey, Fred, did anything ever come of this?

I've attached the patch we applied locally.  Other than the comment
about not putting in the comments, there weren't any other replies to
my last posting or any specific approval posted.  I got busy and
forgot about following up any more.

I guess I could clean up the comments and post again to get official
approval from another maintainer.  Should be able to do that later
this evening.

-Fred




[-- Attachment #2: p --]
[-- Type: text/x-diff, Size: 1603 bytes --]

Index: event-top.c
===================================================================
RCS file: /services/cvs/cvsroot/latest/gdb/gdb/event-top.c,v
retrieving revision 1.1.1.8
retrieving revision 1.1.1.6.2.2
diff -u -p -r1.1.1.8 -r1.1.1.6.2.2
--- event-top.c	16 Jan 2007 01:05:29 -0000	1.1.1.8
+++ event-top.c	11 Dec 2006 18:23:23 -0000	1.1.1.6.2.2
@@ -961,6 +961,16 @@ handle_sigint (int sig)
 {
   signal (sig, handle_sigint);
 
+  /* We used to set the quit flag in async_request_quit, which is either
+     called when immediate_quit is 1, or when we get back to the event
+     loop.  This is wrong, because you could be running in a loop reading
+     in symfiles or something, and it could be quite a while before you 
+     get to the event loop.  Instead, set quit_flag to 1 here, then mark
+     the sigint handler as ready.  Then if somebody calls QUIT before you
+     get to the event loop, they will unwind as expected.  */
+
+  quit_flag = 1;
+
   /* If immediate_quit is set, we go ahead and process the SIGINT right
      away, even if we usually would defer this to the event loop. The
      assumption here is that it is safe to process ^C immediately if
@@ -989,7 +999,14 @@ handle_sigterm (int sig)
 void
 async_request_quit (gdb_client_data arg)
 {
-  quit_flag = 1;
+  /* If the quit_flag has gotten reset back to 0 by the time we get
+     back here, that means that an exception was thrown to unwind
+     the current command before we got back to the event loop.  So
+     there is no reason to call quit again here. */
+
+  if (quit_flag == 0)
+    return;
+
   quit ();
 }
 

Re: [RFC] Patch for QUIT macro support

[Fred Fish]

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

On Sunday 15 October 2006 11:00, Mark Kettenis wrote:
> We really should not put history in our comments.

Good point.  Here is a patch with updated comments.

It would be good if someone could independently review, test, and
approve this, particularly anyone using an interface other than the
normal command line one.

-Fred



[-- Attachment #2: quit.diff --]
[-- Type: text/x-diff, Size: 1484 bytes --]

2007-01-22  Fred Fish  <fnf@specifix.com>

	* event-top.c (handle_sigint): Set quit_flag.
	(async_request_quit): Don't set quit_flag.  Avoid calling quit()
	if quit_flag has already been reset.

Index: event-top.c
===================================================================
RCS file: /cvs/src/src/gdb/event-top.c,v
retrieving revision 1.48
diff -u -p -r1.48 event-top.c
--- event-top.c	9 Jan 2007 17:58:50 -0000	1.48
+++ event-top.c	22 Jan 2007 14:35:14 -0000
@@ -961,6 +961,13 @@ handle_sigint (int sig)
 {
   signal (sig, handle_sigint);
 
+  /* We could be running in a loop reading in symfiles or something so
+     it may be quite a while before we get back to the event loop.  So
+     set quit_flag to 1 here. Then if QUIT is called before we get to
+     the event loop, we will unwind as expected.  */
+
+  quit_flag = 1;
+
   /* If immediate_quit is set, we go ahead and process the SIGINT right
      away, even if we usually would defer this to the event loop. The
      assumption here is that it is safe to process ^C immediately if
@@ -989,7 +996,14 @@ handle_sigterm (int sig)
 void
 async_request_quit (gdb_client_data arg)
 {
-  quit_flag = 1;
+  /* If the quit_flag has gotten reset back to 0 by the time we get
+     back here, that means that an exception was thrown to unwind
+     the current command before we got back to the event loop.  So
+     there is no reason to call quit again here. */
+
+  if (quit_flag == 0)
+    return;
+
   quit ();
 }
 

Re: [RFC] Patch for QUIT macro support

[Daniel Jacobowitz]

On Mon, Jan 22, 2007 at 07:40:52AM -0700, Fred Fish wrote:
> On Sunday 15 October 2006 11:00, Mark Kettenis wrote:
> > We really should not put history in our comments.
> 
> Good point.  Here is a patch with updated comments.
> 
> It would be good if someone could independently review, test, and
> approve this, particularly anyone using an interface other than the
> normal command line one.

> 2007-01-22  Fred Fish  <fnf@specifix.com>
> 
> 	* event-top.c (handle_sigint): Set quit_flag.
> 	(async_request_quit): Don't set quit_flag.  Avoid calling quit()
> 	if quit_flag has already been reset.

Well, I independently reviewed it - I think this is correct.  I
can't readily test it with any other interface at the moment, but I
think you should commit this.  We'll have a while to catch any problems
if there's something we didn't think of.


-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Patch for QUIT macro support

[Nick Roberts]

 > > > We really should not put history in our comments.
 > > 
 > > Good point.  Here is a patch with updated comments.
 > > 
 > > It would be good if someone could independently review, test, and
 > > approve this, particularly anyone using an interface other than the
 > > normal command line one.
 > 
 > > 2007-01-22  Fred Fish  <fnf@specifix.com>
 > > 
 > > 	* event-top.c (handle_sigint): Set quit_flag.
 > > 	(async_request_quit): Don't set quit_flag.  Avoid calling quit()
 > > 	if quit_flag has already been reset.
 > 
 > Well, I independently reviewed it - I think this is correct.  I
 > can't readily test it with any other interface at the moment, but I
 > think you should commit this.  We'll have a while to catch any problems
 > if there's something we didn't think of.

But this is really Apple's change isn't it?  Even the comments (minus history)
are Apple's.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] Patch for QUIT macro support

[Fred Fish]

On Thursday 08 February 2007 13:49, Nick Roberts wrote:

> But this is really Apple's change isn't it?  Even the comments
> (minus history) are Apple's.

Um, yes.  I'd forgotten that I snagged these two chunks out of the
attachment posted to the message at:

  http://sourceware.org/ml/gdb-patches/2006-10/msg00146.html

The comments got reworked slightly but they definitely derived from
that patch.

My sincere apologies for not giving you proper attribution, or the
original author if that's not you.

If you're still doing work in this area and the patch that got checked
in interferes with that, we can simply revert or modify it as appropriate.

-Fred

Re: [RFC] Patch for QUIT macro support

[Nick Roberts]

 > > But this is really Apple's change isn't it?  Even the comments
 > > (minus history) are Apple's.
 > 
 > Um, yes.  I'd forgotten that I snagged these two chunks out of the
 > attachment posted to the message at:
 > 
 >   http://sourceware.org/ml/gdb-patches/2006-10/msg00146.html
 > 
 > The comments got reworked slightly but they definitely derived from
 > that patch.
 > 
 > My sincere apologies for not giving you proper attribution, or the
 > original author if that's not you.

It's probably Jim Ingham's change but we won't know unless Apple tell us.
AFAIK Apple have made a general copyright assignment of all their work on
GDB to FSF.  When I used Apple's changes (2007-02-03) I just prefixed my
ChangeLog entry with:

	    Based on work by Apple Computer, Inc.

 > If you're still doing work in this area and the patch that got checked
 > in interferes with that, we can simply revert or modify it as appropriate.

It's fine with me.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] Patch for QUIT macro support

[Fred Fish]

This doesn't fix the issue with QUIT, but it does get rid of
old cruft that is no longer used.

-Fred


2006-10-13  Fred Fish  <fnf@specifix.com>

	* defs.h (request_quit): Remove declaration.
	* utils.c (request_quit): Remove definition.


Index: gdb/gdb/defs.h
===================================================================
RCS file: /services/cvs/cvsroot/latest/gdb/gdb/defs.h,v
retrieving revision 1.1.1.5.2.5
diff -u -p -r1.1.1.5.2.5 defs.h
--- gdb/gdb/defs.h	13 Oct 2006 14:39:17 -0000	1.1.1.5.2.5
+++ gdb/gdb/defs.h	13 Oct 2006 14:49:32 -0000
@@ -342,8 +342,6 @@ extern int subset_compare (char *, char 
 
 extern char *safe_strerror (int);
 
-extern void request_quit (int);
-
 #define	ALL_CLEANUPS	((struct cleanup *)0)
 
 extern void do_cleanups (struct cleanup *);
Index: gdb/gdb/utils.c
===================================================================
RCS file: /services/cvs/cvsroot/latest/gdb/gdb/utils.c,v
retrieving revision 1.1.1.5.2.2
diff -u -p -r1.1.1.5.2.2 utils.c
--- gdb/gdb/utils.c	25 Sep 2006 02:04:53 -0000	1.1.1.5.2.2
+++ gdb/gdb/utils.c	13 Oct 2006 14:49:32 -0000
@@ -906,18 +906,6 @@ quit (void)
 #endif
 }
 
-/* Control C comes here */
-void
-request_quit (int signo)
-{
-  quit_flag = 1;
-  /* Restore the signal handler.  Harmless with BSD-style signals,
-     needed for System V-style signals.  */
-  signal (signo, request_quit);
-
-  if (immediate_quit)
-    quit ();
-}
 \f
 /* Called when a memory allocation fails, with the number of bytes of
    memory requested in SIZE. */

Re: [RFC] Patch for QUIT macro support

[Daniel Jacobowitz]

On Fri, Oct 13, 2006 at 07:57:44AM -0700, Fred Fish wrote:
> This doesn't fix the issue with QUIT, but it does get rid of
> old cruft that is no longer used.
> 
> -Fred
> 
> 
> 2006-10-13  Fred Fish  <fnf@specifix.com>
> 
> 	* defs.h (request_quit): Remove declaration.
> 	* utils.c (request_quit): Remove definition.

OK (obvious, even!).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Patch for QUIT macro support

[Nick Roberts]

> > Do you mean the big async patch?  If so it's still worth fixing this in
> > isolation.

> Yes, just search for the couple of hunks that make changes to "quit_flag".

> The key difference between my patch and Nick's is the test in
> async_request_quit to avoid call quit twice (once from the QUIT macro and
> once when the event loop gets around to checking the signal handlers).

> Here's the patch I'm now testing.
> ...

Just to be clear, this part of my patch comes from changes Apple made to
their version of GDB.


-- 
Nick                                           http://www.inet.net.nz/~nickrob