Re: [PATCH] Readline: Cleanup some warnings

[Pedro Alves <palves at redhat dot com>]

On 03/20/2019 03:46 PM, Pedro Alves wrote:
> 
> Note however, that the readline signal handling code was changed
> in recent years.  IIRC, it used to do unsafe non-async-signal-safe things,
> but it's gotten better.  It's because readline signal handling changed
> that the gdb.gdb/selftest.exp fails under newer readline.  And one of
> the changes, which I think may be relevant here, is that in callback mode,
> readline no longer installs its own signal handler.
> 
> Using my "info signal-dispositions" script (*), when using the system
> readline (v7):
> 
>  (top-gdb) info signal-dispositions 2
>  Number  Name            Description                     Disposition
>  2       SIGINT          Interrupt                       handle_sigint(int) in section .text of build-sys-readline/gdb/gdb
> 
> While with the bundled readline I get:
> 
>  (top-gdb) info signal-dispositions 2
>  Number  Name            Description                     Disposition
>  2       SIGINT          Interrupt                       rl_signal_handler in section .text of build/gdb/gdb
> 
> 
> I'm not sure whether readline still installs its own signal handler
> internally in other situations, but it'd be worth it to check that.
> Maybe we never run any readline signal handler on Windows at all
> nowadays with recent readlines, which would simplify things for us,
> rendering the gdb hack in question obsolete.

I looked into this a bit more today.  Most of the time gdb's signal handlers
are active, but readline still installs its signal handlers for a bit.
E.g., rl_callback_read_char calls rl_set_signals.  However, the readline
signal handler nowadays is much simplified.

In gdb's readline bundled copy (readline 6.2), the signal handler
looks like this:

static RETSIGTYPE
rl_signal_handler (sig)
     int sig;
{
  if (_rl_interrupt_immediately || RL_ISSTATE(RL_STATE_CALLBACK))
    {
      _rl_interrupt_immediately = 0;
      _rl_handle_signal (sig);
    }
  else
    _rl_caught_signal = sig;

  SIGHANDLER_RETURN;
}


And since we use the callback interface, RL_ISSTATE(RL_STATE_CALLBACK)
will be true.  Thus we'll handle the signal immediately.
_rl_handle_signal will then change readline's state to 
RL_STATE_SIGHANDLER, and do other non-async-signal-safe things, like
calling _rl_reset_completion_state and rl_free_line_state.

But nowadays, it looks like this:

static RETSIGTYPE
rl_signal_handler (int sig)
{
  if (_rl_interrupt_immediately)
    {
      _rl_interrupt_immediately = 0;
      _rl_handle_signal (sig);
    }
  else
    _rl_caught_signal = sig;

  SIGHANDLER_RETURN;
}

the RL_ISSTATE(RL_STATE_CALLBACK) check is gone.
And also, AFAICT, _rl_interrupt_immediately is never 
set anywhere.  So in effect, AFAICS, that's the same as:

static void
rl_signal_handler (int sig)
{
  _rl_caught_signal = sig;
}

I've asked for confirmation on the readline list:
  http://lists.gnu.org/archive/html/bug-readline/2019-03/msg00005.html
(Chet already confirmed, but it may not be on the archives yet when
you read this, there's a delay).

So that means that the readline hack in mingw-hdep.c:

  /* With multi-threaded SIGINT handling, there is a race between the
     readline signal handler and GDB.  It may still be in
     rl_prep_terminal in another thread.  Do not return until it is
     done; we can check the state here because we never longjmp from
     signal handlers on Windows.  */
  while (RL_ISSTATE (RL_STATE_SIGHANDLER))
    Sleep (1);

with new-enough readline, is no longer doing anything, since the while
loop's condition is always false.

However, there's another question that needs answering: what are we going to
do if even after upgrading our readline version, someone builds gdb against
the system readline, and the system readline happens to be an older version
that still depends on this or some other readline hack?  We'll silently
regress.  I think that we need to document the minimum readline version
somewhere (gdb/README?), and have something (configure?) enforce it.
(There's a RL_READLINE_VERSION symbol.)

Thanks,
Pedro Alves