Re: [PATCH] Supress SIGTTOU when handling errors

[Pedro Alves <palves@redhat.com>]

On 5/16/19 7:30 PM, Andrew Burgess wrote:
> Below is one of my attempts at looking into this issue.  This isn't a
> patch for merging, it's just some random hacking at this point, but it
> shows what I'm thinking...
> 
> With this patch applied set the environment variable 'GDB_FAKE_ERROR'
> before starting GDB, set a breakpoint and run and you'll hit the
> SIGTTOU issue.
> 
>   $ GDB_FAKE_ERROR=y ./gdb ./gdb
>   (gdb) b main
>   Breakpoint 1 at 0x410236: main. (24 locations)
>   (gdb) r
>   Starting program: ....
>   Warning:
>   Cannot insert breakpoint 1: fake error
>   ...
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 395c148903ba4a96a55c9ade4b809b9df2ccbcb8
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date:   Tue Sep 4 15:44:48 2018 +0100
> 
>     Remove change of terminal ownership before throw
>     
>     Calling target_terminal::ours_for_output before throwing an error
>     seems wrong, surely the site at which the terminal is passed to the
>     inferior should take care of reclaiming the terminal using a RAII
>     object within that scope.

That's not true/correct, because run control / event handle is asynchronous.
You put the inferior in the foreground, resume the inferior, and then
go back to the event loop.  The stack frame that put the terminal
in the foreground is long gone, it isn't there to restore the terminal
state anymore.  Instead, we (should) switch the terminal back to gdb when
the target stops and/or when we give back the prompt to the user.

>     
>     If the error is caught before we reclaim the terminal _then_ we can
>     call target_terminal::ours_for_output, but I don't think we should be
>     calling this at the throw site.


"I don't think we should be calling this at the throw site"

Agreed with that part.


>     
>     Issues:
>     
>       1. Why do I need to use target_terminal::ours instead of
>       target_terminal::ours_for_output in event-loop.c???

Because target_terminal::ours_for_output does not put gdb in
the foreground -- the tcsetpgrp call in child_terminal_ours_1.

>     
>       2. The change in event-loop.c shouldn't be there anyway, I should be
>       just asserting that the terminal is owned by GDB at this point, the
>       terminal should be returned to GDB using RAII at some other point in
>       the stack (presumably the same frame level as we give up the
>       terminal).

Fully disagreed.  :-)

>     
>       3. Should be able to assert in the every output routing that GDB
>       owns the terminal - but this is broken so badly by our debug.

Right, I tried that once too, in the context of that branch I pointed at
above -- we don't switch to ours_for_output before log output.  I ended
up just shrugging and thinking that at least debug output is not meant for
the users, so if debug output comes out a bit messed up it's not _that_ bad.

>       Maybe we can get GDB to automatically reclaim the terminal before
>       writing out each debug.  I assume in some cases debug output would
>       not work due to not owning the terminal???

I think output should always work.  If you miss an ours_for_output call,
all that happens is that you end up printing in whatever terminal
mode/settings the inferior configured for the terminal, instead of in
gdb's mode/settings.  But then again, if the inferior prints at
the same time as gdb, or queries/saves the terminal settings, it'll
see gdb's settings...  That's another thing that branch fixes, because
with that branch, gdb is in control of flushing the output from the
inferior's terminal to gdb's terminal at the right times, and the
inferior's terminal settings are never tweaked by gdb.

