From patchwork Fri Oct 16 00:35:25 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 9178 Received: (qmail 113074 invoked by alias); 16 Oct 2015 00:35:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 113057 invoked by uid 89); 16 Oct 2015 00:35:30 -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 00:35:28 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id C0A64297FF; Thu, 15 Oct 2015 20:35:26 -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 d2XiHUgTyRlD; Thu, 15 Oct 2015 20:35:26 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 945E1296EE; Thu, 15 Oct 2015 20:35:26 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 0DBC442BB8; Thu, 15 Oct 2015 17:35:25 -0700 (PDT) Date: Thu, 15 Oct 2015 17:35:25 -0700 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: <20151016003525.GB1806@adacore.com> References: <1439398917-22761-1-git-send-email-palves@redhat.com> <1439398917-22761-2-git-send-email-palves@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1439398917-22761-2-git-send-email-palves@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) 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! diff --git a/gdb/infcmd.c b/gdb/infcmd.c index fc27904..8fb133c 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -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); + } /* Done with ARGS. */ do_cleanups (args_chain); diff --git a/gdb/infrun.c b/gdb/infrun.c index cf91370..2c189f9 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -107,29 +107,20 @@ static int maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc); when we have pending events ready to be passed to the core. */ static struct async_event_handler *infrun_async_inferior_event_token; -/* Stores whether infrun_async was previously enabled or disabled. - Starts off as -1, indicating "never enabled/disabled". */ -static int infrun_is_async = -1; - /* See infrun.h. */ void infrun_async (int enable) { - if (infrun_is_async != enable) - { - infrun_is_async = enable; - - if (debug_infrun) - fprintf_unfiltered (gdb_stdlog, - "infrun: infrun_async(%d)\n", - enable); + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: infrun_async(%d)\n", + enable); - if (enable) - mark_async_event_handler (infrun_async_inferior_event_token); - else - clear_async_event_handler (infrun_async_inferior_event_token); - } + if (enable) + mark_async_event_handler (infrun_async_inferior_event_token); + else + clear_async_event_handler (infrun_async_inferior_event_token); } /* See infrun.h. */