[RFA] bpstat_do_actions in one place

[RFA] bpstat_do_actions in one place

[Vladimir Prus]

This patch, to be applied on top of the "Use observers to report stop events."
make async code call bpstat_do_action in the event handler, to make sure that
it's always called.

OK?

- Volodya

---
 gdb/inf-loop.c |    7 +++++++
 gdb/top.c      |    2 --
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/inf-loop.c b/gdb/inf-loop.c
index 4c61dae..3b18abb 100644
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -43,6 +43,7 @@ void
 inferior_event_handler (enum inferior_event_type event_type, 
 			gdb_client_data client_data)
 {
+  struct gdb_exception e;
   int was_sync = 0;
   switch (event_type)
     {
@@ -91,6 +92,12 @@ inferior_event_handler (enum inferior_event_type event_type,
       was_sync = sync_execution;
       async_enable_stdin ();
 
+      /* If there's an error doing breakpoint commands, we don't
+	 want to throw -- continuation might still do something.  */
+      TRY_CATCH (e, RETURN_MASK_ERROR)
+	{
+	  bpstat_do_actions (&stop_bpstat);
+	}
       /* If we were doing a multi-step (eg: step n, next n), but it
 	 got interrupted by a breakpoint, still do the pending
 	 continuations.  The continuation itself is responsible for
diff --git a/gdb/top.c b/gdb/top.c
index 2454d24..feefcf0 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -379,8 +379,6 @@ command_line_handler_continuation (struct continuation_arg *arg, int error)
   if (error)
     return;
 
-  bpstat_do_actions (&stop_bpstat);
-
   if (display_time)
     {
       long cmd_time = get_run_time () - time_at_cmd_start;
-- 
1.5.3.5

Re: [RFA] bpstat_do_actions in one place

[Daniel Jacobowitz]

On Thu, Apr 24, 2008 at 07:02:03PM +0300, Vladimir Prus wrote:
> 
> This patch, to be applied on top of the "Use observers to report stop events."
> make async code call bpstat_do_action in the event handler, to make sure that
> it's always called.
> 
> OK?

OK.

> +      /* If there's an error doing breakpoint commands, we don't
> +	 want to throw -- continuation might still do something.  */
> +      TRY_CATCH (e, RETURN_MASK_ERROR)
> +	{
> +	  bpstat_do_actions (&stop_bpstat);
> +	}

Will things go bad if RETURN_QUIT?  I discovered that we don't handle
quit very well from some places.

Easy to trigger; put a command that generates lots of paginated output
in the commands list and press q at the page prompt.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] bpstat_do_actions in one place

[Vladimir Prus]

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

On Friday 02 May 2008 06:57:53 Daniel Jacobowitz wrote:
> On Thu, Apr 24, 2008 at 07:02:03PM +0300, Vladimir Prus wrote:
> > 
> > This patch, to be applied on top of the "Use observers to report stop events."
> > make async code call bpstat_do_action in the event handler, to make sure that
> > it's always called.
> > 
> > OK?
> 
> OK.
> 
> > +      /* If there's an error doing breakpoint commands, we don't
> > +	 want to throw -- continuation might still do something.  */
> > +      TRY_CATCH (e, RETURN_MASK_ERROR)
> > +	{
> > +	  bpstat_do_actions (&stop_bpstat);
> > +	}
> 
> Will things go bad if RETURN_QUIT?  I discovered that we don't handle
> quit very well from some places.
> 
> Easy to trigger; put a command that generates lots of paginated output
> in the commands list and press q at the page prompt.

In fact, things will go bad. I've checked in the following version of the
patch that uses RETURN_MASK_ALL.

- Volodya


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

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.9373
diff -u -p -r1.9373 ChangeLog
--- gdb/ChangeLog	4 May 2008 22:49:47 -0000	1.9373
+++ gdb/ChangeLog	5 May 2008 09:01:57 -0000
@@ -1,3 +1,10 @@
+2008-05-05  Vladimir Prus  <vladimir@codesourcery.com>
+
+	* inf-loop.c (inferior_event_handler): Call bpstat_do_action,
+	and catch all exceptions from it.
+	* top.c (command_line_handler_continuation): Don't
+	call bpstat_do_action here.
+
 2008-05-04  Daniel Jacobowitz  <dan@codesourcery.com>
 
 	* dwarf2read.c (struct dwarf2_cu): Add type_hash.
Index: gdb/inf-loop.c
===================================================================
RCS file: /cvs/src/src/gdb/inf-loop.c,v
retrieving revision 1.13
diff -u -p -r1.13 inf-loop.c
--- gdb/inf-loop.c	24 Apr 2008 11:13:44 -0000	1.13
+++ gdb/inf-loop.c	5 May 2008 09:01:57 -0000
@@ -43,6 +43,7 @@ void
 inferior_event_handler (enum inferior_event_type event_type, 
 			gdb_client_data client_data)
 {
+  struct gdb_exception e;
   int was_sync = 0;
   switch (event_type)
     {
@@ -91,6 +92,12 @@ inferior_event_handler (enum inferior_ev
       was_sync = sync_execution;
       async_enable_stdin ();
 
+      /* If there's an error doing breakpoint commands, we don't
+	 want to throw -- continuation might still do something.  */
+      TRY_CATCH (e, RETURN_MASK_ALL)
+	{
+	  bpstat_do_actions (&stop_bpstat);
+	}
       /* If we were doing a multi-step (eg: step n, next n), but it
 	 got interrupted by a breakpoint, still do the pending
 	 continuations.  The continuation itself is responsible for
Index: gdb/top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.139
diff -u -p -r1.139 top.c
--- gdb/top.c	24 Apr 2008 11:13:44 -0000	1.139
+++ gdb/top.c	5 May 2008 09:01:57 -0000
@@ -379,8 +379,6 @@ command_line_handler_continuation (struc
   if (error)
     return;
 
-  bpstat_do_actions (&stop_bpstat);
-
   if (display_time)
     {
       long cmd_time = get_run_time () - time_at_cmd_start;

Re: [RFA] bpstat_do_actions in one place

[Daniel Jacobowitz]

On Mon, May 05, 2008 at 01:04:05PM +0400, Vladimir Prus wrote:
> In fact, things will go bad. I've checked in the following version of the
> patch that uses RETURN_MASK_ALL.

RETURN_MASK_ALL is almost never a good idea.  Quit will now not quit
all the way back to a prompt.  Is that really what we want either?

For instance, a program running in a while (1) loop with a breakpoint
set in it, and the breakpoint has a command that produces output, like
backtrace.  Quit will now resume the program and generate another
backtrace, I think.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] bpstat_do_actions in one place

[Vladimir Prus]

On Monday 05 May 2008 15:48:24 Daniel Jacobowitz wrote:
> On Mon, May 05, 2008 at 01:04:05PM +0400, Vladimir Prus wrote:
> > In fact, things will go bad. I've checked in the following version of the
> > patch that uses RETURN_MASK_ALL.
> 
> RETURN_MASK_ALL is almost never a good idea.  Quit will now not quit
> all the way back to a prompt.  Is that really what we want either?

Well, this was the behaviour I though to be the most reasonable. 

> For instance, a program running in a while (1) loop with a breakpoint
> set in it, and the breakpoint has a command that produces output, like
> backtrace.  Quit will now resume the program and generate another
> backtrace, I think.

Yes, I think so; seems fine to me. Is there any documentation for how
'quit' should generally work, especially inside breakpoint commands?

- Volodya

 

Re: [RFA] bpstat_do_actions in one place

[Daniel Jacobowitz]

On Mon, May 05, 2008 at 04:58:31PM +0400, Vladimir Prus wrote:
> Well, this was the behaviour I though to be the most reasonable. 

OK.  If we want to change it, someone gets the lousy job of going
through all the call sites.  I had to fix up a bunch of them recently.

> > For instance, a program running in a while (1) loop with a breakpoint
> > set in it, and the breakpoint has a command that produces output, like
> > backtrace.  Quit will now resume the program and generate another
> > backtrace, I think.
> 
> Yes, I think so; seems fine to me. Is there any documentation for how
> 'quit' should generally work, especially inside breakpoint commands?

It's only a problem because there's no way to stop it.  When the
prompt is displayed C-c will not stop the application (since it's
already stopped), and typing q will stop the current backtrace and
resume the application.  Very hard to get out of it.

No, there isn't any documentation for how this should work, yet.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] bpstat_do_actions in one place

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Mon, 5 May 2008 16:58:31 +0400
> Cc: gdb-patches@sources.redhat.com
> 
> Is there any documentation for how
> 'quit' should generally work, especially inside breakpoint commands?

"quit" is documented to exit GDB.  What else is there to say?

Re: [RFA] bpstat_do_actions in one place

[Daniel Jacobowitz]

On Mon, May 05, 2008 at 10:17:26PM +0300, Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Mon, 5 May 2008 16:58:31 +0400
> > Cc: gdb-patches@sources.redhat.com
> > 
> > Is there any documentation for how
> > 'quit' should generally work, especially inside breakpoint commands?
> 
> "quit" is documented to exit GDB.  What else is there to say?

Sorry, quit at the "hit enter for more" prompt, not the quit command.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] bpstat_do_actions in one place

[Eli Zaretskii]

> Date: Mon, 5 May 2008 15:23:15 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Vladimir Prus <vladimir@codesourcery.com>,
> 	gdb-patches@sources.redhat.com
> 
> On Mon, May 05, 2008 at 10:17:26PM +0300, Eli Zaretskii wrote:
> > > From: Vladimir Prus <vladimir@codesourcery.com>
> > > Date: Mon, 5 May 2008 16:58:31 +0400
> > > Cc: gdb-patches@sources.redhat.com
> > > 
> > > Is there any documentation for how
> > > 'quit' should generally work, especially inside breakpoint commands?
> > 
> > "quit" is documented to exit GDB.  What else is there to say?
> 
> Sorry, quit at the "hit enter for more" prompt, not the quit command.

    Certain commands to GDB may produce large amounts of information output
    to the screen.  To help you read all of it, GDB pauses and asks you for
    input at the end of each page of output.  Type <RET> when you want to
    continue the output, or `q' to discard the remaining output.

So it "discards the remaining output".  But this `q' is not a command,
just a one-letter response (GDB doesn't care what follows the initial
`q'), just like it's sibling <RET> isn't a command, so there's no
reason to expect that it will have _any_ effect inside breakpoint
commands, or in any other context where GDB expects a command.  Am I
missing something?

Re: [RFA] bpstat_do_actions in one place

[Daniel Jacobowitz]

On Mon, May 05, 2008 at 10:38:27PM +0300, Eli Zaretskii wrote:
> So it "discards the remaining output".  But this `q' is not a command,
> just a one-letter response (GDB doesn't care what follows the initial
> `q'), just like it's sibling <RET> isn't a command, so there's no
> reason to expect that it will have _any_ effect inside breakpoint
> commands, or in any other context where GDB expects a command.  Am I
> missing something?

I wasn't worried about embedding the q in the breakpoint commands
list, but about what effect it has when the prompt is triggered by
a command in the breakpoint command list.  Basically, how far up
the call stack is considered "the remaining output"?  Since what
it actually does is abort the command, not just discard the output.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] bpstat_do_actions in one place

[Eli Zaretskii]

> Date: Mon, 5 May 2008 15:41:30 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: vladimir@codesourcery.com, gdb-patches@sources.redhat.com
> 
> I wasn't worried about embedding the q in the breakpoint commands
> list, but about what effect it has when the prompt is triggered by
> a command in the breakpoint command list.  Basically, how far up
> the call stack is considered "the remaining output"?  Since what
> it actually does is abort the command, not just discard the output.

Okay, but is there a real problem here?  That is, can the user really
do something like that from inside breakpoint commands?  I think it's
not possible, since `q' is not a command.  Am I right?  If not, please
show an example of doing this from breakpoint commands, because I
cannot imagine a use case.

Re: [RFA] bpstat_do_actions in one place

[Daniel Jacobowitz]

On Tue, May 06, 2008 at 06:14:44AM +0300, Eli Zaretskii wrote:
> > Date: Mon, 5 May 2008 15:41:30 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: vladimir@codesourcery.com, gdb-patches@sources.redhat.com
> > 
> > I wasn't worried about embedding the q in the breakpoint commands
> > list, but about what effect it has when the prompt is triggered by
> > a command in the breakpoint command list.  Basically, how far up
> > the call stack is considered "the remaining output"?  Since what
> > it actually does is abort the command, not just discard the output.
> 
> Okay, but is there a real problem here?  That is, can the user really
> do something like that from inside breakpoint commands?  I think it's
> not possible, since `q' is not a command.  Am I right?  If not, please
> show an example of doing this from breakpoint commands, because I
> cannot imagine a use case.

Here's the example from upthread with more detail.

int foo()
{
  while (1)
    /* Nothing on line 4 */ ;
}

int main() { foo (); }

(gdb) break 4
(gdb) commands
> backtrace
> continue
> end

This loop is tricky to interrupt.  But I see now that the entire
commands list will still be interrupted by quit so we will not execute
the continue.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] bpstat_do_actions in one place

[Eli Zaretskii]

> Date: Tue, 6 May 2008 08:43:22 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: vladimir@codesourcery.com, gdb-patches@sources.redhat.com
> 
> Here's the example from upthread with more detail.

Thanks.

> (gdb) break 4
> (gdb) commands
> > backtrace

And `q' here will be typed by hand, assuming that the backtrace is
longer than one page?

> This loop is tricky to interrupt.  But I see now that the entire
> commands list will still be interrupted by quit so we will not execute
> the continue.

Sounds like a bug to me.  At least I won't expect it to do that.

Re: [RFA] bpstat_do_actions in one place

[Daniel Jacobowitz]

On Tue, May 06, 2008 at 09:30:22PM +0300, Eli Zaretskii wrote:
> > (gdb) break 4
> > (gdb) commands
> > > backtrace
> 
> And `q' here will be typed by hand, assuming that the backtrace is
> longer than one page?

Pagination length queues up between prompts.  So even if the backtrace
is only a few lines, after a dozen times around the loop we'll prompt.

> > This loop is tricky to interrupt.  But I see now that the entire
> > commands list will still be interrupted by quit so we will not execute
> > the continue.
> 
> Sounds like a bug to me.  At least I won't expect it to do that.

What would you expect to happen?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] bpstat_do_actions in one place

[Eli Zaretskii]

> Date: Tue, 6 May 2008 14:39:09 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: vladimir@codesourcery.com, gdb-patches@sources.redhat.com
> 
> > > This loop is tricky to interrupt.  But I see now that the entire
> > > commands list will still be interrupted by quit so we will not execute
> > > the continue.
> > 
> > Sounds like a bug to me.  At least I won't expect it to do that.
> 
> What would you expect to happen?

That this instance of `backtrace' be aborted, and the loop will
continue.

Re: [RFA] bpstat_do_actions in one place

[Daniel Jacobowitz]

On Wed, May 07, 2008 at 11:02:38AM +0300, Eli Zaretskii wrote:
> That this instance of `backtrace' be aborted, and the loop will
> continue.

Interesting.  That's not what I would expect; for instance, I'd
expect an entire user-defined ("define") command to be aborted.

If that happened, how would you stop the example I gave?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] bpstat_do_actions in one place

[Eli Zaretskii]

> Date: Wed, 7 May 2008 07:37:36 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: vladimir@codesourcery.com, gdb-patches@sources.redhat.com
> 
> On Wed, May 07, 2008 at 11:02:38AM +0300, Eli Zaretskii wrote:
> > That this instance of `backtrace' be aborted, and the loop will
> > continue.
> 
> Interesting.  That's not what I would expect; for instance, I'd
> expect an entire user-defined ("define") command to be aborted.

What user-defined command?  The example was about breakpoint commands,
not about a user-defined command defined by "define", wasn't it?

> If that happened, how would you stop the example I gave?

How would one stop _any_ endless loop?  Ctrl-C, I guess.

Anyway, I don't think `q's behavior should cater first and foremost to
endless loop situations.  Its original intent is save me the time it
takes GDB to emit output I don't want to see, plus the time and
nuisance of typing RET while it does.

Re: [RFA] bpstat_do_actions in one place

[Daniel Jacobowitz]

On Wed, May 07, 2008 at 03:55:42PM +0300, Eli Zaretskii wrote:
> > On Wed, May 07, 2008 at 11:02:38AM +0300, Eli Zaretskii wrote:
> > > That this instance of `backtrace' be aborted, and the loop will
> > > continue.
> > 
> > Interesting.  That's not what I would expect; for instance, I'd
> > expect an entire user-defined ("define") command to be aborted.
> 
> What user-defined command?  The example was about breakpoint commands,
> not about a user-defined command defined by "define", wasn't it?

They're all the same mechanism.  For instance,

define sillybt
backtrace
backtrace
end

If q should stop backtrace, IMO it should stop sillybt too.

> > If that happened, how would you stop the example I gave?
> 
> How would one stop _any_ endless loop?  Ctrl-C, I guess.

But it won't work.  C-c is implemented as the exact same thing as
typing q at a pagination prompt: eventually a quit exception is
thrown.  So you will only stop the currently printing backtrace and
things will keep on going.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] bpstat_do_actions in one place

[Eli Zaretskii]

> Date: Wed, 7 May 2008 15:50:24 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: vladimir@codesourcery.com, gdb-patches@sources.redhat.com
> 
> > > If that happened, how would you stop the example I gave?
> > 
> > How would one stop _any_ endless loop?  Ctrl-C, I guess.
> 
> But it won't work.  C-c is implemented as the exact same thing as
> typing q at a pagination prompt: eventually a quit exception is
> thrown.  So you will only stop the currently printing backtrace and
> things will keep on going.

<Shrug> Another bug, I guess.  We should be throwing to top level.
And no, I don't think `q' should do the same as Ctrl-C.