[RFC][PATCH 2/2][PR cli/34409] gdb: Fix readline macros hazards.

[kurku <sandovin@mail.ru>]

I was trying to fix the ux bug in a tui and started reading
GNU Readline manual. Because gdb tui and cli, uses well, GNU Readline.

This led me to discovery of recordable keyboard macros in Readline.
Which I tried to use in gdb and found it completely broken.

The not so short explanation of the fix is the following:

[1] The api model of GNU Readline callback mode with respect to
    macroinput is for the user to register the callback
    and keep it until readline hands back the control.

    The library would skim through rl_executing_macro string
    replace it with a new string from macros stack.
    And call the user program back.

    Readline thus controls the tty until it hands
    control back.

[2] To the contrary gdb has its own event loop.
    It reacts to async events when the debuggee hits a breakpoint.
    It receives input from polled file descriptors.

    And it would like to call readline for a very short time.
    So that it just processes the newly streamed in data.
    And hands control back and notify gdb back
    when exactly one command for gdb is ready e.g. `continue`.

    GDB would then hand the tty down to the debuggee: one would like
    to debug very interactive program like python[*].

    Receive and react to further events and at some point
    call readline in a stream like fashion again.

As you can suspect in case of macroinput this two worlds clash.
To see that suppose the user issues macros like "n\rn\r"
via map in his `~/.inputrc`.

# What would happen then?

Well gdb wait for events from file descriptors and would wake up
when stdin will receive some chars. It would then call a readline
via `ui->call_readline`. After that readline would discover that the
sequence that the user issued is actually a macro and would enter
`RL_STATE_MACROINPUT`.

Readline `rl_callback_read_char` would then loop until the string 'n'
is formed. This would be sent to gdb.

GDB would receive this 'n' and remove its own callback handler
in `gdb_readline_wrapper_line`. Because it needs to hand the tty
to the debuggee. It does that and goes into background.

[*] To address such usecases as debugging python gdb uses several hacks
    to remove and later reinstall its own readline callback.
    So that readline is tricked to deprep the terminal, so that gdb
    can hand it down to the debuggee.

Readline is still in `RL_STATE_MACROINPUT`, it keeps spinning
through the macros. And throws the parts away because there is no one
to call back. It then, due to the weird implementation details attempts
to read from stdin.

But tty is long time not controlled by the gdb and thus readline.
So the kernel sends us SIGTTIN. We stop here.

# My attempt to fix this

[1] We need to trick somehow readline to behave more like
    a proper streaming api. Then this weird callback pasta.

    To do that: when we remove our handler in
    `gdb_rl_callback_handler_remove`

    If we also detect that we are in a `RL_STATE_MACROINPUT`.
    We should unset this bit as well so that readline loop
    terminates right after issuing a line for us.

    When we reinstall our handler in
    `gdb_rl_callback_handler_install`
    we should recover this bit back so that readline
    would recognize it has macros to handle.

    This immediately removes SIGTTIN problems, but the ux
    is pretty bad, because what happens next is the following:

        We tricked the readline to believe it has no macroinput.
        It hands the control back.

        But gdb doesn't know anything about pending macros.
        After gdb takes the foreground it attempts to listen
        the file descriptor events. And there is nothing.

        So it just waits until the user hits any key.
        And only after that all this merry-go-round spins.

[2] Thus when we need to trick gdb now to process this bogus input.
    We do this by extending the `file_handler` struct with
    additional field `has_synthetic_input`.

    The idea here is that file descriptor may have some
    additional synthetic input associated with it.

    And so even though no actual POLLIN event fired.
    GDB should let the `file_ptr->proc` handle this synthetic input.

    We need to discover how many such synthetic inputs we have.
    Because we don't wanna waste cycles looping through a linked list.
    If there is nothing to read actually. That's why I added
    a new aggregate field to the `gdb_notifier` struct.

    Finally when we reinstall the callback in
    `gdb_rl_callback_handler_install` we also notify gdb that
    we, indeed, have pending readline macros to process.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34409
