[RFC][PATCH 0/2][PR cli/34409] Fix SIGTTIN due to readline macroinput.

[RFC][PATCH 0/2][PR cli/34409] Fix SIGTTIN due to readline macroinput.

[kurku]

The two patches fix (gdb:cli/34409): [gdb receives SIGTTIN when user uses readline macros]

It adds support of any kind of readline macros: the one defined by the user in his ~/.inputrc 
or the one, for example, recorded at a debug scene with Cx-( ... Cx-) Cx-e -- into gdb.

[2] I managed to do so by a slight extension of the current event-loop over file 
    descriptors. Now, apart from direct events like the ones from poll api, 
    file descriptors also can specify that they have a `synthetic input` 
    pending associated with them.

[1] It was easy to integrate this new behavior into select round robin, but not poll.
    So I reworked the poll's round robin so that it looks like select's version now.

I ran the test-suite via `make check -j$(nproc)` on x86-64 Linux under `guix shell -D gdb clang-toolchain`.
There were unexpected failures fluctuating around 456 in gdb, but no visible regressions between 
the original master and my patches on top of it.

I have not previously completed any FSF copyright assignment
This is my first contribution to the project.

kurku (2):
  gdb: event-loop: Rework poll's version of round robin
  gdb: Fix readline macros hazards.

 gdb/event-top.c          |  35 +++++++++++
 gdb/ui.h                 |   3 +
 gdbsupport/event-loop.cc | 131 +++++++++++++++++++++++++++++----------
 gdbsupport/event-loop.h  |   4 ++
 4 files changed, 139 insertions(+), 34 deletions(-)

-- 
2.43.0

[RFC][PATCH 1/2][PR cli/34409] gdb: event-loop: Rework poll's version of round robin

[kurku]

GDB had 2 implementations for round robin over file descriptors.

version for poll
version for select

poll version uses:
 - std::vector index in gdb_notifier
 - linked list from gdb_notifier
it does:
 - circular scan over std::vector to find a slot
   with revents filled in by the kernel
 - additional linear scan of the linked list to find
   the matched file_handler with the same fd

select version uses:
 - linked list in gdb_notifier
it does:
 - circular scan of the linked list
   to find the node which has events

I can't find no clear reason to distinguish the two, or make the shape
of the code to be so vastly different. In both cases we do circular
linear scan over something and if flag of the fired event has been set -
we stop the scan and handle the event.

The only difference is the source of events. One comes directly from the
linked list. The other is from poll_fds std::vector.

Thus I propose to unify the shape of code paths. And unification would
be made on select's version because it already does the tracking of
events for select, we just need to extend it so that now revents issued
by poll api are accessible through the linked list.

( the real use case to do this change
  is due to many readline macros hazards
  I was trying to fix: gdb/34409 )
---
 gdbsupport/event-loop.cc | 72 ++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 33 deletions(-)

diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index c6e1b5b4634..deca3d86e5d 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -77,6 +77,11 @@ struct file_handler
 
   /* Next registered file descriptor.  */
   struct file_handler *next_file;
+
+#ifdef HAVE_POLL
+  /* Reverse index from linked list into vector poll_fds vector */
+  int poll_fds_index;
+#endif
 };
 
 #ifdef HAVE_POLL
@@ -104,7 +109,7 @@ static struct
     /* Ptr to head of file handler list.  */
     file_handler *first_file_handler;
 
-    /* Next file handler to handle, for the select variant.  To level
+    /* Next file handler to handle.  To level
        the fairness across event sources, we serve file handlers in a
        round-robin-like fashion.  The number and order of the polled
        file handlers may change between invocations, but this is good
@@ -115,13 +120,6 @@ static struct
     /* Descriptors to poll.  */
     std::vector<struct pollfd> poll_fds;
 
-    /* Next file descriptor to handle, for the poll variant.  To level
-       the fairness across event sources, we poll the file descriptors
-       in a round-robin-like fashion.  The number and order of the
-       polled file descriptors may change between invocations, but
-       this is good enough.  */
-    int next_poll_fds_index;
-
     /* Timeout in milliseconds for calls to poll().  */
     int poll_timeout;
 #endif
@@ -335,6 +333,7 @@ create_file_handler (int fd, int mask, handler_func * proc,
 #ifdef HAVE_POLL
       if (use_poll)
 	{
+	  file_ptr->poll_fds_index = gdb_notifier.num_fds;
 	  gdb_notifier.num_fds++;
 	  struct pollfd new_fd;
 	  new_fd.fd = fd;
@@ -420,13 +419,30 @@ delete_file_handler (int fd)
 #ifdef HAVE_POLL
   if (use_poll)
     {
-      auto iter = std::remove_if (gdb_notifier.poll_fds.begin (),
-				  gdb_notifier.poll_fds.end (),
-				  [=] (const pollfd &item)
-				  {
-				    return item.fd == fd;
-				  });
-      gdb_notifier.poll_fds.erase (iter, gdb_notifier.poll_fds.end());
+      /* Swap last with the provided file_ptr in poll_fds vector
+	 then do repair of a reverse index */
+
+      int i = file_ptr->poll_fds_index;
+      int j = gdb_notifier.num_fds - 1;
+
+      file_handler *i_ptr = file_ptr;
+      file_handler *j_ptr;
+
+      for (j_ptr = gdb_notifier.first_file_handler;
+	   j_ptr != NULL;
+	   j_ptr = j_ptr->next_file)
+	{
+	  if (j_ptr->poll_fds_index == j)
+	    break;
+	}
+
+      gdb_assert (i_ptr != NULL);
+      gdb_assert (j_ptr != NULL);
+
+      std::swap(gdb_notifier.poll_fds[i], gdb_notifier.poll_fds[j]);
+      std::swap(j_ptr->poll_fds_index, i_ptr->poll_fds_index);
+
+      gdb_notifier.poll_fds.pop_back();
       gdb_notifier.num_fds--;
     }
   else
@@ -645,30 +661,20 @@ gdb_wait_for_event (int block)
 #ifdef HAVE_POLL
   if (use_poll)
     {
-      int i;
-      int mask;
+      int mask = 0;
 
-      while (1)
+      do
 	{
-	  if (gdb_notifier.next_poll_fds_index >= gdb_notifier.num_fds)
-	    gdb_notifier.next_poll_fds_index = 0;
-	  i = gdb_notifier.next_poll_fds_index++;
+	  file_ptr = get_next_file_handler_to_handle_and_advance ();
 
-	  gdb_assert (i < gdb_notifier.num_fds);
-	  if (gdb_notifier.poll_fds[i].revents)
-	    break;
-	}
+	  int i = file_ptr->poll_fds_index;
+	  short revents = gdb_notifier.poll_fds[i].revents;
 
-      for (file_ptr = gdb_notifier.first_file_handler;
-	   file_ptr != NULL;
-	   file_ptr = file_ptr->next_file)
-	{
-	  if (file_ptr->fd == gdb_notifier.poll_fds[i].fd)
-	    break;
+	  if (revents)
+	    mask |= revents;
 	}
-      gdb_assert (file_ptr != NULL);
+      while (mask == 0);
 
-      mask = gdb_notifier.poll_fds[i].revents;
       handle_file_event (file_ptr, mask);
       return 1;
     }
-- 
2.43.0

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

[kurku]

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