From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 103894 invoked by alias); 21 Mar 2019 17:31:22 -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 103760 invoked by uid 89); 21 Mar 2019 17:31:13 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=0.8 required=5.0 tests=BAYES_50,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=HX-Gm-Message-State:APjAAAW, situations, H*f:sk:310315f, H*f:sk:83ftrit 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; Thu, 21 Mar 2019 17:31:11 +0000 Received: by mail-wm1-f65.google.com with SMTP id z11so3693851wmi.0 for ; Thu, 21 Mar 2019 10:31:11 -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 w16sm4165675wrt.84.2019.03.21.10.31.08 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 Mar 2019 10:31:08 -0700 (PDT) Subject: Re: [PATCH] Readline: Cleanup some warnings To: 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> <711b6636-b02c-edb2-308d-5fddbf4c33a9@redhat.com> <83ftritydv.fsf@gnu.org> <3fc20d2b-5a49-928a-b474-f812f43a2c12@redhat.com> Cc: tom@tromey.com, gdb-patches@sourceware.org From: Pedro Alves Message-ID: <3e691193-19c4-056f-eeb5-7837a7e956b3@redhat.com> Date: Thu, 21 Mar 2019 17:31: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: <3fc20d2b-5a49-928a-b474-f812f43a2c12@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-03/txt/msg00465.txt.bz2 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