From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23641 invoked by alias); 18 Nov 2013 15:15:08 -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 23617 invoked by uid 89); 18 Nov 2013 15:15:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.0 required=5.0 tests=AWL,BAYES_50,RDNS_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 18 Nov 2013 15:13:25 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rAIFDH9n009059 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 18 Nov 2013 10:13:17 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id rAIFDFbq010568; Mon, 18 Nov 2013 10:13:16 -0500 Message-ID: <528A2E8B.9050300@redhat.com> Date: Mon, 18 Nov 2013 15:42:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Tom Tromey CC: gdb-patches@sourceware.org Subject: Re: [PATCH v4 9/9] enable target-async References: <1382464769-2465-1-git-send-email-tromey@redhat.com> <1382464769-2465-10-git-send-email-tromey@redhat.com> <52828856.9070904@redhat.com> <87li0qve9y.fsf@fleche.redhat.com> In-Reply-To: <87li0qve9y.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-11/txt/msg00467.txt.bz2 On 11/14/2013 09:56 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves writes: > >>> static int >>> mi_interpreter_prompt_p (void *data) >>> { > > Pedro> Looks quite odd for a predicate function to actually have > Pedro> side effects. I guess this is the hack you mentioned? > Pedro> I think this is missing a comment explaining what is > Pedro> going on. It's not obvious at all to me. > > Yeah, this is the biggest hack. > I will try to comment it some more. > > The fundamental issue here I ran into is that MI is very odd about when > it prints a prompt. > So, the hacks are needed to keep the behavior > consistent -- even though, IMNSHO, the behavior doesn't actually make > any sense. Sorry, I wasn't clear. I understand what as a whole the patch is trying to do; what I don't understand is why a hack was necessary, or its implementation. E.g., what exactly fails if the hack is not in place?; Why this spot for the hack?; What's the predicate used in the hack actually checking? Now that I looked again a little closer, I recalled that GDB in sync mode always outputs a silly extra prompt right after ^running (in response to execution commands), before the target stops, even though GDB is not ready for input then. Guess this hack is related? >>> cleanup = make_cleanup_ui_out_list_begin_end (uiout, "features"); >>> - if (target_can_async_p ()) >>> + if (mi_target_can_async_p ()) >>> ui_out_field_string (uiout, NULL, "async"); >>> if (target_can_execute_reverse) >>> ui_out_field_string (uiout, NULL, "reverse"); > > Pedro> Hmm, not sure this is right. This supposedly returns the set of > Pedro> supported features. But mi_target_can_async_p returns false > Pedro> until the frontend enables target-async. So this change creates > Pedro> a sort of chicken and egg situation. > > That is what I thought, too, but IIRC if one changes this, then a test > will fail. > > Also it is consistent with what gdb does today: > > (gdb) > -list-target-features > ^done,features=[] > (gdb) > set target-async on > &"set target-async on\n" > ^done > (gdb) > -list-target-features > ^done,features=["async"] > (gdb) > I guess we could see it either way. -list-target-features lists target features. So with GDB today, until "set target-async on" is issued, the target doesn't support async. After the series, the target does support async even if MI itself isn't async. E.g., I'd've expected 'interpreter-exec mi "-list-target-features"' issued from the cli to list "async". Given the chicken and egg thing already exists today, this makes me think no frontend is actually looking for this feature... (?) Anyway, fine with me to leave this as you have it for now, and maybe reconsider it after the series is in. > Strange but true. Actually I think this is symptomatic of the general > issue where MI paid attention to "set target-async", whereas I think in > a clean design it would not. Yeah, well. TBC, MI async was not really designed around "set target-async" specifically. Async support goes all the way back to 1999. At the time only the remote target supported async, but it wasn't actually the remote target. You'd do "target async" instead of "target remote" (that's still visible in gdb.base/async.exp). So by then, the frontend always knew what to expect, as it was the one who chose whether to async or not. When we started fixing async bitrot (and fixing lots of things) almost 10 years later, "target async" and "target remote" were merged, and a knob added to enable async-ness (and then GNU/Linux learnt async too). Fundamentally, in a perfect world, MI should have always just printed the prompt whenever GDB is ready for further frontend input. Then, a frontend that is aware of that, and that understands async (that is, is aware that gdb may be ready for further input before the target stops) would be able to cope with sync output too. But, unfortunately, in sync mode, GDB prints the prompt too right after sync execution commands, before the target stops. :-/ So one could say that even without async in the picture, the prompt in sync mode has always been wrong... In async mode, I don't think MI could ever emulate that, as the extra prompt would confuse the frontend. Well, maybe it could, if it had been defined that after ^running there's always an extra prompt that should be ignored. Oh well... > >>> -# so the stop reason is printed into MI uiout an. >>> -if {$async} { >>> - set reason "end-stepping-range" >>> -} else { >>> - set reason "" >>> -} >>> +set reason "end-stepping-range" > > Pedro> I'm a little confused by this one. Isn't it still necessary > Pedro> for targets that don't do async? > > Not sure if you remember the story. > > When I started this project I was working under the belief that "set > target-async" was a "please enable a feature" sort of option -- that is, > it ought to have no user visible effect other than making the "&" > feature available; and as such I could simply enable it always, fix the > test suite failures, and deprecate the option. > > However, it turns out that this model did not fit the reality. MI used > the target-async setting not just to put the target into async mode and > to enable the "&" feature, but also to change its output style in > various spots. > > There's a thread you can dig up where Marc Khouzam says they changed > Eclipse to disable target-async explicitly, just to work around the > oddities that ensued. Yeah, I recall that. https://sourceware.org/ml/gdb-patches/2011-12/msg00810.html The main issue was that ^C doesn't work for interrupting the target in async mode (you have to use -exec-interrupt). But I can imagine that the frontends would get confused by not seeing the extra prompt gdb puts out for (no good reason) after resumption commands in sync mode. > For this test case the check may in fact be irrelevant, since we aren't > enabling target-async. However if that is so, we might as well drop it > anyway on account of clarity. But you're dropping the sync path. How come the test doesn't fail on all other targets but GNU/Linux|Remote after this? > Or maybe this is intended to support running the test suite with some > pre-canned MI sequence to enable target async. I would guess nobody > ever does this, since I think when I tried something like this (naively > setting target_async_permitted = 1), stuff broke all over. Which is > apparently intentional. Now I'm _really_ confused. I (and Vladimir back when he was hacking on async) have done something like that many times in the past. You use a dejagnu board that does something like: set GDBFLAGS "${GDBFLAGS} -ex \"set target-async on\"" That's how async mode has been tested so far. That's why we call mi_detect_async right after starting gdb. Before this series, stuff isn't supposed to be breaking all over with that. Last I tried you'd only get a few regressions (like the bugs this series fixes), and I was assuming that you had done that too, and that at least before patch 9, testing gdb like that would be regression free compared to a default sync run. -- Pedro Alves