[PATCH 0/2] Implement target async for Windows

[PATCH 0/2] Implement target async for Windows

[Tom Tromey]

I wanted target async to work on Windows for another project.  This
short series is the result.

This series is based on my earlier patch to make strwinerror available
to all the Windows native code.  That patch was useful while debugging
these.

The basic problem on Windows is that WaitForDebugEvent can't be mixed
with WaitForMulpleObjects -- there's no way to get some kind of "debug
handle" to wait on.  To handle this, WaitForDebugEvent is now called
in a worker thread.  However, to do that, certain other Windows calls
must be done in that same thread, as various debugging APIs can only
be used from the thread that started the inferior.  (Not all of these
are documented, either.)

I've tested this using the internal AdaCore test suite.

Tom

[PATCH 1/2] Move some Windows operations to worker thread

[Tom Tromey]

On Windows, certain debugging APIs can only be called from the thread
that started (or attached) to the inferior.  Also, there is no way on
Windows to wait for a debug event in addition to other events.
Therefore, in order to implement target async for Windows, gdb will
have to call some functions in a worker thread.

This patch implements the worker thread and moves the necessary
operations there.  Target async isn't yet implemented, so this patch
does not cause any visible changes.
---
 gdb/windows-nat.c | 257 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 182 insertions(+), 75 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 1e310371e47..80cdedce7b9 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -43,6 +43,7 @@
 #endif
 #include <algorithm>
 #include <vector>
+#include <queue>
 
 #include "filenames.h"
 #include "symfile.h"
@@ -244,6 +245,8 @@ static const struct xlate_exception xlate[] =
 
 struct windows_nat_target final : public x86_nat_target<inf_child_target>
 {
+  windows_nat_target ();
+
   void close () override;
 
   void attach (const char *, int) override;
@@ -302,7 +305,8 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
 
   const char *thread_name (struct thread_info *) override;
 
-  int get_windows_debug_event (int pid, struct target_waitstatus *ourstatus);
+  ptid_t get_windows_debug_event (int pid, struct target_waitstatus *ourstatus,
+				  target_wait_flags options);
 
   void do_initial_windows_stuff (DWORD pid, bool attaching);
 
@@ -317,6 +321,34 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
 				   bool main_thread_p);
   void delete_thread (ptid_t ptid, DWORD exit_code, bool main_thread_p);
   DWORD fake_create_process ();
+
+  BOOL windows_continue (DWORD continue_status, int id, int killed);
+
+  /* This function implements the background thread that starts
+     inferiors and waits for events.  */
+  void process_thread ();
+
+  /* Push FUNC onto the queue of requests for process_thread, and wait
+     until it has been called.  On Windows, certain debugging
+     functions can only be called by the thread that started (or
+     attached to) the inferior.  These are all done in the worker
+     thread, via calls to this method.  */
+  void do_synchronously (gdb::function_view<void ()> func);
+
+  /* This waits for a debug event, dispatching to the worker thread as
+     needed.  */
+  void wait_for_debug_event_main_thread (DEBUG_EVENT *event);
+
+  /* Queue used to send requests to process_thread.  This is
+     implicitly locked.  */
+  std::queue<gdb::function_view<void ()>> m_queue;
+
+  /* Event used to signal process_thread that an item has been
+     pushed.  */
+  HANDLE m_pushed_event;
+  /* Event used by process_thread to indicate that it has processed a
+     single function call.  */
+  HANDLE m_response_event;
 };
 
 static windows_nat_target the_windows_nat_target;
@@ -332,6 +364,74 @@ check (BOOL ok, const char *file, int line)
     }
 }
 
+windows_nat_target::windows_nat_target ()
+  : m_pushed_event (CreateEvent (nullptr, false, false, nullptr)),
+    m_response_event (CreateEvent (nullptr, false, false, nullptr))
+{
+  auto fn = [] (LPVOID self) -> DWORD
+    {
+      ((windows_nat_target *) self)->process_thread ();
+      return 0;
+    };
+
+  HANDLE bg_thread = CreateThread (nullptr, 0, fn, this, 0, nullptr);
+  CloseHandle (bg_thread);
+}
+
+/* A wrapper for WaitForSingleObject that issues a warning if
+   something unusual happens.  */
+static void
+wait_for_single (HANDLE handle, DWORD howlong)
+{
+  while (true)
+    {
+      DWORD r = WaitForSingleObject (handle, howlong);
+      if (r == WAIT_OBJECT_0)
+	return;
+      if (r == WAIT_FAILED)
+	{
+	  unsigned err = (unsigned) GetLastError ();
+	  warning ("WaitForSingleObject failed (code %u): %s",
+		   err, strwinerror (err));
+	}
+      else
+	warning ("unexpected result from WaitForSingleObject: %u",
+		 (unsigned) r);
+    }
+}
+
+void
+windows_nat_target::process_thread ()
+{
+  while (true)
+    {
+      wait_for_single (m_pushed_event, INFINITE);
+
+      gdb::function_view<void ()> func = std::move (m_queue.front ());
+      m_queue.pop ();
+
+      func ();
+      SetEvent (m_response_event);
+   }
+}
+
+void
+windows_nat_target::do_synchronously (gdb::function_view<void ()> func)
+{
+  m_queue.emplace (std::move (func));
+  SetEvent (m_pushed_event);
+  wait_for_single (m_response_event, INFINITE);
+}
+
+void
+windows_nat_target::wait_for_debug_event_main_thread (DEBUG_EVENT *event)
+{
+  do_synchronously ([&] ()
+    {
+      wait_for_debug_event (event, INFINITE);
+    });
+}
+
 /* See nat/windows-nat.h.  */
 
 windows_thread_info *
@@ -1079,11 +1179,9 @@ windows_per_inferior::handle_access_violation
    threads, if we are continuing execution.  KILLED non-zero means we
    have killed the inferior, so we should ignore weird errors due to
    threads shutting down.  */
-static BOOL
-windows_continue (DWORD continue_status, int id, int killed)
+BOOL
+windows_nat_target::windows_continue (DWORD continue_status, int id, int killed)
 {
-  BOOL res;
-
   windows_process.desired_stop_thread_id = id;
 
   if (windows_process.matching_pending_stop (debug_events))
@@ -1160,17 +1258,19 @@ windows_continue (DWORD continue_status, int id, int killed)
 	th->suspend ();
       }
 