---
 gdb/event-top.c          | 35 ++++++++++++++++++++++++
 gdb/ui.h                 |  3 ++
 gdbsupport/event-loop.cc | 59 +++++++++++++++++++++++++++++++++++++++-
 gdbsupport/event-loop.h  |  4 +++
 4 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 0fb6eafec57..46a9623b988 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -369,6 +369,25 @@ gdb_rl_callback_handler_remove (void)
 {
   gdb_assert (current_ui == main_ui);
 
+  /* Readline assumes we never remove handler mid macro execution.
+     And assumes it would call us back line after line while it processes macro.
+
+     This clashes with gdb's own event loop.
+     Because readline never properly returns back the control.
+
+     And even uglier: it would make an attempt to read.
+     While tty might be handled to the debuggee (e.g. python) at the moment.
+     Which would lead to SIGTTIN immediately.
+
+     We fix it by tricking readline to assume
+     that it is not in the MACROINPUT state anymore. */
+#ifdef RL_STATE_MACROINPUT
+
+  current_ui->rl_macro_is_pending = RL_ISSTATE (RL_STATE_MACROINPUT);
+  if (current_ui->rl_macro_is_pending)
+    RL_UNSETSTATE (RL_STATE_MACROINPUT);
+#endif /* RL_STATE_MACROINPUT */
+
   rl_callback_handler_remove ();
   callback_handler_installed = false;
 }
@@ -387,6 +406,22 @@ gdb_rl_callback_handler_install (const char *prompt)
      therefore loses input.  */
   gdb_assert (!callback_handler_installed);
 
+  /* Paused macroinput (see gdb_rl_callback_handler_remove)
+     must be restored when gdb is ready to process more input from the promt.
+
+     And be processed before gdb goes into real poll/select which might be empty.
+     And would thus block and require a user to issue another char to spin up the macro.
+
+     Thus, after readline is reinstalled, we need to also notify gdb
+     that we have more macro via update_synthetic_input_fd. */
+#ifdef RL_STATE_MACROINPUT
+  if (current_ui->rl_macro_is_pending)
+    {
+      RL_SETSTATE (RL_STATE_MACROINPUT);
+      notify_has_synthetic_input(current_ui->input_fd);
+    }
+#endif /* RL_STATE_MACROINPUT */
+
 #ifdef RL_STATE_EOF
   /* Some versions of readline contain a bug where the rl_eof_found flag
      would not be reset back to 0 in rl_initialize, despite the
diff --git a/gdb/ui.h b/gdb/ui.h
index 891660896ef..be3a0e5fe14 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -154,6 +154,9 @@ struct ui
      execution.  */
   bool keep_prompt_blocked = false;
 
+  /* Whether there is more readline macro to proceed */
+  bool rl_macro_is_pending;
+
   /* A "smart pointer" that references a particular member of the
      current UI.  */
   template<ui_file *ui::* F>
diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index deca3d86e5d..57f80fae01d 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -72,6 +72,10 @@ struct file_handler
   /* If set, this file descriptor is used for a user interface.  */
   bool is_ui;
 
+  /* If set, this indicates this file has an associated synthetic input
+     to be processed */
+  bool has_synthetic_input;
+
   /* Was an error detected on this fd?  */
   int error;
 
@@ -140,6 +144,9 @@ static struct
 
     /* Flag to tell whether the timeout should be used.  */
     int timeout_valid;
+
+    /* Flag to tell how many files have pending synthetic input to be processed */
+    int synthetic_input_events;
   }
 gdb_notifier;
 
@@ -327,6 +334,7 @@ create_file_handler (int fd, int mask, handler_func * proc,
       file_ptr = new file_handler;
       file_ptr->fd = fd;
       file_ptr->ready_mask = 0;
+      file_ptr->has_synthetic_input = false;
       file_ptr->next_file = gdb_notifier.first_file_handler;
       gdb_notifier.first_file_handler = file_ptr;
 
@@ -396,6 +404,35 @@ get_next_file_handler_to_handle_and_advance (void)
   return curr_next;
 }
 
