Re: [PATCH] Readline: Cleanup some warnings

[Pedro Alves <palves@redhat.com>]

On 03/20/2019 05:38 PM, Eli Zaretskii wrote:
>> Cc: tom@tromey.com, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Wed, 20 Mar 2019 15:46:47 +0000
>>
>>> 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.
> 
> How is this different from what happens on Posix platforms, where the
> SIGINT handler can be invoked at any moment, while Readline might be
> doing anything at all?

On Posix platforms the signal handler runs in the same thread as the
mainline code.  There's _never_ any parallel execution.  When the
mainline code calls _rl_block_sigint, you're absolutely sure that the
mainline code that follows will not be running in parallel with
a SIGINT handler.

> 
>> Also, you're not ever supposed to use SuspendThread for synchronization, I
>> believe.
> 
> We are also not supposed to mix CRT and Win32 API functions, but we do
> that all the time.

That's not at all the same thing, I'm afraid.  We're not talking about
"in theory you shouldn't, but in practice it's OK.".  We're talking about
the very real fact that you cannot use it to synchronize correctly,
as shown in the blog post I pasted below.  If you're using the function
to pause a thread, and then do things, but the thread doesn't actually
pause before you do things, then you have a problem.

> 
>> 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."
> 
> GNU Make does it for many years, and I have yet to hear a single
> complaint about that part.

Not hearing a complaint does not mean that the problem isn't real.
It probably simply means that the race window is narrow, so the number
of people that observe any issue, _and_ are willing to report an issue
is small, and also, is someone notices something odd, it may be simply
impossible to tell what happened and realize what the problem was,
after the fact.

> 
>> 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.
> 
> That's fine with me, but is much more complex, and will probably slow
> down the code.  I won't object if you want to do it that way, though.
> 
>> 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.
> 
> Maybe we should bring Chet into this discussion.

Thanks,
Pedro Alves