-  res = continue_last_debug_event (continue_status, debug_events);
-
-  if (!res)
+  gdb::optional<unsigned> err;
+  do_synchronously ([&] ()
     {
-      unsigned err = (unsigned) GetLastError ();
-      error (_("Failed to resume program execution"
-	       " (ContinueDebugEvent failed, error %u: %s)"),
-	     err, strwinerror (err));
-    }
+      if (!continue_last_debug_event (continue_status, debug_events))
+	err = (unsigned) GetLastError ();
+    });
+
+  if (err.has_value ())
+    error (_("Failed to resume program execution"
+	     " (ContinueDebugEvent failed, error %u: %s)"),
+	   *err, strwinerror (*err));
 
-  return res;
+  return TRUE;
 }
 
 /* Called in pathological case where Windows fails to send a
@@ -1376,14 +1476,12 @@ ctrl_c_handler (DWORD event_type)
   return TRUE;
 }
 
-/* Get the next event from the child.  Returns a non-zero thread id if the event
-   requires handling by WFI (or whatever).  */
+/* Get the next event from the child.  Returns the thread ptid.  */
 
-int
-windows_nat_target::get_windows_debug_event (int pid,
-					     struct target_waitstatus *ourstatus)
+ptid_t
+windows_nat_target::get_windows_debug_event
+     (int pid, struct target_waitstatus *ourstatus, target_wait_flags options)
 {
-  BOOL debug_event;
   DWORD continue_status, event_code;
   DWORD thread_id = 0;
 
@@ -1402,15 +1500,13 @@ windows_nat_target::get_windows_debug_event (int pid,
 	= windows_process.thread_rec (ptid, INVALIDATE_CONTEXT);
       th->reload_context = true;
 
-      return thread_id;
+      return ptid;
     }
 
   windows_process.last_sig = GDB_SIGNAL_0;
   DEBUG_EVENT *current_event = &windows_process.current_event;
 
-  if (!(debug_event = wait_for_debug_event (&windows_process.current_event,
-					    1000)))
-    goto out;
+  wait_for_debug_event_main_thread (&windows_process.current_event);
 
   continue_status = DBG_CONTINUE;
 
@@ -1632,7 +1728,9 @@ windows_nat_target::get_windows_debug_event (int pid,
     }
 
 out:
-  return thread_id;
+  if (thread_id == 0)
+    return null_ptid;
+  return ptid_t (windows_process.current_event.dwProcessId, thread_id, 0);
 }
 
 /* Wait for interesting events to occur in the target process.  */
