Re: [PATCH 7/7] gdbserver: remove support for ARM/WinCE

Kevin Buettner
2020-05-14T20:30:56Z

Hi Simon,

Just one nit, maybe...

On Thu, 14 May 2020 15:05:37 -0400
Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> 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 (&current_event, 0))
> - {
> - OUTMSG2(("no attach events left\n"));
> - fake_breakpoint_event ();
> - attaching = 0;
> - }
> - else
> - OUTMSG2(("got attach event\n"));
> - }
> - else
> -#endif
> - {
> - gdb::optional<pending_stop> stop = fetch_pending_stop (debug_threads);
> - if (stop.has_value ())
> - {
> - *ourstatus = stop->status;
> - current_event = stop->event;
> - ptid = debug_event_ptid (&current_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<pending_stop> stop = fetch_pending_stop (debug_threads);
> + if (stop.has_value ())
> + {
> + *ourstatus = stop->status;
> + current_event = stop->event;
> + ptid = debug_event_ptid (&current_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 (&current_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 (&current_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