From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12981 invoked by alias); 28 Aug 2014 16:22:26 -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 12914 invoked by uid 89); 28 Aug 2014 16:22:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-pa0-f42.google.com Received: from mail-pa0-f42.google.com (HELO mail-pa0-f42.google.com) (209.85.220.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 28 Aug 2014 16:22:23 +0000 Received: by mail-pa0-f42.google.com with SMTP id lf10so3192463pab.29 for ; Thu, 28 Aug 2014 09:22:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=dxJyJFFElB08Ge5WIrgb47tQjbOOnJeVMT1hIzJoX7E=; b=lUJMI2jEqS1jmhl2LmDqgbm3fYB48yYYlfJfZqeD6rum7ZHsL6VP96KTDBoA0ceWPw BT/TSe6JD6eqy//mJE0VQ9tedt9fTun9q9UgWe2FA1pthXpxn7NGzX7nCUI1MZOleRVs vkVMp8FeNprcIhDESihjoH/58twSze/YoEmBsqB9xQAzBvvx336V7ql+by0HpMgANXpp tM70FCPLNVTOU+M8ALA33W/qT49vtS3rrBEpuQlmEEOGHGeqV4Ye7IBBzh4F6dy5wxpi xem5vslC8Drt7CPtdzFXz/s87/yNKemJPsZePeoeUTTpHh0BS2PXFxqMzoPYV7xGsjNA s/mA== X-Gm-Message-State: ALoCoQm76YUUKYnfIJ+uMc8I0RErt8T8VQcOfC5PBhMDLSvoAWXbAkkCszTHDYh1r+eTMoZrfLWL MIME-Version: 1.0 X-Received: by 10.68.233.68 with SMTP id tu4mr7461145pbc.65.1409242940921; Thu, 28 Aug 2014 09:22:20 -0700 (PDT) Received: by 10.70.5.229 with HTTP; Thu, 28 Aug 2014 09:22:20 -0700 (PDT) In-Reply-To: <53FF51F5.80506@redhat.com> References: <1408740286-29326-1-git-send-email-patrick@parcs.ath.cx> <53FE0FF9.9010008@redhat.com> <53FF1102.8050709@redhat.com> <53FF51F5.80506@redhat.com> Date: Thu, 28 Aug 2014 16:22:00 -0000 Message-ID: Subject: Re: [PATCH] Fix the processing of Meta-key commands in TUI From: Patrick Palka To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg00623.txt.bz2 On Thu, Aug 28, 2014 at 11:59 AM, Pedro Alves wrote: > On 08/28/2014 03:13 PM, Patrick Palka wrote: >> On Thu, Aug 28, 2014 at 7:22 AM, Pedro Alves wrote: >>> On 08/27/2014 07:25 PM, Patrick Palka wrote: >>>> On Wed, 27 Aug 2014, Pedro Alves wrote: >>> >>>>> The main reason I think there's a larger problem here, is that >>>>> if curses is reading more than one char from stdin, then that means >>>>> that is must be blocking for a bit waiting for the next character, >>>>> which is a no-no in an async world, where we want to be processing >>>>> target events at the same time. The man page says: >>>>> >>>>> While interpreting an input escape sequence, wgetch sets a timer while waiting for the next character. If notimeout(win, TRUE) is called, then wgetch does not >>>>> set a timer. The purpose of the timeout is to differentiate between sequences received from a function key and those typed by a user. >>>>> >>>>> Looks like there's a default timeout of 1 second. Indeed if I set a >>>>> breakpoint in wgetch and another right after wgetch is called, and >>>>> then I press escape, I see that gdb is stuck inside wgetch for around >>>>> one second. During that time, gdb's own event loop isn't being processed. >>>>> >>>>> Not sure exactly how this is usually handled. Seems like there >>>>> are several knobs that might be able to turn this delay off. >>>>> Sounds like we should enable that (whatever the option is), >>>>> and handle the timeout ourselves? >>>> >>>> I don't think the timeout is the issue here. Even if the timeout is >>>> disabled via notimeout(), wgetch() will still attempt to interpret keypad >>>> sequences by reading multiple characters from stdin -- except that the >>>> read will now be a non-blocking one instead of a blocking one. >>>> >>>> One way or another, someone must read multiple keys from stdin in order >>>> to semantically distinguish between keypad keys and regular key >>>> sequences. And when it turns out that the input is not or cannot be a >>>> keypad key then that someone must place the extraneous keys into a >>>> buffer and notify GDB's event handler that we missed their stdin events. >>> >>> Right, that's a given. What I was talking about is fixing the >>> 1 second block in case the input stops before a sequence is complete. >>> >>>> If we handle the timeout ourselves (for instance by disabling keypad() >>>> and enabling notimeout()) then we'll be responsible for doing the >>>> lookahead, interpreting the sequences and buffering the keypresses. I >>>> say let ncurses continue to handle the timeout so that we'll only be >>>> responsible for notifying the event handler. >>>> >>>> Though I may just be misunderstanding your proposal. >>> >>> The main idea was to not let ncurses ever block, as that prevents >>> gdb's event loop from handling target events. If ncurses internally >>> already handled the timeout by comparing the time between >>> wgetch calls instead of doing a blocking select/poll internally, then >>> it'd be a bit easier, but it looks like it doesn't (GetEscdelay always >>> starts with the window's configured delay, on each wgetch call?), so we'd >>> need to track the timeout ourselves. Even if it did, it wouldn't be that >>> different though. >> >> Is the internal timeout a big deal though? The handling of the target >> event just gets temporarily delayed, not missed entirely, right? And >> isn't this timeout only experienced when one presses ESC (which has no >> use in TUI) and/or attempts to manually type a function key sequence? >> I'm not sure why anyone would do that. > > Right. TBC, I noticed/found this while ramping up for reviewing this patch, > reading the ncurses code and trying to make sense of the whole ncurses/tui > integration and your patch. I'm not _that_ familiar with this > code, although I'm probably one of the most familiar nowadays... > > My main motivation for pointing this out was to brainstorm. I had > no idea if there was a canned solution for this, which your or someone > might know about. :-) I wasn't really sure of the complexity of the > solution until I spent a few more hours this morning working through > what it would take and write out those steps list... Indeed.. it certainly doesn't help that the ncurses source code is pretty difficult reading. > >> Yeah same here. I can't seem to find the magical invocation that >> actually disables this timeout. > > Thanks for poking. > >>> If such delays/blocks can't be eliminated due to buggy ncurses, or >>> something missing in the APIs, then it looks like the only way >>> to fix this would be to move the wgetch call to a separate thread, >>> like, we'd create a pipe, and put one end in the event loop as >>> stdin source instead of the real stdin, and then the separate thread >>> would push the results of wgetch into this pipe... >> >> I am not sure that it's possible to eliminate the internal timeout >> completely so it seems to me that your thread-based solution may be >> necessary to fix this. But is it worth the complexity to fix this >> seemingly obscure issue? > > Yeah, we certainly shouldn't let the perfect be the enemy of the > good. > > I'd think using vi-mode keybindings would need it, but according > to PR11383 those don't work anyway. Interesting, I'll take a look at this aspect. > > The thread-based solution would make your patch unnecessary, > but then again, that's easy to revert if a thread-based solution > ends up written. > > I think we understand the problems and the effort required > for the potential solutions enough to be able to firmly > say: yes, let's proceed with your solution. Thanks Pedro, for your insightful and detailed review. > > Thanks, > Pedro Alves >