@@ -1650,8 +1748,6 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
   while (1)
     {
-      int retval;
-
       /* If the user presses Ctrl-c while the debugger is waiting
 	 for an event, he expects the debugger to interrupt his program
 	 and to get the prompt back.  There are two possible situations:
@@ -1679,14 +1775,11 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 	     the user tries to resume the execution in the inferior.
 	     This is a classic race that we should try to fix one day.  */
       SetConsoleCtrlHandler (&ctrl_c_handler, TRUE);
-      retval = get_windows_debug_event (pid, ourstatus);
+      ptid_t result = get_windows_debug_event (pid, ourstatus, options);
       SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
 
-      if (retval)
+      if (result != null_ptid)
 	{
-	  ptid_t result = ptid_t (windows_process.current_event.dwProcessId,
-				  retval, 0);
-
 	  if (ourstatus->kind () != TARGET_WAITKIND_EXITED
 	      && ourstatus->kind () !=  TARGET_WAITKIND_SIGNALLED)
 	    {
@@ -1869,7 +1962,6 @@ set_process_privilege (const char *privilege, BOOL enable)
 void
 windows_nat_target::attach (const char *args, int from_tty)
 {
-  BOOL ok;
   DWORD pid;
 
   pid = parse_pid_to_attach (args);
@@ -1879,26 +1971,31 @@ windows_nat_target::attach (const char *args, int from_tty)
 	     "This can cause attach to fail on Windows NT/2K/XP");
 
   windows_init_thread_list ();
-  ok = DebugActiveProcess (pid);
   windows_process.saw_create = 0;
 
-#ifdef __CYGWIN__
-  if (!ok)
+  gdb::optional<unsigned> err;
+  do_synchronously ([&] ()
     {
-      /* Try fall back to Cygwin pid.  */
-      pid = cygwin_internal (CW_CYGWIN_PID_TO_WINPID, pid);
+      BOOL ok = DebugActiveProcess (pid);
 
-      if (pid > 0)
-	ok = DebugActiveProcess (pid);
-  }
+#ifdef __CYGWIN__
+      if (!ok)
+	{
+	  /* Try fall back to Cygwin pid.  */
+	  pid = cygwin_internal (CW_CYGWIN_PID_TO_WINPID, pid);
+
+	  if (pid > 0)
+	    ok = DebugActiveProcess (pid);
+	}
 #endif
 
-  if (!ok)
-    {
-      unsigned err = (unsigned) GetLastError ();
-      error (_("Can't attach to process %u (error %u: %s)"),
-	     (unsigned) pid, err, strwinerror (err));
-    }
+      if (!ok)
+	err = (unsigned) GetLastError ();
+    });
+
+  if (err.has_value ())
+    error (_("Can't attach to process %u (error %u: %s)"),
+	   (unsigned) pid, *err, strwinerror (*err));
 
   DebugSetProcessKillOnExit (FALSE);
 
@@ -1925,14 +2022,19 @@ windows_nat_target::detach (inferior *inf, int from_tty)
   ptid_t ptid = minus_one_ptid;
   resume (ptid, 0, GDB_SIGNAL_0);
 
-  if (!DebugActiveProcessStop (windows_process.current_event.dwProcessId))
+  gdb::optional<unsigned> err;
+  do_synchronously ([&] ()
     {
-      unsigned err = (unsigned) GetLastError ();
-      error (_("Can't detach process %u (error %u: %s)"),
-	     (unsigned) windows_process.current_event.dwProcessId,
-	     err, strwinerror (err));
-    }
-  DebugSetProcessKillOnExit (FALSE);
+      if (!DebugActiveProcessStop (windows_process.current_event.dwProcessId))
+	err = (unsigned) GetLastError ();
+      else
+	DebugSetProcessKillOnExit (FALSE);
+    });
+
+  if (err.has_value ())
+    error (_("Can't detach process %u (error %u: %s)"),
+	   (unsigned) windows_process.current_event.dwProcessId,
+	   *err, strwinerror (*err));
 
   target_announce_detach (from_tty);
 
@@ -2378,7 +2480,7 @@ windows_nat_target::create_inferior (const char *exec_file,
 #endif	/* !__CYGWIN__ */
   const char *allargs = origallargs.c_str ();
   PROCESS_INFORMATION pi;
-  BOOL ret;
+  gdb::optional<unsigned> ret;
   DWORD flags = 0;
   const std::string &inferior_tty = current_inferior ()->tty ();
 
@@ -2485,10 +2587,15 @@ windows_nat_target::create_inferior (const char *exec_file,
     }
 
   windows_init_thread_list ();
-  ret = create_process (nullptr, args, flags, w32_env,
-			inferior_cwd != nullptr ? infcwd : nullptr,
-			disable_randomization,
-			&si, &pi);
+  do_synchronously ([&] ()
+    {
+      if (!create_process (nullptr, args, flags, w32_env,
+			   inferior_cwd != nullptr ? infcwd : nullptr,
+			   disable_randomization,
+			   &si, &pi))
+	ret = (unsigned) GetLastError ();
+    });
+
   if (w32_env)
     /* Just free the Win32 environment, if it could be created. */
     free (w32_env);
@@ -2605,14 +2712,18 @@ windows_nat_target::create_inferior (const char *exec_file,
   *temp = 0;
 
   windows_init_thread_list ();
-  ret = create_process (nullptr, /* image */
-			args,	/* command line */
-			flags,	/* start flags */
-			w32env,	/* environment */
-			inferior_cwd, /* current directory */
-			disable_randomization,
-			&si,
-			&pi);
+  do_synchronously ([&] ()
+    {
+      if (!create_process (nullptr, /* image */
+			   args,	/* command line */
+			   flags,	/* start flags */
+			   w32env,	/* environment */
+			   inferior_cwd, /* current directory */
+			   disable_randomization,
+			   &si,
+			   &pi))
+	ret = (unsigned) GetLastError ();
+    });
   if (tty != INVALID_HANDLE_VALUE)
     CloseHandle (tty);
   if (fd_inp >= 0)
@@ -2623,12 +2734,9 @@ windows_nat_target::create_inferior (const char *exec_file,
     _close (fd_err);
 #endif	/* !__CYGWIN__ */
 
-  if (!ret)
-    {
-      unsigned err = (unsigned) GetLastError ();
-      error (_("Error creating process %s, (error %u: %s)"),
-	     exec_file, err, strwinerror (err));
-    }
+  if (ret.has_value ())
+    error (_("Error creating process %s, (error %u: %s)"),
+	   exec_file, *ret, strwinerror (*ret));
 
 #ifdef __x86_64__
   BOOL wow64;
@@ -2724,8 +2832,7 @@ windows_nat_target::kill ()
     {
       if (!windows_continue (DBG_CONTINUE, -1, 1))
 	break;
-      if (!wait_for_debug_event (&windows_process.current_event, INFINITE))
-	break;
+      wait_for_debug_event_main_thread (&windows_process.current_event);
       if (windows_process.current_event.dwDebugEventCode
 	  == EXIT_PROCESS_DEBUG_EVENT)
 	break;
-- 
2.34.1

[PATCH 2/2] Implement target async for Windows

[Tom Tromey]

This implements target async for Windows.  The basic idea is to have
the worker thread block in WaitForDebugEvent, then notify the event
loop when an event is seen.  In a few situations, this blocking
behavior is undesirable, so the functions passed to do_synchronously
are changed to return a boolean indicating which behavior is needed.
---
 gdb/windows-nat.c | 123 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 108 insertions(+), 15 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 80cdedce7b9..9c277e9a93d 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -34,6 +34,7 @@
 #include <signal.h>
 #include <sys/types.h>
 #include <fcntl.h>
+#include <winsock2.h>
 #include <windows.h>
 #include <imagehlp.h>
 #ifdef __CYGWIN__
@@ -72,6 +73,8 @@
 #include "gdbsupport/gdb_wait.h"
 #include "nat/windows-nat.h"
 #include "gdbsupport/symbol.h"
+#include "ser-event.h"
+#include "inf-loop.h"
 
 using namespace windows_nat;
 
@@ -315,6 +318,23 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
     return disable_randomization_available ();
   }
 
+  bool can_async_p () override
+  {
+    return true;
+  }
+
+  bool is_async_p () override
+  {
+    return m_is_async;
+  }
+
+  void async (bool enable) override;
+
+  int async_wait_fd () override
+  {
+    return serial_event_fd (m_wait_event);
+  }
+
 private:
 
   windows_thread_info *add_thread (ptid_t ptid, HANDLE h, void *tlb,
@@ -322,7 +342,8 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
   void delete_thread (ptid_t ptid, DWORD exit_code, bool main_thread_p);
   DWORD fake_create_process ();
 
-  BOOL windows_continue (DWORD continue_status, int id, int killed);
+  BOOL windows_continue (DWORD continue_status, int id, int killed,
+			 bool last_call = false);
 
   /* This function implements the background thread that starts
      inferiors and waits for events.  */
@@ -332,8 +353,9 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
      until it has been called.  On Windows, certain debugging
      functions can only be called by the thread that started (or
      attached to) the inferior.  These are all done in the worker
-     thread, via calls to this method.  */
-  void do_synchronously (gdb::function_view<void ()> func);
+     thread, via calls to this method.  If FUNC returns true,
+     process_thread will wait for debug events when FUNC returns.  */
+  void do_synchronously (gdb::function_view<bool ()> func);
 
   /* This waits for a debug event, dispatching to the worker thread as
      needed.  */
@@ -341,7 +363,7 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
 
   /* Queue used to send requests to process_thread.  This is
      implicitly locked.  */
-  std::queue<gdb::function_view<void ()>> m_queue;
+  std::queue<gdb::function_view<bool ()>> m_queue;
 
   /* Event used to signal process_thread that an item has been
      pushed.  */
@@ -349,6 +371,18 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
   /* Event used by process_thread to indicate that it has processed a
      single function call.  */
   HANDLE m_response_event;
+
+  /* Serial event used to communicate wait event availability to the
+     main loop.  */
+  serial_event *m_wait_event;
+
+  /* The last debug event, when M_WAIT_EVENT has been set.  */
+  DEBUG_EVENT m_last_debug_event {};
+  /* True if a debug event is pending.  */
+  std::atomic<bool> m_debug_event_pending { false };
+
+  /* True if currently in async mode.  */
+  bool m_is_async = false;
 };
 
 static windows_nat_target the_windows_nat_target;
@@ -366,7 +400,8 @@ check (BOOL ok, const char *file, int line)
 
 windows_nat_target::windows_nat_target ()
   : m_pushed_event (CreateEvent (nullptr, false, false, nullptr)),
-    m_response_event (CreateEvent (nullptr, false, false, nullptr))
+    m_response_event (CreateEvent (nullptr, false, false, nullptr)),
+    m_wait_event (make_serial_event ())
 {
   auto fn = [] (LPVOID self) -> DWORD
     {
@@ -378,6 +413,23 @@ windows_nat_target::windows_nat_target ()
   CloseHandle (bg_thread);
 }
 
+void
+windows_nat_target::async (bool enable)
+{
+  if (enable == is_async_p ())
+    return;
+
+  if (enable)
+    add_file_handler (async_wait_fd (),
+		      [] (int, gdb_client_data)
+		      {
+			inferior_event_handler (INF_REG_EVENT);
+		      },
+		      nullptr, "windows_nat_target");
+  else
+    delete_file_handler (async_wait_fd ());
+}
+
 /* A wrapper for WaitForSingleObject that issues a warning if
    something unusual happens.  */
 static void
@@ -407,16 +459,26 @@ windows_nat_target::process_thread ()
     {
       wait_for_single (m_pushed_event, INFINITE);
 
-      gdb::function_view<void ()> func = std::move (m_queue.front ());
+      gdb::function_view<bool ()> func = std::move (m_queue.front ());
       m_queue.pop ();
 
-      func ();
+      bool should_wait = func ();
       SetEvent (m_response_event);
+
+      if (should_wait)
+	{
+	  if (!m_debug_event_pending)
+	    {
+	      wait_for_debug_event (&m_last_debug_event, INFINITE);
+	      m_debug_event_pending = true;
+	    }
+	  serial_event_set (m_wait_event);
+	}
    }
 }
 
 void
-windows_nat_target::do_synchronously (gdb::function_view<void ()> func)
+windows_nat_target::do_synchronously (gdb::function_view<bool ()> func)
 {
   m_queue.emplace (std::move (func));
   SetEvent (m_pushed_event);
@@ -428,7 +490,15 @@ windows_nat_target::wait_for_debug_event_main_thread (DEBUG_EVENT *event)
 {
   do_synchronously ([&] ()
     {
-      wait_for_debug_event (event, INFINITE);
+      if (m_debug_event_pending)
+	{
+	  *event = m_last_debug_event;
+	  m_debug_event_pending = false;
+	  serial_event_clear (m_wait_event);
+	}
+      else
+	wait_for_debug_event (event, INFINITE);
+      return false;
     });
 }
 
@@ -1178,14 +1248,23 @@ windows_per_inferior::handle_access_violation
 /* Resume thread specified by ID, or all artificially suspended
    threads, if we are continuing execution.  KILLED non-zero means we
    have killed the inferior, so we should ignore weird errors due to
-   threads shutting down.  */
+   threads shutting down.  LAST_CALL is true if we expect this to be
+   the last call to continue the inferior -- we are either mourning it
+   or detaching.  */
 BOOL
-windows_nat_target::windows_continue (DWORD continue_status, int id, int killed)
+windows_nat_target::windows_continue (DWORD continue_status, int id,
+				      int killed, bool last_call)
 {
   windows_process.desired_stop_thread_id = id;
 
   if (windows_process.matching_pending_stop (debug_events))
-    return TRUE;
+    {
+      /* There's no need to really continue, because there's already
+	 another event pending.  However, we do need to inform the
+	 event loop of this.  */
+      serial_event_set (m_wait_event);
+      return TRUE;
+    }
 
   for (auto &th : windows_process.thread_list)
     if (id == -1 || id == (int) th->tid)
@@ -1263,6 +1342,9 @@ windows_nat_target::windows_continue (DWORD continue_status, int id, int killed)
     {
       if (!continue_last_debug_event (continue_status, debug_events))
 	err = (unsigned) GetLastError ();
+      /* On the last call, do not block waiting for an event that will
+	 never come.  */
+      return !last_call;
     });
 
   if (err.has_value ())
@@ -1506,6 +1588,12 @@ windows_nat_target::get_windows_debug_event
   windows_process.last_sig = GDB_SIGNAL_0;
   DEBUG_EVENT *current_event = &windows_process.current_event;
 
+  if ((options & TARGET_WNOHANG) != 0 && !m_debug_event_pending)
+    {
+      ourstatus->set_ignore ();
+      return minus_one_ptid;
+    }
+
   wait_for_debug_event_main_thread (&windows_process.current_event);
 
   continue_status = DBG_CONTINUE;
@@ -1991,6 +2079,8 @@ windows_nat_target::attach (const char *args, int from_tty)
 
       if (!ok)
 	err = (unsigned) GetLastError ();
+
+      return true;
     });
 
   if (err.has_value ())
@@ -2019,8 +2109,7 @@ windows_nat_target::attach (const char *args, int from_tty)
 void
 windows_nat_target::detach (inferior *inf, int from_tty)
 {
-  ptid_t ptid = minus_one_ptid;
-  resume (ptid, 0, GDB_SIGNAL_0);
+  windows_continue (DBG_CONTINUE, -1, 0, true);
 
   gdb::optional<unsigned> err;
   do_synchronously ([&] ()
@@ -2029,6 +2118,7 @@ windows_nat_target::detach (inferior *inf, int from_tty)
 	err = (unsigned) GetLastError ();
       else
 	DebugSetProcessKillOnExit (FALSE);
+      return false;
     });
 
   if (err.has_value ())
@@ -2594,6 +2684,7 @@ windows_nat_target::create_inferior (const char *exec_file,
 			   disable_randomization,
 			   &si, &pi))
 	ret = (unsigned) GetLastError ();
+      return true;
     });
 
   if (w32_env)
