From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5423 invoked by alias); 16 Oct 2015 12:24:06 -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 5184 invoked by uid 89); 16 Oct 2015 12:24:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 16 Oct 2015 12:24:04 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id BFD8D8EA45; Fri, 16 Oct 2015 11:04:52 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9GB4ovn002117; Fri, 16 Oct 2015 07:04:51 -0400 Message-ID: <5620D9D1.8080205@redhat.com> Date: Fri, 16 Oct 2015 12:24:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Joel Brobecker CC: gdb-patches@sourceware.org Subject: Re: [PATCH 1/7] Merge async and sync code paths some more References: <1439398917-22761-1-git-send-email-palves@redhat.com> <1439398917-22761-2-git-send-email-palves@redhat.com> <20151016003525.GB1806@adacore.com> In-Reply-To: <20151016003525.GB1806@adacore.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-10/txt/msg00280.txt.bz2 Hi Joel, I'm trying to build a Windows gdb to see this in action. Many thanks for the detailed analysis! Thanks, Pedro Alves On 10/16/2015 01:35 AM, Joel Brobecker wrote: > Hi Pedro, > > On Wed, Aug 12, 2015 at 06:01:51PM +0100, Pedro Alves wrote: >> This patch makes the execution control code use largely the same >> mechanisms in both sync- and async-capable targets. This means using >> continuations and use the event loop to react to target events on sync >> targets as well. The trick is to immediately mark infrun's event loop >> source after resume instead of calling wait_for_inferior. Then >> fetch_inferior_event is adjusted to do a blocking wait on sync >> targets. >> >> gdb/ChangeLog: >> 2015-08-12 Pedro Alves >> >> * breakpoint.c (bpstat_do_actions_1, until_break_command): Don't >> check whether the target can async. >> * inf-loop.c (inferior_event_handler): Only call target_async if >> the target can async. > > This patch unfortunately breaks attaching on Windows. I suspect this > might be related to the fact that target_attach_no_wait is True on > this platform (meaning, once we have attached to the inferior, we do not > have to wait for an extra event, unlike on Linux). > > This is what we observe, after attaching to any process. At first, > it seems like everything worked fine, since the process stops, and > we get the prompt back: > > (gdb) att 3156 > Attaching to program `C:\[...]\foo.exe', process 3156 > [New Thread 3156.0xcd8] > [New Thread 3156.0xfe4] > 0x7770000d in ntdll!DbgBreakPoint () from C:\Windows\SysWOW64\ntdll.dll > (gdb) > > However, enter any commands at all, and GDB appears to be hanging. > For instance: > > (gdb) set lang ada > [nothing happens] > > What happens, in fact, is that despite appearances, GDB is not reading > from the prompt. It is waiting for an event from the inferior. And > since our inferior has stopped, there aren't going to be any event > to read. > > In chronological order, what happens is that windows_attach calls > do_initial_windows_stuff, which performs the inferior creation, > and repeatedly waits until we get the first SIGTRAP: > > while (1) > { > stop_after_trap = 1; > wait_for_inferior (); > tp = inferior_thread (); > if (tp->suspend.stop_signal != GDB_SIGNAL_TRAP) > resume (tp->suspend.stop_signal); > else > break; > } > > The call to wait_for_inferior triggers a call to do_target_wait > to get the event, followed by handle_inferior_event to process it. > However, because the first couple of events are "spurious" events, > GDB resumes the execution, and prepares the inferior to wait again: > > case TARGET_WAITKIND_SPURIOUS: > [...] > resume (GDB_SIGNAL_0); > prepare_to_wait (ecs); > > And prepare_to_wait just does... > > ecs->wait_some_more = 1; > if (!target_is_async_p ()) > mark_infrun_async_event_handler (); > > ... which as a result sets the infrun_async_event_handler "ready" > flag to 1. > > We get a couple of spurious events before we get our first SIGTRAP, > at which point we exit the "while (1)" loop above, after which > we reach the end of the attach_command, followed by the normal > end-of-command processing (normal_stop, bp handling, printing the > GDB prompt), back finally to the root of the event loop. > > Notice that, at this point, nothing has unset the "ready" flag > for the infrun_async_event_handler. So, when another cycle of > gdb_do_one_event from the event loop, we eventually call > check_async_event_handlers, which finds that the infrun async > event handler is "ready", and therefore calls it's associated > "proc" callback, which does... > > inferior_event_handler (INF_REG_EVENT, NULL); > > ... triggering a call to the target's to_wait method, thus hanging > forever. > > Comparing what happens on GNU/Linux, the difference is that we do > expect events from the inferior after attaching. And fetching > those events cause the infrun async event handler's "ready" flag > to be unset before we return to the mainloop. > > It seems to me that, for target such as Windows where there is > no need to wait for an event after the we've attached to the inferior, > the flag should be reset somewhere, and the the best place seemed > to be at the end of attach_command, when target_attach_no_wait is > true. That's the infrun_async (0) call. > > Although, now that I think of it, shouldn't it we do it like in > the mainloop where we clear the "ready" flag before fetching > the inferior event? > > Anyways, adding the "infrun_async (0)" did fix the problem, except > it only fixed it for the first attach. It turns out that the same > problem would resurface if you use the attach command a second time. > For instance: > > (gdb) attach 1234 > [all OK] > (gdb) detach > [so far, so good] > (gdb) attach 1234 > [seems OK, and we get the prompt back, but...] > (gdb) > > ... at this point we realize that GDB is no longer responsive. > > This is because the "ready" flag is set via > mark_infrun_async_event_handler, which has no guard, so always > changes the "ready" flag. However, because there was no public > API for unsetting the infrun async event handler other than > infrun_async, I used that. But I missed the fact that the behavior > of that function is guarded by a global: > > /* Stores whether infrun_async was previously enabled or disabled. > Starts off as -1, indicating "never enabled/disabled". */ > static int infrun_is_async = -1; > > void > infrun_async (int enable) > { > if (infrun_is_async != enable) > { > [...] > } > } > > That global got set to zero at the end of the first attach. > But no one else changed the value of that global since then. > So, on the second attach, the added "infrun_async (0)" has > no effect, thus leading us to the same perpetual wait for > inferior events. > > 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. > > The attached patch is a prototype tested on x86-windows. > If you agree, I'll make a proper patch submission, after > having tested it on GNU/Linux as well. > > WDYT? > > Thanks! >