From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 107003 invoked by alias); 19 Mar 2019 19:02:49 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 106992 invoked by uid 89); 19 Mar 2019 19:02:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=essence, HX-Received:c115, hide X-HELO: mail-wm1-f65.google.com Received: from mail-wm1-f65.google.com (HELO mail-wm1-f65.google.com) (209.85.128.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 19 Mar 2019 19:02:47 +0000 Received: by mail-wm1-f65.google.com with SMTP id f3so18205095wmj.4 for ; Tue, 19 Mar 2019 12:02:47 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id n11sm3968462wrt.63.2019.03.19.12.02.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 Mar 2019 12:02:44 -0700 (PDT) Subject: Re: [PATCH] Readline: Cleanup some warnings To: Tom Tromey , Eli Zaretskii References: <20190130085716.75179-1-alan.hayward@arm.com> <20190131075907.GA313@adacore.com> <3463805B-A8BF-4C20-ACE3-C21AE3F7DB62@arm.com> <20190201080533.GA31043@adacore.com> <877eejvfoq.fsf@tromey.com> <1549047248.2630.7.camel@skynet.be> <310315f8-62ab-2eff-042f-9f2ae9de07da@redhat.com> <87wokxtnlt.fsf@tromey.com> <83h8c1wdr5.fsf@gnu.org> <87imwex333.fsf@tromey.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <711b6636-b02c-edb2-308d-5fddbf4c33a9@redhat.com> Date: Tue, 19 Mar 2019 19:02:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <87imwex333.fsf@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-03/txt/msg00414.txt.bz2 On 03/19/2019 04:04 PM, Tom Tromey wrote: >>>>>> "Eli" == Eli Zaretskii writes: > >>> From: Tom Tromey >>> Cc: "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