@@ -2723,6 +2814,7 @@ windows_nat_target::create_inferior (const char *exec_file,
 			   &si,
 			   &pi))
 	ret = (unsigned) GetLastError ();
+      return true;
     });
   if (tty != INVALID_HANDLE_VALUE)
     CloseHandle (tty);
@@ -2760,7 +2852,7 @@ windows_nat_target::create_inferior (const char *exec_file,
 void
 windows_nat_target::mourn_inferior ()
 {
-  (void) windows_continue (DBG_CONTINUE, -1, 0);
+  (void) windows_continue (DBG_CONTINUE, -1, 0, true);
   x86_cleanup_dregs();
   if (windows_process.open_process_used)
     {
@@ -2845,6 +2937,7 @@ void
 windows_nat_target::close ()
 {
   DEBUG_EVENTS ("inferior_ptid=%d\n", inferior_ptid.pid ());
+  async (false);
 }
 
 /* Convert pid to printable format.  */
-- 
2.34.1

Re: [PATCH 1/2] Move some Windows operations to worker thread

[Eli Zaretskii]

> Date: Wed,  3 Aug 2022 07:08:21 -0600
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Tom Tromey <tromey@adacore.com>
> 
> On Windows, certain debugging APIs can only be called from the thread
> that started (or attached) to the inferior.  Also, there is no way on
> Windows to wait for a debug event in addition to other events.
> Therefore, in order to implement target async for Windows, gdb will
> have to call some functions in a worker thread.
> 
> This patch implements the worker thread and moves the necessary
> operations there.  Target async isn't yet implemented, so this patch
> does not cause any visible changes.

Thanks!

> +  HANDLE bg_thread = CreateThread (nullptr, 0, fn, this, 0, nullptr);

That zero in the 2nd argument of CreateThread means the worker thread
will get the same stack size as the value recorded in the GDB
executable file's header, which is 12MB according to my reading of
gdb/Makefile, at least in my build?  That's unlikely to be required
for this thread, so I suggest to specify a non-zero value there
instead.  I guess 64KB should be enough?

The downside of leaving it at zero is that the OS reserves the
addresses of the entire stack space as for the GDB process, which
could then prevent GDB from allocating enough memory for other
purposes.  And if we could start several such worker threads (is that
possible in some usage scenario?), then we could simply run out of
memory for no good reason, and the next thread will fail to start.

Re: [PATCH 0/2] Implement target async for Windows

[Eli Zaretskii]

> Date: Wed,  3 Aug 2022 07:08:20 -0600
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> 
> I wanted target async to work on Windows for another project.  This
> short series is the result.

Thanks.  I think this is important enough to be in NEWS.

Re: [PATCH 1/2] Move some Windows operations to worker thread

[Tom Tromey]

>> +  HANDLE bg_thread = CreateThread (nullptr, 0, fn, this, 0, nullptr);

Eli> That zero in the 2nd argument of CreateThread means the worker thread
Eli> will get the same stack size as the value recorded in the GDB
Eli> executable file's header, which is 12MB according to my reading of
Eli> gdb/Makefile, at least in my build?  That's unlikely to be required
Eli> for this thread, so I suggest to specify a non-zero value there
Eli> instead.  I guess 64KB should be enough?

I changed it to `64 * 1024` and re-tested, and this worked ok.

Tom

Re: [PATCH 0/2] Implement target async for Windows

[Tom Tromey]

Eli> Thanks.  I think this is important enough to be in NEWS.

How about this?

Tom

diff --git a/gdb/NEWS b/gdb/NEWS
index 8c837df76e5..66ec883d782 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -55,6 +55,8 @@
   Python Pygments is still used.  For supported targets, libopcodes
   styling is used by default.
 
+* The Windows native target now supports target async.
+
 * New commands
 
 maintenance set ignore-prologue-end-flag on|off

Re: [PATCH 1/2] Move some Windows operations to worker thread

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>,  gdb-patches@sourceware.org
> Date: Wed, 03 Aug 2022 12:47:24 -0600
> 
> >> +  HANDLE bg_thread = CreateThread (nullptr, 0, fn, this, 0, nullptr);
> 
> Eli> That zero in the 2nd argument of CreateThread means the worker thread
> Eli> will get the same stack size as the value recorded in the GDB
> Eli> executable file's header, which is 12MB according to my reading of
> Eli> gdb/Makefile, at least in my build?  That's unlikely to be required
> Eli> for this thread, so I suggest to specify a non-zero value there
> Eli> instead.  I guess 64KB should be enough?
> 
> I changed it to `64 * 1024` and re-tested, and this worked ok.

Thanks.

Re: [PATCH 0/2] Implement target async for Windows

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>,  gdb-patches@sourceware.org
> Date: Wed, 03 Aug 2022 12:54:42 -0600
> 
> Eli> Thanks.  I think this is important enough to be in NEWS.
> 
> How about this?
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 8c837df76e5..66ec883d782 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -55,6 +55,8 @@
>    Python Pygments is still used.  For supported targets, libopcodes
>    styling is used by default.
>  
> +* The Windows native target now supports target async.
> +
>  * New commands
>  
>  maintenance set ignore-prologue-end-flag on|off
> 

This is fine, thanks.

Re: [PATCH 0/2] Implement target async for Windows

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> I wanted target async to work on Windows for another project.  This
Tom> short series is the result.

I'm checking these in now.

Tom

Re: [PATCH 2/2] Implement target async for Windows

[Jon Turney]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

On 03/08/2022 14:08, Tom Tromey via Gdb-patches wrote:
> This implements target async for Windows.  The basic idea is to have
> the worker thread block in WaitForDebugEvent, then notify the event
> loop when an event is seen.  In a few situations, this blocking
> behavior is undesirable, so the functions passed to do_synchronously
> are changed to return a boolean indicating which behavior is needed.
> ---
>   gdb/windows-nat.c | 123 ++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 108 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 80cdedce7b9..9c277e9a93d 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -34,6 +34,7 @@
>   #include <signal.h>
>   #include <sys/types.h>
>   #include <fcntl.h>
> +#include <winsock2.h>
>   #include <windows.h>
>   #include <imagehlp.h>
>   #ifdef __CYGWIN__
This breaks the build on Cygwin, as including both winsock2 and Cygwin's 
sys/select.h is a big no-no.

I'm going to suggest the attached to fix this, but from a quick reading 
of the patch, I just wonder if winsock2.h is actually needed at all, as 
I don't immediately see anything that leaps out as needing it...

[-- Attachment #2: 0001-Fix-Cygwin-build-after-d08bae3d.patch --]
[-- Type: text/plain, Size: 787 bytes --]

From c4d35aa363a8037746ae8bd17f7b59f3c581e69f Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Wed, 2 Nov 2022 14:32:55 +0000
Subject: [PATCH] Fix Cygwin build after d08bae3d

Commit d08bae3d ("Implement target async for Windows") unconditionally
includes winsock2.h.  Don't do this on Cygwin, since we don't want to
use Windows socket functions.
---
 gdb/windows-nat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index ab94de03bbf..7342c71d7db 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -34,7 +34,9 @@
 #include <signal.h>
 #include <sys/types.h>
 #include <fcntl.h>
+#ifndef __CYGWIN__
 #include <winsock2.h>
+#endif
 #include <windows.h>
 #include <imagehlp.h>
 #ifdef __CYGWIN__
-- 
2.38.1

Re: [PATCH 2/2] Implement target async for Windows

[Jon Turney]

[-- Attachment #1: Type: text/plain, Size: 2917 bytes --]

On 03/08/2022 14:08, Tom Tromey via Gdb-patches wrote:
> This implements target async for Windows.  The basic idea is to have
> the worker thread block in WaitForDebugEvent, then notify the event
> loop when an event is seen.  In a few situations, this blocking
> behavior is undesirable, so the functions passed to do_synchronously
> are changed to return a boolean indicating which behavior is needed.
> ---
>   gdb/windows-nat.c | 123 ++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 108 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 80cdedce7b9..9c277e9a93d 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
[...]
>   
>   static windows_nat_target the_windows_nat_target;
> @@ -366,7 +400,8 @@ check (BOOL ok, const char *file, int line)
>   
>   windows_nat_target::windows_nat_target ()
>     : m_pushed_event (CreateEvent (nullptr, false, false, nullptr)),
> -    m_response_event (CreateEvent (nullptr, false, false, nullptr))
> +    m_response_event (CreateEvent (nullptr, false, false, nullptr)),
> +    m_wait_event (make_serial_event ())
>   {
>     auto fn = [] (LPVOID self) -> DWORD
>       {
This introduces a crash on gdb startup for me.

> Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
> ops=ops@entry=0x100abfca0 <serial_event_ops>, open_name=open_name@entry=0x0) at ../../gdb/serial.c:249
> 249       if (!serial_logfile.empty ())
> (top-gdb) p serial_logfile
> $1 = <error reading variable: Cannot access memory at address 0xffffffffffffffe8>
> (top-gdb) bt
> #0  0x000000010071e755 in serial_open_ops_1 (ops=ops@entry=0x100abfca0 <serial_event_ops>, open_name=open_name@entry=0x0) at ../../gdb/serial.c:249
> #1  0x000000010071ec77 in serial_open_ops (ops=ops@entry=0x100abfca0 <serial_event_ops>) at ../../gdb/serial.c:267
> #2  0x000000010071d0ac in make_serial_event () at ../../gdb/ser-event.c:164
> #3  0x0000000100810f9e in windows_nat_target::windows_nat_target (this=0x100cdf2e0 <the_windows_nat_target>) at ../../gdb/windows-nat.c:406
> #4  0x00000001009d77f0 in __static_initialization_and_destruction_0 (__initialize_p=1, __priority=65535) at ../../gdb/windows-nat.c:390
> #5  _GLOBAL__sub_I__ZN18windows_nat_targetC2Ev () at ../../gdb/windows-nat.c:3311
> #6  0x00007fff152a6582 in do_global_ctors (force=0, in_pfunc=0x1009d8700 <___CTOR_LIST__>) at ../../../../src/winsup/cygwin/dcrt0.cc:78
> #7  __main () at ../../../../src/winsup/cygwin/dcrt0.cc:1084
> #8  0x00000001009cd7d0 in main (argc=1, argv=0x7ffffcc50) at ../../gdb/gdb.c:25

This looks like an problem with static object construction ordering: 
the serial_logfile object needs to be constructed before before 
the_windows_nat_target object, but nothing enforces that, currently.

Attached is a patch I wrote to verify the issue, and applying it fixes 
the crash for me, but I'm sure there is a more elegant solution out there...

[-- Attachment #2: 0001-Avoid-crash-due-to-undefined-ordering-of-static-obje.patch --]
[-- Type: text/plain, Size: 3392 bytes --]

From 11d56dbbd8034eaa1f128f9b4a5fdcbf16cdbb9e Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Fri, 4 Nov 2022 14:14:41 +0000
Subject: [PATCH] Avoid crash due to undefined ordering of static object
 construction

This looks like an issue with static object construction ordering:
the serial_logfile object needs to be constructed before before
the_windows_nat_target object, but nothing enforces that.

Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
ops=ops@entry=0x100abfca0 <serial_event_ops>, open_name=open_name@entry=0x0) at ../../gdb/serial.c:249
249       if (!serial_logfile.empty ())
> (top-gdb) p serial_logfile
> $1 = <error reading variable: Cannot access memory at address 0xffffffffffffffe8>
(top-gdb) bt
0  0x000000010071e755 in serial_open_ops_1 (ops=ops@entry=0x100abfca0 <serial_event_ops>, open_name=open_name@entry=0x0) at ../../gdb/serial.c:249
1  0x000000010071ec77 in serial_open_ops (ops=ops@entry=0x100abfca0 <serial_event_ops>) at ../../gdb/serial.c:267
2  0x000000010071d0ac in make_serial_event () at ../../gdb/ser-event.c:164
3  0x0000000100810f9e in windows_nat_target::windows_nat_target (this=0x100cdf2e0 <the_windows_nat_target>) at ../../gdb/windows-nat.c:406
4  0x00000001009d77f0 in __static_initialization_and_destruction_0 (__initialize_p=1, __priority=65535) at ../../gdb/windows-nat.c:390
5  _GLOBAL__sub_I__ZN18windows_nat_targetC2Ev () at ../../gdb/windows-nat.c:3311
6  0x00007fff152a6582 in do_global_ctors (force=0, in_pfunc=0x1009d8700 <___CTOR_LIST__>) at ../../../../src/winsup/cygwin/dcrt0.cc:78
7  __main () at ../../../../src/winsup/cygwin/dcrt0.cc:1084
8  0x00000001009cd7d0 in main (argc=1, argv=0x7ffffcc50) at ../../gdb/gdb.c:25
---
 gdb/serial.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/gdb/serial.c b/gdb/serial.c
index b304807c597..1cc0d2a9b5d 100644
--- a/gdb/serial.c
+++ b/gdb/serial.c
@@ -38,7 +38,13 @@ static struct serial *scb_base;
 /* Non-NULL gives filename which contains a recording of the remote session,
    suitable for playback by gdbserver.  */
 
-static std::string serial_logfile;
+static inline
+std::string &serial_logfile()
+{
+  static std::string serial_logfile_;
+  return serial_logfile_;
+}
+
 static struct ui_file *serial_logfp = NULL;
 
 static const struct serial_ops *serial_interface_lookup (const char *);
@@ -246,12 +252,12 @@ serial_open_ops_1 (const struct serial_ops *ops, const char *open_name)
   scb->next = scb_base;
   scb_base = scb;
 
-  if (!serial_logfile.empty ())
+  if (!serial_logfile().empty ())
     {
       stdio_file_up file (new stdio_file ());
 
-      if (!file->open (serial_logfile.c_str (), "w"))
-	perror_with_name (serial_logfile.c_str ());
+      if (!file->open (serial_logfile().c_str (), "w"))
+	perror_with_name (serial_logfile().c_str ());
 
       serial_logfp = file.release ();
     }
@@ -688,7 +694,7 @@ Show parity for remote serial I/O."), NULL,
 			NULL, /* FIXME: i18n: */
 			&serial_set_cmdlist, &serial_show_cmdlist);
 
-  add_setshow_filename_cmd ("remotelogfile", no_class, &serial_logfile, _("\
+  add_setshow_filename_cmd ("remotelogfile", no_class, &serial_logfile(), _("\
 Set filename for remote session recording."), _("\
 Show filename for remote session recording."), _("\
 This file is used to record the remote session for future playback\n\
-- 
2.38.1

Re: [PATCH 2/2] Implement target async for Windows

[Tom Tromey]

>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:

Jon> I'm going to suggest the attached to fix this, but from a quick
Jon> reading of the patch, I just wonder if winsock2.h is actually needed
Jon> at all, as I don't immediately see anything that leaps out as needing
Jon> it...

I don't remember why I added it, but I suspect you're right and that the
include can just be removed.

Tom

Re: [PATCH 2/2] Implement target async for Windows

[Tom Tromey]

>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:

>> Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
>> ops=ops@entry=0x100abfca0 <serial_event_ops>, open_name=open_name@entry=0x0) at ../../gdb/serial.c:249
>> 249       if (!serial_logfile.empty ())
>> (top-gdb) p serial_logfile
>> $1 = <error reading variable: Cannot access memory at address 0xffffffffffffffe8>
>> (top-gdb) bt
>> #0  0x000000010071e755 in serial_open_ops_1 (ops=ops@entry=0x100abfca0 <serial_event_ops>, open_name=open_name@entry=0x0) at ../../gdb/serial.c:249
>> #1  0x000000010071ec77 in serial_open_ops (ops=ops@entry=0x100abfca0 <serial_event_ops>) at ../../gdb/serial.c:267
>> #2  0x000000010071d0ac in make_serial_event () at ../../gdb/ser-event.c:164
>> #3  0x0000000100810f9e in windows_nat_target::windows_nat_target (this=0x100cdf2e0 <the_windows_nat_target>) at ../../gdb/windows-nat.c:406
>> #4  0x00000001009d77f0 in __static_initialization_and_destruction_0 (__initialize_p=1, __priority=65535) at ../../gdb/windows-nat.c:390
>> #5  _GLOBAL__sub_I__ZN18windows_nat_targetC2Ev () at ../../gdb/windows-nat.c:3311
>> #6  0x00007fff152a6582 in do_global_ctors (force=0, in_pfunc=0x1009d8700 <___CTOR_LIST__>) at ../../../../src/winsup/cygwin/dcrt0.cc:78
>> #7  __main () at ../../../../src/winsup/cygwin/dcrt0.cc:1084
>> #8  0x00000001009cd7d0 in main (argc=1, argv=0x7ffffcc50) at ../../gdb/gdb.c:25

Jon> This looks like an problem with static object construction ordering:
Jon> the serial_logfile object needs to be constructed before before 
Jon> the_windows_nat_target object, but nothing enforces that, currently.

Ugh...

Jon> Attached is a patch I wrote to verify the issue, and applying it fixes
Jon> the crash for me, but I'm sure there is a more elegant solution out
Jon> there...

It seems to me that serial.c is blameless and that windows-nat should
probably not have a static initializer with dependencies.

Could you try this change instead?

Tom

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index ab94de03bbf..ecd5cf7e3fb 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -385,7 +385,9 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
   bool m_is_async = false;
 };
 
-static windows_nat_target the_windows_nat_target;
+/* This is a pointer and not a global specifically to avoid a C++
+   "static initializer fiasco" situation.  */
+static windows_nat_target *the_windows_nat_target;
 
 static void
 check (BOOL ok, const char *file, int line)
@@ -621,7 +623,7 @@ windows_nat_target::delete_thread (ptid_t ptid, DWORD exit_code,
 		target_pid_to_str (ptid).c_str (),
 		(unsigned) exit_code);
 
-  ::delete_thread (find_thread_ptid (&the_windows_nat_target, ptid));
+  ::delete_thread (find_thread_ptid (the_windows_nat_target, ptid));
 
   auto iter = std::find_if (windows_process.thread_list.begin (),
 			    windows_process.thread_list.end (),
@@ -3119,7 +3121,8 @@ _initialize_windows_nat ()
      calling x86_set_debug_register_length function
      in processor windows specific native file.  */
 
-  add_inf_child_target (&the_windows_nat_target);
+  the_windows_nat_target = new windows_nat_target;
+  add_inf_child_target (the_windows_nat_target);
 
 #ifdef __CYGWIN__
   cygwin_internal (CW_SET_DOS_FILE_WARNING, 0);

Re: [PATCH 2/2] Implement target async for Windows

[Jon Turney]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On 08/11/2022 18:38, Tom Tromey via Gdb-patches wrote:
>>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:
> 
> Jon> I'm going to suggest the attached to fix this, but from a quick
> Jon> reading of the patch, I just wonder if winsock2.h is actually needed
> Jon> at all, as I don't immediately see anything that leaps out as needing
> Jon> it...
> 
> I don't remember why I added it, but I suspect you're right and that the
> include can just be removed.

I tested building on MSYS2 with that removed, which appears to be 
successful, so I've pushed as trivial the attached.


[-- Attachment #2: 0001-Drop-apparently-unneeded-include-of-winsock2.h.patch --]
[-- Type: text/plain, Size: 896 bytes --]

From 90f902daf5edb817d57b5e377d7ad54948a5a9f4 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Wed, 2 Nov 2022 14:32:55 +0000
Subject: [PATCH] Drop apparently unneeded include of winsock2.h

Commit d08bae3d ("Implement target async for Windows") unconditionally
includes winsock2.h.  We don't want to do that on Cygwin, since
including both winsock2.h and sys/select.h causes incompatible
redefinition problems.

Since that include is apparently unneeded, just drop it.

Fixes: d08bae3d
---
 gdb/windows-nat.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index ab94de03bbf..6250cbc27a5 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -34,7 +34,6 @@
 #include <signal.h>
 #include <sys/types.h>
 #include <fcntl.h>
-#include <winsock2.h>
 #include <windows.h>
 #include <imagehlp.h>
 #ifdef __CYGWIN__
-- 
2.38.1

Re: [PATCH 2/2] Implement target async for Windows

[Jon Turney]

On 08/11/2022 18:52, Tom Tromey via Gdb-patches wrote:
>>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:
> 
>>> Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
>>> ops=ops@entry=0x100abfca0 <serial_event_ops>, open_name=open_name@entry=0x0) at ../../gdb/serial.c:249
>>> 249       if (!serial_logfile.empty ())
>>> (top-gdb) p serial_logfile
>>> $1 = <error reading variable: Cannot access memory at address 0xffffffffffffffe8>
>>> (top-gdb) bt
>>> #0  0x000000010071e755 in serial_open_ops_1 (ops=ops@entry=0x100abfca0 <serial_event_ops>, open_name=open_name@entry=0x0) at ../../gdb/serial.c:249
>>> #1  0x000000010071ec77 in serial_open_ops (ops=ops@entry=0x100abfca0 <serial_event_ops>) at ../../gdb/serial.c:267
>>> #2  0x000000010071d0ac in make_serial_event () at ../../gdb/ser-event.c:164
>>> #3  0x0000000100810f9e in windows_nat_target::windows_nat_target (this=0x100cdf2e0 <the_windows_nat_target>) at ../../gdb/windows-nat.c:406
>>> #4  0x00000001009d77f0 in __static_initialization_and_destruction_0 (__initialize_p=1, __priority=65535) at ../../gdb/windows-nat.c:390
>>> #5  _GLOBAL__sub_I__ZN18windows_nat_targetC2Ev () at ../../gdb/windows-nat.c:3311
>>> #6  0x00007fff152a6582 in do_global_ctors (force=0, in_pfunc=0x1009d8700 <___CTOR_LIST__>) at ../../../../src/winsup/cygwin/dcrt0.cc:78
>>> #7  __main () at ../../../../src/winsup/cygwin/dcrt0.cc:1084
>>> #8  0x00000001009cd7d0 in main (argc=1, argv=0x7ffffcc50) at ../../gdb/gdb.c:25
> 
> Jon> This looks like an problem with static object construction ordering:
> Jon> the serial_logfile object needs to be constructed before before
> Jon> the_windows_nat_target object, but nothing enforces that, currently.
> 
> Ugh...

I was a bit sloppy in the way I described the issue here, using 'static' 
as a shortcut for 'static storage duration'.

Using the language of the standard, the cause would probably be better 
described as "unordered dynamic initialization of non-local variables 
with static storage duration" or something like that.

> Jon> Attached is a patch I wrote to verify the issue, and applying it fixes
> Jon> the crash for me, but I'm sure there is a more elegant solution out
> Jon> there...
> 
> It seems to me that serial.c is blameless and that windows-nat should
> probably not have a static initializer with dependencies.
> 
> Could you try this change instead?

Yes, that fixes the crash, also.

Please apply, if you are happy with it.

Re: [PATCH 2/2] Implement target async for Windows

[Tom Tromey]

>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:

Jon> Using the language of the standard, the cause would probably be better
Jon> described as "unordered dynamic initialization of non-local variables 
Jon> with static storage duration" or something like that.

The normal name in the C++ community is the "static initialization order
fiasco":

https://en.cppreference.com/w/cpp/language/siof

Jon> Yes, that fixes the crash, also.

Jon> Please apply, if you are happy with it.

I'll check it in shortly.  Thanks for trying it.

Tom

Re: [PATCH 2/2] Implement target async for Windows

[Tom Tromey]

Tom> I'll check it in shortly.  Thanks for trying it.

Here's what I'm checking in.

Tom

commit c2659a9b7bf554ca8dae011031e5003059b8621f
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Nov 8 12:14:20 2022 -0700

    Fix static initialization order problem in windows-nat.c
    
    This patch fixes a static initialization order problem in
    windows-nat.c that was pointed out by Jon Turney.  The underlying
    problem is that the windows_nat_target constructor relies on
    serial_logfile already being constructed, but this is not enforced by
    C++ rules.  This patch fixes the problem by initializing the global
    windows_nat_target later.

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 6250cbc27a5..5d506507b6d 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -384,7 +384,9 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
   bool m_is_async = false;
 };
 
-static windows_nat_target the_windows_nat_target;
+/* This is a pointer and not a global specifically to avoid a C++
+   "static initializer fiasco" situation.  */
+static windows_nat_target *the_windows_nat_target;
 
 static void
 check (BOOL ok, const char *file, int line)
@@ -620,7 +622,7 @@ windows_nat_target::delete_thread (ptid_t ptid, DWORD exit_code,
 		target_pid_to_str (ptid).c_str (),
 		(unsigned) exit_code);
 
-  ::delete_thread (find_thread_ptid (&the_windows_nat_target, ptid));
+  ::delete_thread (find_thread_ptid (the_windows_nat_target, ptid));
 
   auto iter = std::find_if (windows_process.thread_list.begin (),
 			    windows_process.thread_list.end (),
@@ -3118,7 +3120,8 @@ _initialize_windows_nat ()
      calling x86_set_debug_register_length function
      in processor windows specific native file.  */
 
-  add_inf_child_target (&the_windows_nat_target);
+  the_windows_nat_target = new windows_nat_target;
+  add_inf_child_target (the_windows_nat_target);
 
 #ifdef __CYGWIN__
   cygwin_internal (CW_SET_DOS_FILE_WARNING, 0);