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

[Jon Turney <jon.turney@dronecode.org.uk>]

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