Re: [PATCH] Readline: Cleanup some warnings

[Pedro Alves <palves@redhat.com>]

On 03/19/2019 08:14 PM, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>

> Not sure if the above refers to what I wanted to say, but: as I'm sure
> you know, SIGINT handlers on Windows run in a separate thread, created
> by the OS, so a Readline SIGINT handler could ruin in parallel both
> with Readline's other code and in parallel with GDB's code, depending
> on when exactly did the user type Ctrl-C.  In a few cases where it was
> important to emulate Posix behavior in order not to step on the troes
> of the mainline code, I needed to stop the main thread while the
> SIGINT handler was running.  Could it be that the code we are
> discussing does something similar?
> 
>> But again, why isn't that a readline problem, instead of
>> a gdb problem?
> 
> I agree: the right solution would be for the Readline's SIGINT handler
> to stop the main thread (e.g., by using SuspendThread).

I don't sure how having the SIGINT handler stop the main thread is
a 100% correct solution.  By the time you stop it, the main thread can well
be already running readline code, halfway through updating some data
structure, even if the mainline code disabled SIGINT temporarily,
with _rl_block_sigint, because by the time the mainline code calls 
_rl_block_sigint, the SIGINT thread may have already have been spawned.

Also, you're not ever supposed to use SuspendThread for synchronization, I
believe.

MSDN at <https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-suspendthread> says:

 "This function is primarily designed for use by debuggers. It is not intended
 to be used for thread synchronization."

And here <https://devblogs.microsoft.com/oldnewthing/?p=44743> it says:

  "The Suspend­Thread function tells the scheduler to suspend the thread
  but does not wait for an acknowledgment from the scheduler that the suspension
  has actually occurred."

I think a mutex/lock for synchronization would be a better solution within
readline.  Make all mainline readline entry points grab a mutex on entry
and release it on exit.  Likewise, make the readline signal handler
hold a mutex around any code that is unsafe to run in parallel
with mainline code.

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.

(*) - http://palves.net/list-active-signal-handlers-with-gdb/

Thanks,
Pedro Alves

> 
>> I'm still puzzled on why this isn't a readline issue.  Shouldn't
>> readline's Windows signal handler be synchronizing with mainline
>> code such that if a signal handler is running, mainline calls into
>> readline would block?
> 
> Yes, I think so.
> 
>> I think there must be something else to this.
> 
> Maybe.  I will try to read that discussion soon.