From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89367 invoked by alias); 16 Apr 2015 19:24:24 -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 88354 invoked by uid 89); 16 Apr 2015 19:24:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: out4-smtp.messagingengine.com Received: from out4-smtp.messagingengine.com (HELO out4-smtp.messagingengine.com) (66.111.4.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 16 Apr 2015 19:24:21 +0000 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id 92A132076B for ; Thu, 16 Apr 2015 15:24:19 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute5.internal (MEProxy); Thu, 16 Apr 2015 15:24:19 -0400 Received: from [192.168.1.102] (unknown [31.51.207.104]) by mail.messagingengine.com (Postfix) with ESMTPA id 1F61EC00020; Thu, 16 Apr 2015 15:24:19 -0400 (EDT) Message-ID: <55300C61.2080102@dronecode.org.uk> Date: Thu, 16 Apr 2015 19:24:00 -0000 From: Jon TURNEY User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Joel Brobecker CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Fixes to Cygwin-specific signal handling References: <1429009382-21040-1-git-send-email-jon.turney@dronecode.org.uk> <20150414131615.GI4704@adacore.com> In-Reply-To: <20150414131615.GI4704@adacore.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg00634.txt.bz2 Thanks for taking the time to look at this. On 14/04/2015 14:16, Joel Brobecker wrote: > > 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. Ok. Unfortunately I don't have access to the original problems which inspired these patches, so I have reduced the scope to a problem I can easily demonstrate and split things up further. > > The three issues I view as independent: > a. ignoring "invalid handle" errors; Yes, this is an unrelated change I should have excluded. This was discussed somewhat in the thread starting at https://cygwin.com/ml/gdb-patches/2013-06/msg00237.html > 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? I've dropped this change for the moment. >> @@ -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? I'm sorry I don't follow you. have_saved_context is reset in do_windows_fetch_inferior_registers() when the context is actually retrieved. It seems odd to reset it in get_windows_debug_event() as it seems that any other Windows debug event between the Cygwin signal message and the do_windows_fetch_inferior_registers() will prevent the saved context being used, if it's possible for that to happen. >> @@ -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); I've done this (with the last line as an assignment), but you'll have to help me find where the Coding Standard says that. [1] mentions not using assignments inside if conditions, but I don't see any mention of ?: [1] http://www.gnu.org/prep/standards/standards.html#Syntactic-Conventions >> out: >> - return retval; >> + return (int) thread_id; > > I'm wondering whether the cast is actually necessary? On third thoughts, I think not. > 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? Done.