>     
>     gdb/ChangeLog:
>     
>             * breakpoint.c (update_inserted_breakpoint_locations): Remove call
>             to target_terminal::ours_for_output before throwing an error.
>             (insert_breakpoint_locations): Likewise.
>             (bkpt_insert_location): ***REMOVE*** Add code to raise a fake
>             error.
>             * event-loop.c: Add 'target.h' include.
>             (start_event_loop): Claim terminal before printing the exception.
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 5b0a9fde61f..3a5396f7725 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,13 @@
> +2019-05-12  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	* breakpoint.c (update_inserted_breakpoint_locations): Remove call
> +	to target_terminal::ours_for_output before throwing an error.
> +	(insert_breakpoint_locations): Likewise.
> +	(bkpt_insert_location): ***REMOVE*** Add code to raise a fake
> +	error.
> +	* event-loop.c: Add 'target.h' include.
> +	(start_event_loop): Claim terminal before printing the exception.
> +
>  2019-05-08  Tom Tromey  <tom@tromey.com>
>  
>  	* gdbtypes.c (objfile_type_data): Change type.
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 35da97bd041..053d75dcfd4 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2913,10 +2913,7 @@ update_inserted_breakpoint_locations (void)
>      }
>  
>    if (error_flag)
> -    {
> -      target_terminal::ours_for_output ();
> -      error_stream (tmp_error_stream);
> -    }
> +    error_stream (tmp_error_stream);

This bit is OK.

>  }
>  
>  /* Used when starting or continuing the program.  */
> @@ -3013,7 +3010,6 @@ insert_breakpoint_locations (void)
>  	  tmp_error_stream.printf ("Could not insert hardware breakpoints:\n\
>  You may have requested too many hardware breakpoints/watchpoints.\n");
>  	}
> -      target_terminal::ours_for_output ();
>        error_stream (tmp_error_stream);

This bit is OK.

>      }
>  }
> @@ -12343,6 +12339,9 @@ bkpt_insert_location (struct bp_location *bl)
>  {
>    CORE_ADDR addr = bl->target_info.reqstd_address;
>  
> +  if (getenv ("GDB_FAKE_ERROR") != NULL)
> +    error ("fake error");
> +
>    bl->target_info.kind = breakpoint_kind (bl, &addr);
>    bl->target_info.placed_address = addr;
>  
> diff --git a/gdb/event-loop.c b/gdb/event-loop.c
> index caeb5f38d9b..611f63b5942 100644
> --- a/gdb/event-loop.c
> +++ b/gdb/event-loop.c
> @@ -21,6 +21,7 @@
>  #include "event-loop.h"
>  #include "event-top.h"
>  #include "ser-event.h"
> +#include "target.h"
>  
>  #ifdef HAVE_POLL
>  #if defined (HAVE_POLL_H)
> @@ -371,6 +372,13 @@ start_event_loop (void)
>  	}
>        catch (const gdb_exception &ex)
>  	{
> +	  /* Ideally the terminal should have been restored to GDB as part
> +	     of unwinding the stack to get back to here, but things are
> +	     not ideal.  Further, based on the name alone we should be able
> +	     to call target_terminal::ours_for_output () here, but that's
> +	     not enough, we need to call full target_terminal::ours ().  */

This comment is not OK.

> +	  target_terminal::ours ();

See comment just below exception_print.  It reads:

	  /* If any exception escaped to here, we better enable
	     stdin.  Otherwise, any command that calls async_disable_stdin,
	     and then throws, will leave stdin inoperable.  */

Also see async_enable_stdin:

/* Re-enable stdin after the end of an execution command in
   synchronous mode, or after an error from the target, and we aborted
   the exec operation.  */

void
async_enable_stdin (void)
{
  struct ui *ui = current_ui;

  if (ui->prompt_state == PROMPT_BLOCKED)
    {
      target_terminal::ours ();
      ui_register_input_event_handler (ui);
      ui->prompt_state = PROMPT_NEEDED;
    }
}

So it seems to be that we should call async_enable_stdin _before_
calling exception_print, instead of after.  Does that work?
If not, why not?

I would have tried it myself to confirm, but I couldn't reproduce
the SIGTTOU issue with your patch, for some reason.

> +
>  	  exception_print (gdb_stderr, ex);
>  
>  	  /* If any exception escaped to here, we better enable
> 

Thanks,
Pedro Alves