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

[Simon Marchi <simark@simark.ca>]

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