From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 114770 invoked by alias); 14 Apr 2015 13:16:17 -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 114751 invoked by uid 89); 14 Apr 2015 13:16:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY 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; Tue, 14 Apr 2015 13:16:10 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id C820BD3717; Tue, 14 Apr 2015 09:16:08 -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 W1BAziT74SjG; Tue, 14 Apr 2015 09:16:08 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 977F811651B; Tue, 14 Apr 2015 09:16:08 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 846DA40EAD; Tue, 14 Apr 2015 06:16:15 -0700 (PDT) Date: Tue, 14 Apr 2015 13:16:00 -0000 From: Joel Brobecker To: Jon Turney Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Fixes to Cygwin-specific signal handling Message-ID: <20150414131615.GI4704@adacore.com> References: <1429009382-21040-1-git-send-email-jon.turney@dronecode.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1429009382-21040-1-git-send-email-jon.turney@dronecode.org.uk> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2015-04/txt/msg00524.txt.bz2 Jon, On Tue, Apr 14, 2015 at 12:03:02PM +0100, Jon Turney wrote: > Originally by cgf, this patch has been carried in Cygwin's gdb package > for a few years. I've cleaned it up a bit and revised it for master. > > Without this patch, it's impossible to usefully run the testsuite on > Cygwin. > > gdb/ChangeLog: > > 2015-04-11 Jon Turney > > * windows-nat.c: Replace have_saved_context with signal_thread_id > throughout. > (thread_rec): Don't retrieve context if we have a saved one. > Ignore 'Invalid Handle' errors. > (handle_output_debug_string): Mark signal context as not to be > written to inferior by windows_continue() or windows_resume(). > (get_windows_debug_event): Replace retval with thread_id > throughout. Don't clear any saved context. Overall, the patch looks reasonable to me. But I think there are at least 3 independent changes, and it would be nice to split those two out, for a couple of reasons: 1. It allows you to explain the nature of the problem, from the user's standpoint, that the patch is fixing (ie, what user-visible symptoms it fixes); 2. it allows us to see how each problem is fixed, and to deal with each issue separately. The three issues I view as independent: a. ignoring "invalid handle" errors; b. unsetting saved_context.ContextFlags c. the renaming of have_saved_context into signal_thread_id so you can compare the current thread id with the saved signal_thread_id. A few minor comments below. > @@ -849,8 +853,12 @@ handle_output_debug_string (struct target_waitstatus *ourstatus) > &saved_context, > __COPY_CONTEXT_SIZE, &n) > && n == __COPY_CONTEXT_SIZE) > - have_saved_context = 1; > - current_event.dwThreadId = retval; > + { > + signal_thread_id = retval; > + saved_context.ContextFlags = 0; /* Don't attempt to call SetThreadContext */ Can you move the comment just above the statement so as to avoid exceeding the maximum line length, and also explain why we should not call SetThreadContext? > @@ -1330,7 +1338,6 @@ get_windows_debug_event (struct target_ops *ops, > event_code = current_event.dwDebugEventCode; > ourstatus->kind = TARGET_WAITKIND_SPURIOUS; > th = NULL; > - have_saved_context = 0; Normally, there is a corresponding addition that sets signal_thread_id to zero. This leads me to be believe that there might be an additional user-visible symptom that this patch fixes? > @@ -1501,12 +1508,12 @@ get_windows_debug_event (struct target_ops *ops, > else > { > inferior_ptid = ptid_build (current_event.dwProcessId, 0, > - retval); > - current_thread = th ?: thread_rec (current_event.dwThreadId, TRUE); > + thread_id); > + current_thread = th ?: thread_rec (thread_id, TRUE); I know you are just repeating what was there before, but I don't think this is the clearest way to do things. GDB Coding Standards also do not allow assignments within conditions. I suggest instead: current_thread = th; if (!current_thread) thread_rec (thread_id, TRUE); > out: > - return retval; > + return (int) thread_id; I'm wondering whether the cast is actually necessary? Also, it looks like the function's comment might be stale: /* Get the next event from the child. Return 1 if the event requires handling by WFI (or whatever). */ static int get_windows_debug_event (struct target_ops *ops, int pid, struct target_waitstatus *ourstatus) Would you mind fixing that? Thanks, -- Joel