This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Readline: Cleanup some warnings


On 03/19/2019 04:04 PM, Tom Tromey wrote:
>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
>>> From: Tom Tromey <tom@tromey.com>
>>> Cc: "gdb-patches\@sourceware.org" <gdb-patches@sourceware.org>
>>> Date: Sun, 17 Mar 2019 11:30:38 -0600
>>>
>>> I still don't really know what to do about the readline-related hack in
>>> the mingw gdb_select.  I'd like to remove it, but I can't test it and I
>>> don't know whether it's still needed.
> 
> Eli> Can someone point me to the PR that led to that hack?  I'd like to see
> Eli> what happens in that use case and why is this code needed to solve it.
> Eli> (I tried "git annotate", but that didn't tell me anything interesting
> Eli> about the history of that snippet.  Apologies if I missed something.)
> 
> All I know is what annotate says.  The commit message is appended.
> 
> Here's the gdb-patches thread about it:
> 
> https://sourceware.org/ml/gdb-patches/2008-02/msg00423.html

Hmmm, 

Daniel wrote:

> GDB has several SIGINT handlers which call longjmp.  This is
> problematic for at least two reasons.  One is that we could be in the
> middle of something unwise to longjmp out of, for instance malloc.  In
> practice, this never happens because we're usually waiting for I/O
> when one of the relevant handlers is invoked, but there are a number
> of places where it could definitely happen.

That was indeed true back then, but since then, immediate_quit
was completely eliminated, and we no longer longjmp from signal
handlers anymore, since:
 https://sourceware.org/ml/gdb-patches/2016-03/msg00351.html

Daniel wrote:

> My goals in fixing this were to hide the Windows ugliness, and to fit
> in nicely with GDB's asynchronous event loop.  Since we do not return
> to the primary event loop during target actions (for the current,
> non-async GDB), I couldn't rely on the event loop entirely.  But I
> could use the same token mechanism and thus share the bodies of
> handlers for async mode with the Windows case.
>
> The new interface is gdb_call_async_signal_handler.  SIGINT handlers,

This interface he mentioned, gdb_call_async_signal_handler, was
eliminated by that series too:

 https://sourceware.org/ml/gdb-patches/2016-03/msg00347.html

So all that's left is that little readline hack, it seems:

  /* 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);

(Curiously, that bit only appeared in a later version of Dan's patch,
here: https://sourceware.org/ml/gdb-patches/2008-03/msg00034.html)

I'm not seeing why we'd still need that bit, but then again,
I'm not seeing why it was needed in the first place.
The signal handler could run concurrently with gdb at any other
point in the gdb code, not just here, so at any point we
call into readline, we can be running readline code in parallel
with a signal handler touching readline's state.  It sounds like
that should be a readline problem to worry about.

That could be related to the fact that readline's signal handler
overrides gdb's, does its thing, and then calls gdb's signal
handler manually?  If the WaitForSingleObject call had already
woken up, then gdb's signal handler has already run and SetEvent
on sigint_event.  Then the code would go and run the deferred
signal handler.  In the remote case, that handler would
issue prompt "Give up (and stop debugging it)? (y or n)" prompt,
and if that is running in parallel with readline's signal
handler still calling rl_prep_terminal, bad things would happen.

But again, why isn't that a readline problem, instead of
a gdb problem?

In current GDB, we no longer issue that query from a signal
handler, we run it from a quit handler, always in mainline
code.  In essence, you could see it as if Dan's old Windows
signal-handler-deferring code was generalized.
But that would mean that we still have the same issue.
We end up in a quick handler issuing that same prompt shortly
after the SIGINT interrupting the event loop, via a serial_event,
which for Windows is a CreateEvent event, just like sigint_event
was on Dan's patch.

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?

I think there must be something else to this.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]