From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id A5D263858D35 for ; Thu, 14 May 2020 23:14:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A5D263858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.193] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B01021E5F9; Thu, 14 May 2020 19:14:24 -0400 (EDT) Subject: Re: [PATCH 7/7] gdbserver: remove support for ARM/WinCE To: Kevin Buettner , gdb-patches@sourceware.org Cc: Simon Marchi References: <20200514174359.2272960-1-simon.marchi@efficios.com> <20200514190537.2321826-8-simon.marchi@efficios.com> <20200514133049.2297a58a@f31-4.lan> From: Simon Marchi Message-ID: <9d88e2ba-3edd-7b7f-f88a-618f400b5a39@simark.ca> Date: Thu, 14 May 2020 19:14:23 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200514133049.2297a58a@f31-4.lan> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 May 2020 23:14:36 -0000 On 2020-05-14 4:30 p.m., Kevin Buettner via Gdb-patches wrote: > Hi Simon, > > Just one nit, maybe... > > On Thu, 14 May 2020 15:05:37 -0400 > Simon Marchi via Gdb-patches wrote: > >> diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc >> index 4eb63b7ca25a..d671691a575d 100644 >> --- a/gdbserver/win32-low.cc >> +++ b/gdbserver/win32-low.cc >> @@ -1414,69 +1414,39 @@ get_child_debug_event (DWORD *continue_status, >> goto gotevent; >> } >> >> -#ifndef _WIN32_WCE >> attaching = 0; >> -#else >> - if (attaching) >> - { >> - /* WinCE doesn't set an initial breakpoint automatically. To >> - stop the inferior, we flush all currently pending debug >> - events -- the thread list and the dll list are always >> - reported immediatelly without delay, then, we suspend all >> - threads and pretend we saw a trap at the current PC of the >> - main thread. >> - >> - Contrary to desktop Windows, Windows CE *does* report the dll >> - names on LOAD_DLL_DEBUG_EVENTs resulting from a >> - DebugActiveProcess call. This limits the way we can detect >> - if all the dlls have already been reported. If we get a real >> - debug event before leaving attaching, the worst that will >> - happen is the user will see a spurious breakpoint. */ >> - >> - current_event.dwDebugEventCode = 0; >> - if (!wait_for_debug_event (¤t_event, 0)) >> - { >> - OUTMSG2(("no attach events left\n")); >> - fake_breakpoint_event (); >> - attaching = 0; >> - } >> - else >> - OUTMSG2(("got attach event\n")); >> - } >> - else >> -#endif >> - { >> - gdb::optional stop = fetch_pending_stop (debug_threads); >> - if (stop.has_value ()) >> - { >> - *ourstatus = stop->status; >> - current_event = stop->event; >> - ptid = debug_event_ptid (¤t_event); >> - current_thread = find_thread_ptid (ptid); >> - return 1; >> - } >> + { > > I think this brace and the matching one later on can be removed, with > a corresponding reduction in indent level for the enclosed block. > >> + gdb::optional stop = fetch_pending_stop (debug_threads); >> + if (stop.has_value ()) >> + { >> + *ourstatus = stop->status; >> + current_event = stop->event; >> + ptid = debug_event_ptid (¤t_event); >> + current_thread = find_thread_ptid (ptid); >> + return 1; >> + } >> >> - /* Keep the wait time low enough for comfortable remote >> - interruption, but high enough so gdbserver doesn't become a >> - bottleneck. */ >> - if (!wait_for_debug_event (¤t_event, 250)) >> - { >> - DWORD e = GetLastError(); >> + /* Keep the wait time low enough for comfortable remote >> + interruption, but high enough so gdbserver doesn't become a >> + bottleneck. */ >> + if (!wait_for_debug_event (¤t_event, 250)) >> + { >> + DWORD e = GetLastError(); >> >> - if (e == ERROR_PIPE_NOT_CONNECTED) >> - { >> - /* This will happen if the loader fails to succesfully >> - load the application, e.g., if the main executable >> - tries to pull in a non-existing export from a >> - DLL. */ >> - ourstatus->kind = TARGET_WAITKIND_EXITED; >> - ourstatus->value.integer = 1; >> - return 1; >> - } >> + if (e == ERROR_PIPE_NOT_CONNECTED) >> + { >> + /* This will happen if the loader fails to succesfully >> + load the application, e.g., if the main executable >> + tries to pull in a non-existing export from a >> + DLL. */ >> + ourstatus->kind = TARGET_WAITKIND_EXITED; >> + ourstatus->value.integer = 1; >> + return 1; >> + } >> >> - return 0; >> - } >> - } >> + return 0; >> + } >> + } > > I think that's the matching brace, above. > > Kevin > Thanks for the suggestion. I tried this, but just removing the braces leads to this error: CXX win32-low.o cc1plus: warning: command line option ‘-Wmissing-prototypes’ is valid for C/ObjC but not for C++ /home/smarchi/src/binutils-gdb/gdbserver/win32-low.cc: In function ‘int get_child_debug_event(DWORD*, target_waitstatus*)’: /home/smarchi/src/binutils-gdb/gdbserver/win32-low.cc:1449:2: error: jump to label ‘gotevent’ 1449 | gotevent: | ^~~~~~~~ /home/smarchi/src/binutils-gdb/gdbserver/win32-low.cc:1414:12: note: from here 1414 | goto gotevent; | ^~~~~~~~ /home/smarchi/src/binutils-gdb/gdbserver/win32-low.cc:1418:33: note: crosses initialization of ‘gdb::optional stop’ 1418 | gdb::optional stop = fetch_pending_stop (debug_threads); | ^~~~ So it would require other changes. I'd rather keep this patch trivial, just removing the ifdefs, and keep this other change for another time. Simon