From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 39952 invoked by alias); 16 Oct 2015 17:05:25 -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 39940 invoked by uid 89); 16 Oct 2015 17:05:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 16 Oct 2015 17:05:19 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 71ECF11824A; Fri, 16 Oct 2015 13:05:17 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id nWXutlI8vgj6; Fri, 16 Oct 2015 13:05:17 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 4953D118247; Fri, 16 Oct 2015 13:05:17 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id C162040587; Fri, 16 Oct 2015 10:05:15 -0700 (PDT) Date: Fri, 16 Oct 2015 17:05:00 -0000 From: Joel Brobecker To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/7] Merge async and sync code paths some more Message-ID: <20151016170515.GI3341@adacore.com> References: <1439398917-22761-1-git-send-email-palves@redhat.com> <1439398917-22761-2-git-send-email-palves@redhat.com> <20151016003525.GB1806@adacore.com> <5620D9D1.8080205@redhat.com> <20151016162250.GH3341@adacore.com> <562127AB.4050303@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <562127AB.4050303@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2015-10/txt/msg00294.txt.bz2 > I had forgotten how building on Cygwin takes ages! :-P :-) > So my main gripe with the patch is this: > > @@ -2821,6 +2821,15 @@ attach_command (char *args, int from_tty) > mark_infrun_async_event_handler (); > return; > } > + else > + { > + /* We don't expect any additional attach event from the target. > + So make sure that the infrun_async_event_handler is disabled. > + Otherwise, the main event loop might believe that we have > + inferior events ready, causing us to wait for those event > + that will never come, since our inferior is now stopped. */ > + infrun_async (0); > + } > > If debugging more than one inferior (granted, Windows doesn't > support this, but, alas), this prevents gdb from reacting to pending events > for threads of the other inferior. Ah, yes indeed. And besides, the patch is not Windows-specific either, so presumably could negatively affect another target that does support multi-inferior debugging. > How about we make do_initial_windows_stuff call windows_wait/windows_resume > directly, like we did for gdbserver here: > > https://sourceware.org/ml/gdb-patches/2013-12/msg00371.html > > As mentioned in that thread, I've wanted to do this before > in order to get rid of stop_after_trap. > > The patch below seems to work for me. Of course! Why take the roundabout way when you can take the direct route? I don't see a reason in this case, and it completely avoids the issue of the async even handler. And it looks like a nice simplification to boot, at least to me. Patch looks good, and I tested it against AdaCore's testsuite, showing that it fixes the "attach" regressions without introducing any new failure. Slightly unrelated to your patch, now, but would you mind sharing your thoughts on: > I couldn't figure out why we needed both infrun_async and > mark_infrun_async_event_handler, and in particular, I didn't > see the need for the infrun_is_async != enable guard. > So, this patch just makes infrun_async always call the > relevant {mark,clear}_infrun_async_event_handler routine. > > If you agree that's the right thing to do, I propose we delete > {mark,clear}_infrun_async_event_handler, or replace them by > the associated infrun_async routine. Although, from someone > not very familiar with all this async stuff, the function > names {mark,clear}_infrun_async_event_handler speak a little > more to me. On the other hand, I like infrun_async because of > the debug trace. So we could also eliminate infrun_async and > move the debug trace into {mark,clear}_infrun_async_event_handler. > We'd have to make clear_infrun_async_event_handler non-static. If there is an improvement to be had, be it just adding more comments, I can take care of that. Thanks a lot! -- Joel