+static void
+update_synthetic_input (file_handler *file_ptr, bool has_synthetic_input)
+{
+  if (file_ptr->has_synthetic_input < has_synthetic_input)
+    gdb_notifier.synthetic_input_events++;
+  else if (file_ptr->has_synthetic_input > has_synthetic_input)
+    gdb_notifier.synthetic_input_events--;
+
+  file_ptr->has_synthetic_input = has_synthetic_input;
+}
+
+void notify_has_synthetic_input (int fd)
+{
+  file_handler *file_ptr;
+
+  for (file_ptr = gdb_notifier.first_file_handler;
+      file_ptr != NULL;
+      file_ptr = file_ptr->next_file)
+    {
+      if (file_ptr->fd == fd)
+	break;
+    }
+
+  if (file_ptr == NULL)
+    return;
+  update_synthetic_input (file_ptr, true);
+}
+
+
 /* Remove the file descriptor FD from the list of monitored fd's:
    i.e. we don't care anymore about events on the FD.  */
 void
@@ -416,6 +453,10 @@ delete_file_handler (int fd)
   if (file_ptr == NULL)
     return;
 
+  /* Reset synthetic input events number agregate */
+  if (file_ptr->has_synthetic_input)
+    gdb_notifier.synthetic_input_events -= 1;
+
 #ifdef HAVE_POLL
   if (use_poll)
     {
@@ -564,6 +605,12 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
       event_loop_ui_debug_printf (file_ptr->is_ui,
 				  "invoking fd file handler `%s`",
 				  file_ptr->name.c_str ());
+
+      /* Clear synthetic input flag now
+	 because the proc would handle it
+	 and set it back if there is more. */
+      if (file_ptr->has_synthetic_input)
+	update_synthetic_input(file_ptr, false);
       file_ptr->proc (file_ptr->error, file_ptr->client_data);
     }
 }
@@ -587,6 +634,10 @@ gdb_wait_for_event (int block)
   if (gdb_notifier.num_fds == 0)
     return -1;
 
+  /* If we have synthetic input available we should not block */
+  if (gdb_notifier.synthetic_input_events > 0)
+    block = 0;
+
   if (block)
     update_wait_timeout ();
 
@@ -647,7 +698,9 @@ gdb_wait_for_event (int block)
     }
 
   /* Avoid looking at poll_fds[i]->revents if no event fired.  */
-  if (num_found <= 0)
+  if (num_found < 0)
+    return 0;
+  if (num_found + gdb_notifier.synthetic_input_events == 0)
     return 0;
 
   /* Run event handlers.  We always run just one handler and go back
@@ -670,6 +723,8 @@ gdb_wait_for_event (int block)
 	  int i = file_ptr->poll_fds_index;
 	  short revents = gdb_notifier.poll_fds[i].revents;
 
+	  if (file_ptr->has_synthetic_input)
+	    mask |= POLLIN;
 	  if (revents)
 	    mask |= revents;
 	}
@@ -688,6 +743,8 @@ gdb_wait_for_event (int block)
 	{
 	  file_ptr = get_next_file_handler_to_handle_and_advance ();
 
+	  if (file_ptr->has_synthetic_input)
+	    mask |= GDB_READABLE;
 	  if (FD_ISSET (file_ptr->fd, &gdb_notifier.ready_masks[0]))
 	    mask |= GDB_READABLE;
 	  if (FD_ISSET (file_ptr->fd, &gdb_notifier.ready_masks[1]))
diff --git a/gdbsupport/event-loop.h b/gdbsupport/event-loop.h
index 85cae592965..8b0fdb19915 100644
--- a/gdbsupport/event-loop.h
+++ b/gdbsupport/event-loop.h
@@ -97,6 +97,10 @@ extern int create_timer (int milliseconds,
 			 gdb_client_data client_data);
 extern void delete_timer (int id);
 
+/* Notifies gdb that there is more synthetic input associated with fd */
+
+extern void notify_has_synthetic_input (int fd);
+
 /* Must be defined by client.  */
 
 extern void handle_event_loop_exception (const gdb_exception &);
-- 
2.43.0