From patchwork Thu Oct 22 15:54:41 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 9326 Received: (qmail 65477 invoked by alias); 22 Oct 2015 15:54:46 -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 65461 invoked by uid 89); 22 Oct 2015 15:54:46 -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, RP_MATCHES_RCVD, SPF_HELO_PASS 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; Thu, 22 Oct 2015 15:54:44 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id A3FDD8F26E; Thu, 22 Oct 2015 15:54:43 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9MFsfip003681; Thu, 22 Oct 2015 11:54:42 -0400 Message-ID: <562906C1.8040306@redhat.com> Date: Thu, 22 Oct 2015 16:54:41 +0100 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> <5620D9D1.8080205@redhat.com> <20151016162250.GH3341@adacore.com> <562127AB.4050303@redhat.com> <20151016170515.GI3341@adacore.com> In-Reply-To: <20151016170515.GI3341@adacore.com> On 10/16/2015 06:05 PM, Joel Brobecker wrote: > 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. Alright, now pushed, as below. From c72f45d16c16954478dcd87531df146f68acd87c Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 22 Oct 2015 16:40:45 +0100 Subject: [PATCH] gdb/Windows: use windows_wait/windows_resume directly in initial startup Explanation below based on what Joel wrote at: https://sourceware.org/ml/gdb-patches/2015-10/msg00274.html The merge async/sync code paths patch broke attaching on Windows. 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] Despite appearances, GDB is not reading from the prompt. It is blocked waiting for an event from the inferior. And since our inferior is stopped, there aren't going to be any events 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 the initial 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 blocking call to target_wait, thus hanging forever. The fix is to use windows_wait and windows_resume directly, similarly to gdbserver. This will also allow getting rid of 'stop_after_trap'. gdb/ChangeLog: 2015-10-22 Pedro Alves * windows-nat.c (do_initial_windows_stuff): Rewrite loop using windows_wait and windows_resume directly instead of wait_for_inferior and resume. --- gdb/ChangeLog | 6 ++++++ gdb/windows-nat.c | 21 +++++++++++---------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 0df8246..65d056b 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,9 @@ +2015-10-22 Pedro Alves + + * windows-nat.c (do_initial_windows_stuff): Rewrite loop using + windows_wait and windows_resume directly instead of + wait_for_inferior and resume. + 2015-10-22 Simon Marchi * xtensa-tdep.h (XTREG): Add casts. diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index 4ab74b4..e6c396b 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -1642,7 +1642,6 @@ windows_add_all_dlls (void) static void do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching) { - extern int stop_after_trap; int i; struct inferior *inf; struct thread_info *tp; @@ -1681,16 +1680,20 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching) target_terminal_inferior (); windows_initialization_done = 0; - inf->control.stop_soon = STOP_QUIETLY; + 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 + struct target_waitstatus status; + + windows_wait (ops, minus_one_ptid, &status, 0); + + /* Note windows_wait returns TARGET_WAITKIND_SPURIOUS for thread + events. */ + if (status.kind != TARGET_WAITKIND_LOADED + && status.kind != TARGET_WAITKIND_SPURIOUS) break; + + windows_resume (ops, minus_one_ptid, 0, GDB_SIGNAL_0); } /* Now that the inferior has been started and all DLLs have been mapped, @@ -1711,8 +1714,6 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching) windows_add_all_dlls (); windows_initialization_done = 1; - inf->control.stop_soon = NO_STOP_QUIETLY; - stop_after_trap = 0; return; }