Re: [PATCH 7/8] fix py-finish-breakpoint.exp with always-async

[Doug Evans <dje at google dot com>]

Tom Tromey writes:
 > With target async enabled, py-finish-breakpoint.exp will trigger an
 > assertion failure.
 > 
 > The failure occurs because execute_command re-enters the event loop in
 > some circumstances, and in this case resets the sync_execution flag.
 > Then later gdb reaches this assertion in normal_stop:
 > 
 >       gdb_assert (sync_execution || !target_can_async_p ());
 > 
 > execute_command has a comment explaining why it dispatches events:
 > 
 >       /* If the interpreter is in sync mode (we're running a user
 > 	 command's list, running command hooks or similars), and we
 > 	 just ran a synchronous command that started the target, wait
 > 	 for that command to end.  */
 > 
 > However, the code did not follow this comment -- it didn't check to
 > see if the command started the target.
 > 
 > This patch fixes the problem by noting whether the target was
 > executing in sync_execution mode before running the command, and then
 > augmenting the condition to test this as well.
 > 
 > Built and regtested on x86-64 Fedora 18.
 > 
 > 	* top.c (execute_command): Only dispatch events if command
 > 	started target.
 > ---
 >  gdb/top.c | 5 ++++-
 >  1 file changed, 4 insertions(+), 1 deletion(-)
 > 
 > diff --git a/gdb/top.c b/gdb/top.c
 > index 467e6a2..6af0fad 100644
 > --- a/gdb/top.c
 > +++ b/gdb/top.c
 > @@ -426,6 +426,8 @@ execute_command (char *p, int from_tty)
 >      {
 >        const char *cmd = p;
 >        char *arg;
 > +      int was_executing = sync_execution && target_has_execution;
 > +
 >        line = p;
 >  
 >        /* If trace-commands is set then this will print this command.  */
 > @@ -481,7 +483,8 @@ execute_command (char *p, int from_tty)
 >  	 command's list, running command hooks or similars), and we
 >  	 just ran a synchronous command that started the target, wait
 >  	 for that command to end.  */
 > -      if (!interpreter_async && sync_execution)
 > +      if (!interpreter_async && !was_executing
 > +	  && sync_execution && target_has_execution)
 >  	{
 >  	  while (gdb_do_one_event () >= 0)
 >  	    if (!sync_execution)

Hi.
The patch is a bit confusing.
Assignment of was_execution tests for sync_execution,
and later we test sync_execution && !was_executing.

It may be correct to do it that way (I'm not sure ATM),
but the phrase "was_executing" doesn't really convey
any notion of what "sync_execution" means.

IOW, it'd be clearer if the patch did:

  int was_executing = target_has_execution;

and then augment the patch with whatever else is necessary
to make it correct.
[Assuming I understand correctly what the patch is trying to do.]

----
Also, is target_has_execution what you want here?
If I'm stopped at a breakpoint target_has_execution still returns non-zero.
E.g. do we want that while loop for a "next" command?
[where the target "has execution" before and after the command]
If that's *not* the case then that code needs a lot more commenting. :-)
Maybe I'm missing something of course.