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

Simon Marchi
2020-05-14T23:14:25Z

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 <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
>

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<windows_nat::pending_stop> stop’
1418 | gdb::optional<pending_stop> 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