Re: [PATCH] Supress SIGTTOU when handling errors

[Andrew Burgess <andrew.burgess@embecosm.com>]

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.
    
    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.
    
    Issues:
    
      1. Why do I need to use target_terminal::ours instead of
      target_terminal::ours_for_output in event-loop.c???
    
      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).
    
      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.
      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???
    
    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);
 }
 
 /* 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);
     }
 }
@@ -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 ().  */
+	  target_terminal::ours ();
+
 	  exception_print (gdb_stderr, ex);
 
 	  /* If any exception escaped to here, we better enable