[PATCH 0/5] Coredump under 'ulimit -c' control (v2)

[PATCH 0/5] Coredump under 'ulimit -c' control (v2)

[Jon Turney]

Write a coredump under 'ulimit -c' control and related changes.

The idea here is to make debugging using a coredump work as usual on a unix,
e.g.:

$ ulimit -c unlimited

$ ./segv-program
*** starting '"C:\cygwin64\bin\dumper.exe" "C:\cygwin64\work\segv-program.exe" 16156' for pid 1398, tid 7136

$ gdb segv-program.exe segv-program.exe.core
[...]

Jon Turney (5):
  Cygwin: Make 'ulimit -c' control writing a coredump
  Cygwin: Disable writing core dumps by default.
  Cygwin: Define and use __WCOREFLAG
  Cygwin: Treat api_fatal() similarly to a core-dumping signal
  Cygwin: Update documentation for cygwin_stackdump

 winsup/cygwin/dcrt0.cc                |   6 +-
 winsup/cygwin/environ.cc              |   1 +
 winsup/cygwin/exceptions.cc           | 122 ++++++++++++++++++++++----
 winsup/cygwin/include/cygwin/wait.h   |   5 +-
 winsup/cygwin/local_includes/winsup.h |   2 +
 winsup/cygwin/mm/cygheap.cc           |   2 +-
 winsup/cygwin/release/3.5.0           |   7 ++
 winsup/doc/cygwinenv.xml              |  25 ++++--
 winsup/doc/misc-funcs.xml             |   4 +
 winsup/doc/new-features.xml           |  12 +++
 winsup/doc/utils.xml                  |  43 +++++----
 11 files changed, 180 insertions(+), 49 deletions(-)

-- 
2.43.0

[PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney]

Pre-format a command to be executed on a fatal error to run 'dumper'
(using an absolute path).

Factor out executing a pre-formatted command, so we can use that for
invoking the JIT debugger in try_to_debug() (if error_start is present
in the CYGWIN env var) and to invoke dumper when a fatal error occurs.

On a fatal error, if the core file size limit is greater than 1MB,
invoke dumper to write a core dump. Otherwise, if that limit is greater
than 0, write a .stackdump file, as previously.

Adjust and clarify the associated documentation.

Also: Fix so that the error_start JIT debugger is now invoked, even when
ulimit -c is zero.

Also: Fix uses of console_printf() inside exec_prepared_command(). It's
output is written via the Windows console device, so needs to use
Windows-style line endings.

Future work: Truncate or remove the file written, if it exceeds the
maximum size set by the ulimit.

Future work: Using the words "fatal error" could probably be improved
on. This means exiting on one of the "certain signals whose default
action is to cause the process to terminate and produce a core dump
file".
---
 winsup/cygwin/environ.cc              |   1 +
 winsup/cygwin/exceptions.cc           | 104 +++++++++++++++++++++-----
 winsup/cygwin/local_includes/winsup.h |   1 +
 winsup/cygwin/release/3.5.0           |   4 +
 winsup/doc/cygwinenv.xml              |  25 +++++--
 winsup/doc/new-features.xml           |   6 ++
 winsup/doc/utils.xml                  |  43 +++++++----
 7 files changed, 143 insertions(+), 41 deletions(-)

diff --git a/winsup/cygwin/environ.cc b/winsup/cygwin/environ.cc
index 008854a07..dca5c5db0 100644
--- a/winsup/cygwin/environ.cc
+++ b/winsup/cygwin/environ.cc
@@ -832,6 +832,7 @@ environ_init (char **envp, int envc)
     out:
       findenv_func = (char * (*)(const char*, int*)) my_findenv;
       environ = envp;
+      dumper_init ();
       if (envp_passed_in)
 	{
 	  p = getenv ("CYGWIN");
diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 642afb788..6bd34392a 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -52,6 +52,7 @@ details. */
 #define DUMPSTACK_FRAME_LIMIT    32
 
 PWCHAR debugger_command;
+PWCHAR dumper_command;
 extern uint8_t _sigbe;
 extern uint8_t _sigdelayed_end;
 
@@ -132,6 +133,42 @@ error_start_init (const char *buf)
   wcscat (cp, L"\"");
 }
 
+extern "C" void
+dumper_init (void)
+{
+  WCHAR dll_dir[PATH_MAX];
+  if (!GetModuleFileNameW (cygwin_hmodule, dll_dir, PATH_MAX))
+    return;
+
+  /* Strip off last path component ("\\cygwin1.dll") */
+  PWCHAR w = wcsrchr (dll_dir, L'\\');
+  if (!w)
+    return;
+
+  *w = L'\0';
+
+  /* Calculate the length of the command, allowing for an appended DWORD PID and
+     terminating null */
+  int cmd_len = 1 + wcslen(dll_dir) + 11 + 2 + 1 + wcslen(global_progname) + 1 + 10 + 1;
+  if (cmd_len > 32767)
+    {
+      /* If this comes to more than the 32,767 characters CreateProcess() can
+           accept, we can't work, so don't set dumper_command */
+      return;
+    }
+
+  dumper_command = (PWCHAR) malloc(cmd_len * sizeof (WCHAR));
+
+  PWCHAR cp = dumper_command;
+  cp = wcpcpy (cp, L"\"");
+  cp = wcpcpy (cp, dll_dir);
+  cp = wcpcpy (cp, L"\\dumper.exe");
+  cp = wcpcpy (cp, L"\" ");
+  cp = wcpcpy (cp, L"\"");
+  cp = wcpcpy (cp, global_progname);
+  wcscat (cp, L"\"");
+}
+
 void
 cygwin_exception::open_stackdumpfile ()
 {
@@ -454,20 +491,14 @@ cygwin_stackdump ()
   exc.dumpstack ();
 }
 
-extern "C" int
-try_to_debug ()
+static
+int exec_prepared_command (PWCHAR command)
 {
-  if (!debugger_command)
+  if (!command)
     return 0;
-  debug_printf ("debugger_command '%W'", debugger_command);
-  if (being_debugged ())
-    {
-      extern void break_here ();
-      break_here ();
-      return 0;
-    }
+  debug_printf ("executing prepared command '%W'", command);
 
-  PWCHAR dbg_end = wcschr (debugger_command, L'\0');
+  PWCHAR dbg_end = wcschr (command, L'\0');
   __small_swprintf (dbg_end, L" %u", GetCurrentProcessId ());
 
   LONG prio = GetThreadPriority (GetCurrentThread ());
@@ -509,11 +540,12 @@ try_to_debug ()
     }
   FreeEnvironmentStringsW (rawenv);
 
-  console_printf ("*** starting debugger for pid %u, tid %u\n",
+  console_printf ("*** starting '%W' for pid %u, tid %u\r\n",
+		  command,
 		  cygwin_pid (GetCurrentProcessId ()), GetCurrentThreadId ());
   BOOL dbg;
   dbg = CreateProcessW (NULL,
-			debugger_command,
+			command,
 			NULL,
 			NULL,
 			FALSE,
@@ -527,11 +559,15 @@ try_to_debug ()
      we can't wait here for the error_start process to exit, as if it's a
      debugger, it might want to continue this thread.  So we busy wait until a
      debugger attaches, which stops this process, after which it can decide if
-     we continue or not. */
+     we continue or not.
+
+     Note that this is still racy: if the error_start process does it's work too
+     fast, we don't notice that it attached and get stuck here.
+  */
 
   *dbg_end = L'\0';
   if (!dbg)
-    system_printf ("Failed to start debugger, %E");
+    system_printf ("Failed to start, %E");
   else
     {
       SetThreadPriority (GetCurrentThread (), THREAD_PRIORITY_IDLE);
@@ -540,13 +576,29 @@ try_to_debug ()
       Sleep (2000);
     }
 
-  console_printf ("*** continuing pid %u from debugger call (%d)\n",
-		  cygwin_pid (GetCurrentProcessId ()), dbg);
+  console_printf ("*** continuing pid %u\r\n",
+		  cygwin_pid (GetCurrentProcessId ()));
 
   SetThreadPriority (GetCurrentThread (), prio);
   return dbg;
 }
 
+extern "C" int
+try_to_debug ()
+{
+  /* If already being debugged, break into the debugger (Note that this function
+     can be called from places other than an exception) */
+  if (being_debugged ())
+    {
+      extern void break_here ();
+      break_here ();
+      return 0;
+    }
+
+  /* Otherwise, invoke the JIT debugger, if set */
+  return exec_prepared_command (debugger_command);
+}
+
 /* myfault exception handler. */
 EXCEPTION_DISPOSITION
 exception::myfault (EXCEPTION_RECORD *e, exception_list *frame, CONTEXT *in,
@@ -1264,7 +1316,6 @@ signal_exit (int sig, siginfo_t *si, void *)
   debug_printf ("exiting due to signal %d", sig);
   exit_state = ES_SIGNAL_EXIT;
 
-  if (cygheap->rlim_core > 0UL)
     switch (sig)
       {
       case SIGABRT:
@@ -1277,9 +1328,24 @@ signal_exit (int sig, siginfo_t *si, void *)
       case SIGTRAP:
       case SIGXCPU:
       case SIGXFSZ:
-	sig |= 0x80;		/* Flag that we've "dumped core" */
 	if (try_to_debug ())
 	  break;
+
+	if (cygheap->rlim_core == 0Ul)
+	  break;
+
+	sig |= 0x80; /* Set flag in exit status to show that we've "dumped core" */
+
+	/* If core dump size is >1MB, try to invoke dumper to write a
+	   .core file */
+	if (cygheap->rlim_core > 1024*1024)
+	  {
+	    if (exec_prepared_command (dumper_command))
+	      break;
+	    /* If that failed, fall-through to... */
+	  }
+
+	/* Otherwise write a .stackdump */
 	if (si->si_code != SI_USER && si->si_cyg)
 	  {
 	    cygwin_exception *exc = (cygwin_exception *) si->si_cyg;
diff --git a/winsup/cygwin/local_includes/winsup.h b/winsup/cygwin/local_includes/winsup.h
index bf0a0bcc3..76957618b 100644
--- a/winsup/cygwin/local_includes/winsup.h
+++ b/winsup/cygwin/local_includes/winsup.h
@@ -179,6 +179,7 @@ void close_all_files (bool = false);
 
 /* debug_on_trap support. see exceptions.cc:try_to_debug() */
 extern "C" void error_start_init (const char*);
+extern "C" void dumper_init (void);
 extern "C" int try_to_debug ();
 
 void ld_preload ();
diff --git a/winsup/cygwin/release/3.5.0 b/winsup/cygwin/release/3.5.0
index d0a6c2fc8..ed27ee32a 100644
--- a/winsup/cygwin/release/3.5.0
+++ b/winsup/cygwin/release/3.5.0
@@ -58,3 +58,7 @@ What changed:
 
 - Enable automatic sparsifying of files on SSDs, independent of the
   "sparse" mount mode.
+
+- When RLIMIT_CORE is more than 1MB, a core dump file which can be loaded by gdb
+  is now written on a fatal error. Otherwise, if it's greater than zero, a text
+  format .stackdump file is written, as previously.
diff --git a/winsup/doc/cygwinenv.xml b/winsup/doc/cygwinenv.xml
index 5e17404a7..d97f2b77d 100644
--- a/winsup/doc/cygwinenv.xml
+++ b/winsup/doc/cygwinenv.xml
@@ -23,18 +23,29 @@ Defaults to off.</para>
 
 <listitem>
 <para>
-<envar>error_start:Win32filepath</envar> - if set, runs 
-<filename>Win32filepath</filename> when cygwin encounters a fatal error,
-which is useful for debugging.  <filename>Win32filepath</filename> is
-usually set to the path to <command>gdb</command> or
-<command>dumper</command>, for example
-<filename>C:\cygwin\bin\gdb.exe</filename>.
-There is no default set.
+<envar>error_start:Win32filepath</envar> - if set, runs
+<filename>Win32filepath</filename> when cygwin encounters a fatal error, which
+can be useful for debugging. Defaults to not set.
+</para>
+<para>
+<filename>Win32filepath</filename> is typically set to <command>gdb</command> or
+<command>dumper</command>.  If giving a path in
+<filename>Win32filepath</filename>, note that it is a Windows-style path and not
+a Cygwin path.
 </para>
 <para>
 The filename of the executing program and it's Windows process id are appended
 to the command as arguments.
 </para>
+<para>
+  Note: This takes priority over writing core dump or .stackdump files, if
+  enabled by <function>setrlimit(RLIMIT_CORE)</function> (e.g. via
+  <command>ulimit -c</command>).
+</para>
+<para>
+  Note: This has no effect if a debugger is already attached when the fatal
+  error occurs.
+</para>
 </listitem>
 
 <listitem>
diff --git a/winsup/doc/new-features.xml b/winsup/doc/new-features.xml
index bd856525e..b6daadc2b 100644
--- a/winsup/doc/new-features.xml
+++ b/winsup/doc/new-features.xml
@@ -93,6 +93,12 @@ Enable automatic sparsifying of files on SSDs, independent of the
 "sparse" mount mode.
 </para></listitem>
 
+<listitem><para>
+When RLIMIT_CORE is more than 1MB, a core dump file which can be loaded by gdb
+is now written on a fatal error. Otherwise, if it's greater than zero, a text
+format .stackdump file is written, as previously.
+</para></listitem>
+
 </itemizedlist>
 
 </sect2>
diff --git a/winsup/doc/utils.xml b/winsup/doc/utils.xml
index 9210c94e2..692dae38f 100644
--- a/winsup/doc/utils.xml
+++ b/winsup/doc/utils.xml
@@ -721,17 +721,17 @@ explorer $XPATH &
     <refsect1 id="dumper-desc">
       <title>Description</title>
     <para>The <command>dumper</command> utility can be used to create a core
-      dump of running Windows process. This core dump can be later loaded to
-      <command>gdb</command> and analyzed. One common way to use
-      <command>dumper</command> is to plug it into cygwin's Just-In-Time
-      debugging facility by adding
-      <screen>
-error_start=x:\path\to\dumper.exe
-</screen> to the
-      <emphasis>CYGWIN</emphasis> environment variable. Please note that
-      <literal>x:\path\to\dumper.exe</literal> is Windows-style and not cygwin
-      path. If <literal>error_start</literal> is set this way, then dumper will
-      be started whenever some program encounters a fatal error. </para>
+      dump of running Windows process. This core dump can be later loaded into
+      <command>gdb</command> and analyzed.
+    </para>
+
+    <para>
+      If the core file size limit is set to unlimited (e.g. <command>ulimit -c
+      unlimited</command>) and an <literal>error_start</literal> executable
+      hasn't been configured in the <literal>CYGWIN</literal> environment
+      variable, Cygwin will automatically run <command>dumper</command> when a
+      fatal error occurs.
+    </para>
 
     <para> <command>dumper</command> can be also be started from the command
       line to create a core dump of any running process.</para>
@@ -742,14 +742,25 @@ error_start=x:\path\to\dumper.exe
     terminated.</para>
 
     <para> To save space in the core dump, <command>dumper</command> doesn't
-      write those portions of the target process's memory space that are loaded
+      write those portions of the target process's memory space that were loaded
       from executable and dll files and are unchanged (e.g. program code).
       Instead, <command>dumper</command> saves paths to the files which
       contain that data. When a core dump is loaded into gdb, it uses these
       paths to load the appropriate files. That means that if you create a core
       dump on one machine and try to debug it on another, you'll need to place
       identical copies of the executable and dlls in the same directories as on
-      the machine where the core dump was created. </para>
+      the machine where the core dump was created.
+    </para>
+  </refsect1>
+
+  <refsect1 id="dumper-notes">
+    <title>Notes</title>
+    <para>
+      A Cygwin "core dump file" is an ELF file containing the mutable parts of
+      the process memory and special note sections which capture the process,
+      thread and loaded module context needed to recreate the process image in a
+      debugger.
+    </para>
     </refsect1>
   </refentry>
 
@@ -1497,8 +1508,10 @@ bash$ locale noexpr
 
   <para>
     <command>minidumper</command> can be used with cygwin's Just-In-Time
-    debugging facility in exactly the same way as <command>dumper</command>
-    (See <xref linkend="dumper"></xref>).
+    debugging facility by adding <code>error_start=minidumper</code> to the
+    <literal>CYGWIN</literal> environment variable. If <literal>CYGWIN</literal>
+    is set this way, then <command>minidumper</command> will be started whenever
+    a program encounters a fatal exception.
   </para>
 
   <para>
-- 
2.43.0

[PATCH 2/5] Cygwin: Disable writing core dumps by default.

[Jon Turney]

Change the default core limit from unlimited to 0 (disabled)
---
 winsup/cygwin/mm/cygheap.cc | 2 +-
 winsup/cygwin/release/3.5.0 | 3 +++
 winsup/doc/new-features.xml | 6 ++++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/mm/cygheap.cc b/winsup/cygwin/mm/cygheap.cc
index a20ee5972..3dc0c011f 100644
--- a/winsup/cygwin/mm/cygheap.cc
+++ b/winsup/cygwin/mm/cygheap.cc
@@ -294,7 +294,7 @@ cygheap_init ()
       cygheap->locale.mbtowc = __utf8_mbtowc;
       /* Set umask to a sane default. */
       cygheap->umask = 022;
-      cygheap->rlim_core = RLIM_INFINITY;
+      cygheap->rlim_core = 0;
     }
   if (!cygheap->fdtab)
     cygheap->fdtab.init ();
diff --git a/winsup/cygwin/release/3.5.0 b/winsup/cygwin/release/3.5.0
index ed27ee32a..3e6d60376 100644
--- a/winsup/cygwin/release/3.5.0
+++ b/winsup/cygwin/release/3.5.0
@@ -62,3 +62,6 @@ What changed:
 - When RLIMIT_CORE is more than 1MB, a core dump file which can be loaded by gdb
   is now written on a fatal error. Otherwise, if it's greater than zero, a text
   format .stackdump file is written, as previously.
+
+- The default RLIMIT_CORE is now 0, disabling the generation of core dump or
+  stackdump files.
diff --git a/winsup/doc/new-features.xml b/winsup/doc/new-features.xml
index b6daadc2b..a22b78a60 100644
--- a/winsup/doc/new-features.xml
+++ b/winsup/doc/new-features.xml
@@ -99,6 +99,12 @@ is now written on a fatal error. Otherwise, if it's greater than zero, a text
 format .stackdump file is written, as previously.
 </para></listitem>
 
+<listitem><para>
+The default RLIMIT_CORE is now 0, disabling the generation of core dump or
+stackdump files. Use e.g. <code>ulimit -c unlimited</code> or <code>ulimit -c
+1024</code> to enable them again.
+</para></listitem>
+
 </itemizedlist>
 
 </sect2>
-- 
2.43.0

[PATCH 3/5] Cygwin: Define and use __WCOREFLAG

[Jon Turney]

Also fix a typo in description of exit status
---
 winsup/cygwin/exceptions.cc         | 2 +-
 winsup/cygwin/include/cygwin/wait.h | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 6bd34392a..05ffdc27e 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1334,7 +1334,7 @@ signal_exit (int sig, siginfo_t *si, void *)
 	if (cygheap->rlim_core == 0Ul)
 	  break;
 
-	sig |= 0x80; /* Set flag in exit status to show that we've "dumped core" */
+	sig |= __WCOREFLAG; /* Set flag in exit status to show that we've "dumped core" */
 
 	/* If core dump size is >1MB, try to invoke dumper to write a
 	   .core file */
diff --git a/winsup/cygwin/include/cygwin/wait.h b/winsup/cygwin/include/cygwin/wait.h
index 7e40c8d6c..0d42e8920 100644
--- a/winsup/cygwin/include/cygwin/wait.h
+++ b/winsup/cygwin/include/cygwin/wait.h
@@ -16,12 +16,13 @@ details. */
 #define WUNTRACED 2
 #define WCONTINUED 8
 #define __W_CONTINUED	0xffff
+#define __WCOREFLAG 0x80
 
 /* A status is 16 bits, and looks like:
       <1 byte info> <1 byte code>
 
       <code> == 0, child has exited, info is the exit value
-      <code> == 1..7e, child has exited, info is the signal number.
+      <code> == 1..7e, child has exited, code is the signal number.
       <code> == 7f, child has stopped, info was the signal number.
       <code> == 80, there was a core dump.
 */
@@ -34,6 +35,6 @@ details. */
 #define WEXITSTATUS(_w)		(((_w) >> 8) & 0xff)
 #define WTERMSIG(_w)		((_w) & 0x7f)
 #define WSTOPSIG		WEXITSTATUS
-#define WCOREDUMP(_w)		(WIFSIGNALED(_w) && ((_w) & 0x80))
+#define WCOREDUMP(_w)		(WIFSIGNALED(_w) && ((_w) & __WCOREFLAG))
 
 #endif /* _CYGWIN_WAIT_H */
-- 
2.43.0

[PATCH 4/5] Cygwin: Treat api_fatal() similarly to a core-dumping signal

[Jon Turney]

Provide the same debugging opportunities for api_fatal() as we do for a
core-dumping signal:

1) Break into any attached debugger
2) Start JIT debugger (if configured) (keeping these under DEBUGGING doesn't seem helpful)
3) Write a coredump (if rlim_core > 1MB)
4) Write a stackdump (if that failed, or 0 < rlim_core <= 1MB)
---
 winsup/cygwin/dcrt0.cc                |  6 +-----
 winsup/cygwin/exceptions.cc           | 18 ++++++++++++++++++
 winsup/cygwin/local_includes/winsup.h |  1 +
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
index 130d652aa..17c9be731 100644
--- a/winsup/cygwin/dcrt0.cc
+++ b/winsup/cygwin/dcrt0.cc
@@ -1250,11 +1250,7 @@ vapi_fatal (const char *fmt, va_list ap)
   __small_vsprintf (buf + n, fmt, ap);
   va_end (ap);
   strace.prntf (_STRACE_SYSTEM, NULL, "%s", buf);
-
-#ifdef DEBUGGING
-  try_to_debug ();
-#endif
-  cygwin_stackdump ();
+  api_fatal_debug();
   myself.exit (__api_fatal_exit_val);
 }
 
diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 05ffdc27e..32c64b3d7 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1396,6 +1396,24 @@ signal_exit (int sig, siginfo_t *si, void *)
 }
 } /* extern "C" */
 
+/* As above, but before exiting due to api_fatal */
+extern "C"
+void
+api_fatal_debug ()
+{
+  if (try_to_debug ())
+    return;
+
+  if (cygheap->rlim_core == 0Ul)
+    return;
+
+  if (cygheap->rlim_core > 1024*1024)
+    if (exec_prepared_command (dumper_command))
+      return;
+
+  cygwin_stackdump();
+}
+
 /* Attempt to carefully handle SIGCONT when we are stopped. */
 void
 _cygtls::handle_SIGCONT ()
diff --git a/winsup/cygwin/local_includes/winsup.h b/winsup/cygwin/local_includes/winsup.h
index 76957618b..38313962d 100644
--- a/winsup/cygwin/local_includes/winsup.h
+++ b/winsup/cygwin/local_includes/winsup.h
@@ -181,6 +181,7 @@ void close_all_files (bool = false);
 extern "C" void error_start_init (const char*);
 extern "C" void dumper_init (void);
 extern "C" int try_to_debug ();
+extern "C" void api_fatal_debug ();
 
 void ld_preload ();
 void fixup_hooks_after_fork ();
-- 
2.43.0

[PATCH 5/5] Cygwin: Update documentation for cygwin_stackdump

[Jon Turney]

---
 winsup/doc/misc-funcs.xml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/winsup/doc/misc-funcs.xml b/winsup/doc/misc-funcs.xml
index 7463942e6..55c5cac94 100644
--- a/winsup/doc/misc-funcs.xml
+++ b/winsup/doc/misc-funcs.xml
@@ -106,6 +106,10 @@ enum.  The second is an optional pointer.</para>
   <refsect1 id="func-cygwin-stackdump-desc">
     <title>Description</title>
 <para> Outputs a stackdump to stderr from the called location.
+</para>
+<para> Note: This function is has an effect the first time it is called by a process.
+</para>
+<para> Note: This function is deprecated.
 </para>
   </refsect1>
 </refentry>
-- 
2.43.0

Re: [PATCH 0/5] Coredump under 'ulimit -c' control (v2)

[Corinna Vinschen]

On Jan 12 14:09, Jon Turney wrote:
> Write a coredump under 'ulimit -c' control and related changes.
> 
> The idea here is to make debugging using a coredump work as usual on a unix,
> e.g.:
> 
> $ ulimit -c unlimited
> 
> $ ./segv-program
> *** starting '"C:\cygwin64\bin\dumper.exe" "C:\cygwin64\work\segv-program.exe" 16156' for pid 1398, tid 7136
> 
> $ gdb segv-program.exe segv-program.exe.core
> [...]
> 
> Jon Turney (5):
>   Cygwin: Make 'ulimit -c' control writing a coredump
>   Cygwin: Disable writing core dumps by default.
>   Cygwin: Define and use __WCOREFLAG
>   Cygwin: Treat api_fatal() similarly to a core-dumping signal
>   Cygwin: Update documentation for cygwin_stackdump
> 
>  winsup/cygwin/dcrt0.cc                |   6 +-
>  winsup/cygwin/environ.cc              |   1 +
>  winsup/cygwin/exceptions.cc           | 122 ++++++++++++++++++++++----
>  winsup/cygwin/include/cygwin/wait.h   |   5 +-
>  winsup/cygwin/local_includes/winsup.h |   2 +
>  winsup/cygwin/mm/cygheap.cc           |   2 +-
>  winsup/cygwin/release/3.5.0           |   7 ++
>  winsup/doc/cygwinenv.xml              |  25 ++++--
>  winsup/doc/misc-funcs.xml             |   4 +
>  winsup/doc/new-features.xml           |  12 +++
>  winsup/doc/utils.xml                  |  43 +++++----
>  11 files changed, 180 insertions(+), 49 deletions(-)
> 
> -- 
> 2.43.0

This patchset looks good to me, except one typo in the last patch
of the series...

Thanks,
Corinna

Re: [PATCH 5/5] Cygwin: Update documentation for cygwin_stackdump

[Corinna Vinschen]

On Jan 12 14:09, Jon Turney wrote:
> ---
>  winsup/doc/misc-funcs.xml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/winsup/doc/misc-funcs.xml b/winsup/doc/misc-funcs.xml
> index 7463942e6..55c5cac94 100644
> --- a/winsup/doc/misc-funcs.xml
> +++ b/winsup/doc/misc-funcs.xml
> @@ -106,6 +106,10 @@ enum.  The second is an optional pointer.</para>
>    <refsect1 id="func-cygwin-stackdump-desc">
>      <title>Description</title>
>  <para> Outputs a stackdump to stderr from the called location.
> +</para>
> +<para> Note: This function is has an effect the first time it is called by a process.
                              ^^^^^^
This looks like a rephrasing attempt gone slightly wrong.

I'm also not quite sure what you're trying to say here. Maybe a bit more
detailed would help me and other readers?


Thanks,
Corinna

Re: [PATCH 5/5] Cygwin: Update documentation for cygwin_stackdump

[Jon Turney]

On 12/01/2024 18:44, Corinna Vinschen wrote:
> On Jan 12 14:09, Jon Turney wrote:
>> ---
>>   winsup/doc/misc-funcs.xml | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/winsup/doc/misc-funcs.xml b/winsup/doc/misc-funcs.xml
>> index 7463942e6..55c5cac94 100644
>> --- a/winsup/doc/misc-funcs.xml
>> +++ b/winsup/doc/misc-funcs.xml
>> @@ -106,6 +106,10 @@ enum.  The second is an optional pointer.</para>
>>     <refsect1 id="func-cygwin-stackdump-desc">
>>       <title>Description</title>
>>   <para> Outputs a stackdump to stderr from the called location.
>> +</para>
>> +<para> Note: This function is has an effect the first time it is called by a process.
>                                ^^^^^^
> This looks like a rephrasing attempt gone slightly wrong.
> 
> I'm also not quite sure what you're trying to say here. Maybe a bit more
> detailed would help me and other readers?

Sorry about that. It seems like I changed my mind about what I was 
writing halfway through the sentence. I guess I meant:

"This function only has an effect the first time it is called by a process."

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney]

On 12/01/2024 14:09, Jon Turney wrote:
> +
> +  PWCHAR cp = dumper_command;
> +  cp = wcpcpy (cp, L"\"");
> +  cp = wcpcpy (cp, dll_dir);
> +  cp = wcpcpy (cp, L"\\dumper.exe");
> +  cp = wcpcpy (cp, L"\" ");
> +  cp = wcpcpy (cp, L"\"");
> +  cp = wcpcpy (cp, global_progname);

I wonder if this should be program_invocation_short_name, so that the 
coredump is created in the cwd, rather than next to the executable.


But then, there's then no way to get similar behaviour if you decide you 
want to use minidumps instead (by setting 
CYGWIN="error_start=minidumper"), as the first argument to 
dumper/minidump is the full path to the program (to match the 'prog 
procID' style of invoking gdb), but they only use it to add an 
.core/.dmp extension to name the file to write.

I guess that could by fixed by adding an option to the dumpers to strip 
paths, or more control about how the JIT command is formatted.

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Corinna Vinschen]

On Jan 13 14:20, Jon Turney wrote:
> On 12/01/2024 14:09, Jon Turney wrote:
> > +
> > +  PWCHAR cp = dumper_command;
> > +  cp = wcpcpy (cp, L"\"");
> > +  cp = wcpcpy (cp, dll_dir);
> > +  cp = wcpcpy (cp, L"\\dumper.exe");
> > +  cp = wcpcpy (cp, L"\" ");
> > +  cp = wcpcpy (cp, L"\"");
> > +  cp = wcpcpy (cp, global_progname);
> 
> I wonder if this should be program_invocation_short_name, so that the
> coredump is created in the cwd, rather than next to the executable.

program_invocation_short_name would be nice, but does it really matter?

Because...

> But then, there's then no way to get similar behaviour if you decide you
> want to use minidumps instead (by setting CYGWIN="error_start=minidumper"),
> as the first argument to dumper/minidump is the full path to the program (to
> match the 'prog procID' style of invoking gdb), but they only use it to add
> an .core/.dmp extension to name the file to write.
> 
> I guess that could by fixed by adding an option to the dumpers to strip
> paths, or more control about how the JIT command is formatted.

dumper/minidumper are both called with the current working directory set
to the ... current working directory, right?  With the full pathname as
input, and the CWD already set the same as the dumped application, they
can easily generate any target path for the corefile they like.

Given the actual path of the corefile can be generated by the dumpers,
the question is how to specify where to store the corefile. For instance

- no option: CWD
- some option -c/--coredir for anywhere else

Under Linux versions using systemd, corefiles are by default not stored
in the CWD anymore, but to /var/lib/systemd/coredump, so there is a
use case for arbitrary corefile paths.


Corinna

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney]

On 15/01/2024 09:46, Corinna Vinschen wrote:
> On Jan 13 14:20, Jon Turney wrote:
>> On 12/01/2024 14:09, Jon Turney wrote:
>>> +
>>> +  PWCHAR cp = dumper_command;
>>> +  cp = wcpcpy (cp, L"\"");
>>> +  cp = wcpcpy (cp, dll_dir);
>>> +  cp = wcpcpy (cp, L"\\dumper.exe");
>>> +  cp = wcpcpy (cp, L"\" ");
>>> +  cp = wcpcpy (cp, L"\"");
>>> +  cp = wcpcpy (cp, global_progname);
>>
>> I wonder if this should be program_invocation_short_name, so that the
>> coredump is created in the cwd, rather than next to the executable.
> 
> program_invocation_short_name would be nice, but does it really matter?
> 
> Because...
> 
>> But then, there's then no way to get similar behaviour if you decide you
>> want to use minidumps instead (by setting CYGWIN="error_start=minidumper"),
>> as the first argument to dumper/minidump is the full path to the program (to
>> match the 'prog procID' style of invoking gdb), but they only use it to add
>> an .core/.dmp extension to name the file to write.
>>
>> I guess that could by fixed by adding an option to the dumpers to strip
>> paths, or more control about how the JIT command is formatted.
> 
> dumper/minidumper are both called with the current working directory set
> to the ... current working directory, right?  With the full pathname as
> input, and the CWD already set the same as the dumped application, they
> can easily generate any target path for the corefile they like.
> 
> Given the actual path of the corefile can be generated by the dumpers,
> the question is how to specify where to store the corefile. For instance
> 
> - no option: CWD
> - some option -c/--coredir for anywhere else
> 
> Under Linux versions using systemd, corefiles are by default not stored
> in the CWD anymore, but to /var/lib/systemd/coredump, so there is a
> use case for arbitrary corefile paths.

Yeah, I guess an option to the dumper to control where the file is 
written is probably the best way to address this, which is something 
which can perhaps be added later...

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Corinna Vinschen]

On Jan 15 13:27, Jon Turney wrote:
> On 15/01/2024 09:46, Corinna Vinschen wrote:
> > On Jan 13 14:20, Jon Turney wrote:
> > > On 12/01/2024 14:09, Jon Turney wrote:
> > > > +
> > > > +  PWCHAR cp = dumper_command;
> > > > +  cp = wcpcpy (cp, L"\"");
> > > > +  cp = wcpcpy (cp, dll_dir);
> > > > +  cp = wcpcpy (cp, L"\\dumper.exe");
> > > > +  cp = wcpcpy (cp, L"\" ");
> > > > +  cp = wcpcpy (cp, L"\"");
> > > > +  cp = wcpcpy (cp, global_progname);
> > > 
> > > I wonder if this should be program_invocation_short_name, so that the
> > > coredump is created in the cwd, rather than next to the executable.
> > 
> > program_invocation_short_name would be nice, but does it really matter?
> > 
> > Because...
> > 
> > > But then, there's then no way to get similar behaviour if you decide you
> > > want to use minidumps instead (by setting CYGWIN="error_start=minidumper"),
> > > as the first argument to dumper/minidump is the full path to the program (to
> > > match the 'prog procID' style of invoking gdb), but they only use it to add
> > > an .core/.dmp extension to name the file to write.
> > > 
> > > I guess that could by fixed by adding an option to the dumpers to strip
> > > paths, or more control about how the JIT command is formatted.
> > 
> > dumper/minidumper are both called with the current working directory set
> > to the ... current working directory, right?  With the full pathname as
> > input, and the CWD already set the same as the dumped application, they
> > can easily generate any target path for the corefile they like.
> > 
> > Given the actual path of the corefile can be generated by the dumpers,
> > the question is how to specify where to store the corefile. For instance
> > 
> > - no option: CWD
> > - some option -c/--coredir for anywhere else
> > 
> > Under Linux versions using systemd, corefiles are by default not stored
> > in the CWD anymore, but to /var/lib/systemd/coredump, so there is a
> > use case for arbitrary corefile paths.
> 
> Yeah, I guess an option to the dumper to control where the file is written
> is probably the best way to address this, which is something which can
> perhaps be added later...

Yeah. In that case we should write the coredump to CWD for the time
being and improve that for 3.6, ok?


Corinna

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney]

On 15/01/2024 14:28, Corinna Vinschen wrote:
> On Jan 15 13:27, Jon Turney wrote:
>> On 15/01/2024 09:46, Corinna Vinschen wrote:
>>> On Jan 13 14:20, Jon Turney wrote:
>>>> On 12/01/2024 14:09, Jon Turney wrote:
>>>>> +
>>>>> +  PWCHAR cp = dumper_command;
>>>>> +  cp = wcpcpy (cp, L"\"");
>>>>> +  cp = wcpcpy (cp, dll_dir);
>>>>> +  cp = wcpcpy (cp, L"\\dumper.exe");
>>>>> +  cp = wcpcpy (cp, L"\" ");
>>>>> +  cp = wcpcpy (cp, L"\"");
>>>>> +  cp = wcpcpy (cp, global_progname);
>>>>
>>>> I wonder if this should be program_invocation_short_name, so that the
>>>> coredump is created in the cwd, rather than next to the executable.

So, when I actually look further into this, what I wrote is utter 
nonsense. dumper/minidumper includes the following:

>       ssize_t len = cygwin_conv_path (CCP_POSIX_TO_WIN_A | CCP_RELATIVE,
>                                       *(argv + optind), NULL, 0);
>       char *win32_name = (char *) alloca (len);
>       cygwin_conv_path (CCP_POSIX_TO_WIN_A | CCP_RELATIVE,  *(argv + optind),
>                         win32_name, len);
>       if ((p = strrchr (win32_name, '\\')))
>         p++;
>       else
>         p = win32_name;

My eyes moving over this lightly, my brain assumes it just converts from 
a Win32 path to a POSIX path, but actually it does:

1) convert from POSIX path to Windows path (assuming it's a no-op if the 
path is already in a Windows form
2) (now having a consistent path delimiter) use the part after the last 
delimiter (if any), as the basename.

So: no problem here, after all. coredump file is already created in the cwd.

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Corinna Vinschen]

On Jan 16 13:52, Jon Turney wrote:
> On 15/01/2024 14:28, Corinna Vinschen wrote:
> > On Jan 15 13:27, Jon Turney wrote:
> > > On 15/01/2024 09:46, Corinna Vinschen wrote:
> > > > On Jan 13 14:20, Jon Turney wrote:
> > > > > On 12/01/2024 14:09, Jon Turney wrote:
> > > > > > +
> > > > > > +  PWCHAR cp = dumper_command;
> > > > > > +  cp = wcpcpy (cp, L"\"");
> > > > > > +  cp = wcpcpy (cp, dll_dir);
> > > > > > +  cp = wcpcpy (cp, L"\\dumper.exe");
> > > > > > +  cp = wcpcpy (cp, L"\" ");
> > > > > > +  cp = wcpcpy (cp, L"\"");
> > > > > > +  cp = wcpcpy (cp, global_progname);
> > > > > 
> > > > > I wonder if this should be program_invocation_short_name, so that the
> > > > > coredump is created in the cwd, rather than next to the executable.
> 
> So, when I actually look further into this, what I wrote is utter nonsense.
> dumper/minidumper includes the following:
> 
> >       ssize_t len = cygwin_conv_path (CCP_POSIX_TO_WIN_A | CCP_RELATIVE,
> >                                       *(argv + optind), NULL, 0);
> >       char *win32_name = (char *) alloca (len);
> >       cygwin_conv_path (CCP_POSIX_TO_WIN_A | CCP_RELATIVE,  *(argv + optind),
> >                         win32_name, len);
> >       if ((p = strrchr (win32_name, '\\')))
> >         p++;
> >       else
> >         p = win32_name;
> 
> My eyes moving over this lightly, my brain assumes it just converts from a
> Win32 path to a POSIX path, but actually it does:
> 
> 1) convert from POSIX path to Windows path (assuming it's a no-op if the
> path is already in a Windows form
> 2) (now having a consistent path delimiter) use the part after the last
> delimiter (if any), as the basename.
> 
> So: no problem here, after all. coredump file is already created in the cwd.

:+1:

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney]

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

On 12/01/2024 14:09, Jon Turney wrote:
> Pre-format a command to be executed on a fatal error to run 'dumper'
> (using an absolute path).
> 
> Factor out executing a pre-formatted command, so we can use that for
> invoking the JIT debugger in try_to_debug() (if error_start is present
> in the CYGWIN env var) and to invoke dumper when a fatal error occurs.
> 

So, there is a small problem with this change: because dumper itself 
terminates the dumped process, it doesn't go on to exit with the 
signal+128 exit status.

(In fact, it seems to exit with status 0 when terminated by an attached 
debugger terminating, which isn't great)

That's relatively easy to fix: just use the '-n' option to dumper so it 
detaches before exiting, to prevent that terminating the dumped process, 
but then we run into the difficulties of reliably detecting that dumper 
has attached and done it's work, so it's safe for us to exit.

Attached patch does that, and documents the expectations on the 
error_start command a bit more clearly.

Even then this is clearly not totally bullet-proof. Maybe the right 
thing to do is add a suitable timeout here, so even if we fail to notice 
the DebugActiveProcess() (or there's a custom JIT debugger which just 
writes the fact a process crashed to a logfile or something), we'll exit 
eventually?

[-- Attachment #2: 0001-Cygwin-Don-t-terminate-via-dumper.patch --]
[-- Type: text/plain, Size: 2715 bytes --]

From 85120a1697294cd93ff68f6b1840145251ce4185 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Tue, 16 Jan 2024 16:12:51 +0000
Subject: [PATCH] Cygwin: Don't terminate via dumper

A process which is exiting due to a core dumping signal doesn't
propagate the correct exist status after dumping core, because 'dumper'
itself forcibly terminates the process.

Use 'dumper -n' to avoid killing the dumped process, so we continue to
the end of signal_exit() , to exit with the 128+signal exit status.

Busy-wait in exec_prepared_command() in an attempt to reliably notice
the dumper attaching, so we don't get stuck there.

Also: document these important facts for custom uses of error_start.
---
 winsup/cygwin/exceptions.cc | 7 +++----
 winsup/doc/cygwinenv.xml    | 6 ++++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 8b1c5493e..0e1a804ca 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -149,7 +149,7 @@ dumper_init (void)
 
   /* Calculate the length of the command, allowing for an appended DWORD PID and
      terminating null */
-  int cmd_len = 1 + wcslen(dll_dir) + 11 + 2 + 1 + wcslen(global_progname) + 1 + 10 + 1;
+  int cmd_len = 1 + wcslen(dll_dir) + 11 + 5 + 1 + wcslen(global_progname) + 1 + 10 + 1;
   if (cmd_len > 32767)
     {
       /* If this comes to more than the 32,767 characters CreateProcess() can
@@ -163,7 +163,7 @@ dumper_init (void)
   cp = wcpcpy (cp, L"\"");
   cp = wcpcpy (cp, dll_dir);
   cp = wcpcpy (cp, L"\\dumper.exe");
-  cp = wcpcpy (cp, L"\" ");
+  cp = wcpcpy (cp, L"\" -n ");
   cp = wcpcpy (cp, L"\"");
   cp = wcpcpy (cp, global_progname);
   wcscat (cp, L"\"");
@@ -570,9 +570,8 @@ int exec_prepared_command (PWCHAR command)
     system_printf ("Failed to start, %E");
   else
     {
-      SetThreadPriority (GetCurrentThread (), THREAD_PRIORITY_IDLE);
       while (!being_debugged ())
-	Sleep (1);
+	Sleep (0);
       Sleep (2000);
     }
 
diff --git a/winsup/doc/cygwinenv.xml b/winsup/doc/cygwinenv.xml
index d97f2b77d..05672c404 100644
--- a/winsup/doc/cygwinenv.xml
+++ b/winsup/doc/cygwinenv.xml
@@ -46,6 +46,12 @@ to the command as arguments.
   Note: This has no effect if a debugger is already attached when the fatal
   error occurs.
 </para>
+<para>
+  Note: The command invoked must either (i) attach to the errored process with
+  <function>DebugActiveProcess()</function>, or (ii) forcibly terminate the
+  errored process (with <function>TerminateProcess()</function> or similar), as
+  otherwise the errored process will wait forever for a debugger to attach.
+</para>
 </listitem>
 
 <listitem>
-- 
2.43.0

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Corinna Vinschen]

On Jan 23 14:20, Jon Turney wrote:
> On 12/01/2024 14:09, Jon Turney wrote:
> > Pre-format a command to be executed on a fatal error to run 'dumper'
> > (using an absolute path).
> > 
> > Factor out executing a pre-formatted command, so we can use that for
> > invoking the JIT debugger in try_to_debug() (if error_start is present
> > in the CYGWIN env var) and to invoke dumper when a fatal error occurs.
> > 
> 
> So, there is a small problem with this change: because dumper itself
> terminates the dumped process, it doesn't go on to exit with the signal+128
> exit status.
> 
> (In fact, it seems to exit with status 0 when terminated by an attached
> debugger terminating, which isn't great)
> 
> That's relatively easy to fix: just use the '-n' option to dumper so it
> detaches before exiting, to prevent that terminating the dumped process, but
> then we run into the difficulties of reliably detecting that dumper has
> attached and done it's work, so it's safe for us to exit.
> 
> Attached patch does that, and documents the expectations on the error_start
> command a bit more clearly.

Please push.

> Even then this is clearly not totally bullet-proof. Maybe the right thing to
> do is add a suitable timeout here, so even if we fail to notice the
> DebugActiveProcess() (or there's a custom JIT debugger which just writes the
> fact a process crashed to a logfile or something), we'll exit eventually?

Timeouts are just that tiny little bit more bullet-proof, they still
aren't totally bullet-proof.

What timeout were you thinking of?  milliseconds?


Corinna

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney]

On 23/01/2024 14:29, Corinna Vinschen wrote:
> On Jan 23 14:20, Jon Turney wrote:
> 
>> Even then this is clearly not totally bullet-proof. Maybe the right thing to
>> do is add a suitable timeout here, so even if we fail to notice the
>> DebugActiveProcess() (or there's a custom JIT debugger which just writes the
>> fact a process crashed to a logfile or something), we'll exit eventually?
> 
> Timeouts are just that tiny little bit more bullet-proof, they still
> aren't totally bullet-proof.
> 
> What timeout were you thinking of?  milliseconds?

Oh no, tens of seconds or something, just as a fail-safe.

To be clear, I'm suggesting something like this:

-      while (!being_debugged ())
+      while (!being_debugged () || GetTickCount64() > timeout)
         Sleep (0);


As the comment above identifies, the concern is that if the executed 
command runs too quickly, we don't notice and get stuck there.

This isn't a concern when invoking gdb, as if the debugee is allowed to 
continue, being_debugged will return TRUE and we'll exit the loop.

But if we're invoking dumper, if it attaches and detaches quickly 
enough, we never notice and just get stuck.

(Ofc, all this is working around the fact that Win32 API doesn't have a
WaitForDebuggerPresent(timeout) function)

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Corinna Vinschen]

On Jan 24 13:28, Jon Turney wrote:
> On 23/01/2024 14:29, Corinna Vinschen wrote:
> > On Jan 23 14:20, Jon Turney wrote:
> > 
> > > Even then this is clearly not totally bullet-proof. Maybe the right thing to
> > > do is add a suitable timeout here, so even if we fail to notice the
> > > DebugActiveProcess() (or there's a custom JIT debugger which just writes the
> > > fact a process crashed to a logfile or something), we'll exit eventually?
> > 
> > Timeouts are just that tiny little bit more bullet-proof, they still
> > aren't totally bullet-proof.
> > 
> > What timeout were you thinking of?  milliseconds?
> 
> Oh no, tens of seconds or something, just as a fail-safe.

Uh, sounds a lot.  10 secs?  Not longer, I think.

If you want to do that for 3.5, please do it this week.  You can
push the change without waiting for approval.

> (Ofc, all this is working around the fact that Win32 API doesn't have a
> WaitForDebuggerPresent(timeout) function)

Yeah, and there's no alternative way using the native API afaics :(


Corinna

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney]

On 24/01/2024 14:39, Corinna Vinschen wrote:
> On Jan 24 13:28, Jon Turney wrote:
>> On 23/01/2024 14:29, Corinna Vinschen wrote:
>>> On Jan 23 14:20, Jon Turney wrote:
>>>
>>>> Even then this is clearly not totally bullet-proof. Maybe the right thing to
>>>> do is add a suitable timeout here, so even if we fail to notice the
>>>> DebugActiveProcess() (or there's a custom JIT debugger which just writes the
>>>> fact a process crashed to a logfile or something), we'll exit eventually?
>>>
>>> Timeouts are just that tiny little bit more bullet-proof, they still
>>> aren't totally bullet-proof.
>>>
>>> What timeout were you thinking of?  milliseconds?
>>
>> Oh no, tens of seconds or something, just as a fail-safe.
> 
> Uh, sounds a lot.  10 secs?  Not longer, I think.
> 
> If you want to do that for 3.5, please do it this week.  You can
> push the change without waiting for approval.

Thanks.

I pushed a small change adding this timeout.

>> (Ofc, all this is working around the fact that Win32 API doesn't have a
>> WaitForDebuggerPresent(timeout) function)
> 
> Yeah, and there's no alternative way using the native API afaics :(

So this situation with a JIT debugger is even stranger than my 
emendations to the documentation say.

Because we're hitting try_to_debug() in exception::handle(), which has 
some code to replay the exception via ExceptionContinueExecution (which 
I guess the debugger will catch as a first-chance) (and goes into a mode 
where it ignores the next half-million exceptions... no idea what that's 
about!)

That's not the same as signal_exit() with a coredumping signal (haven't 
checked if those are all generated from exceptions, but seemly probable, 
so the try_to_debug() there maybe isn't doing anything?), where we're 
going to exit thereafter.

The practical upshot of this is if the JIT debugger doesn't terminate or 
fix the erroring process, we'll just replay the faulting instruction and 
invoke the JIT debugger again.

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Corinna Vinschen]

On Jan 25 14:50, Jon Turney wrote:
> On 24/01/2024 14:39, Corinna Vinschen wrote:
> > On Jan 24 13:28, Jon Turney wrote:
> > > On 23/01/2024 14:29, Corinna Vinschen wrote:
> > > > On Jan 23 14:20, Jon Turney wrote:
> > > > 
> > > > > Even then this is clearly not totally bullet-proof. Maybe the right thing to
> > > > > do is add a suitable timeout here, so even if we fail to notice the
> > > > > DebugActiveProcess() (or there's a custom JIT debugger which just writes the
> > > > > fact a process crashed to a logfile or something), we'll exit eventually?
> > > > 
> > > > Timeouts are just that tiny little bit more bullet-proof, they still
> > > > aren't totally bullet-proof.
> > > > 
> > > > What timeout were you thinking of?  milliseconds?
> > > 
> > > Oh no, tens of seconds or something, just as a fail-safe.
> > 
> > Uh, sounds a lot.  10 secs?  Not longer, I think.
> > 
> > If you want to do that for 3.5, please do it this week.  You can
> > push the change without waiting for approval.
> 
> Thanks.
> 
> I pushed a small change adding this timeout.
> 
> > > (Ofc, all this is working around the fact that Win32 API doesn't have a
> > > WaitForDebuggerPresent(timeout) function)
> > 
> > Yeah, and there's no alternative way using the native API afaics :(
> 
> So this situation with a JIT debugger is even stranger than my emendations
> to the documentation say.
> 
> Because we're hitting try_to_debug() in exception::handle(), which has some
> code to replay the exception via ExceptionContinueExecution (which I guess
> the debugger will catch as a first-chance) (and goes into a mode where it
> ignores the next half-million exceptions... no idea what that's about!)
> 
> That's not the same as signal_exit() with a coredumping signal (haven't
> checked if those are all generated from exceptions, but seemly probable, so
> the try_to_debug() there maybe isn't doing anything?), where we're going to
> exit thereafter.

try_to_debug() is only calling IsDebuggerPresent() as test, and that's
nothing but a flag in the PEB which is set by the OS after a debugger
attached to the process.  So the test is by definition extremely
flaky, if the debugger is connecting and disconnecting, as you
already pointed out.

I'm wondering if we can't define our own way to attach to a process,
allowing to "WaitForDebugger" as long as the debugger is a Cygwin
debugger.  If we define a matching function (along the lines of
prctl(2) on Linux), we could change our debuggers, core dumpers
and stracers to call this attach function.

The idea would be to define some shared mutex object, the inferior
waits for and the debugger releases after having attached.

Is there really any need to support non-Cygwin debuggers?

> The practical upshot of this is if the JIT debugger doesn't terminate or fix
> the erroring process, we'll just replay the faulting instruction and invoke
> the JIT debugger again.

Hmm, ok.  This signal stuff *is* complicated and I'd be happy
if anybody finds out how to fix that...


Corinna

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney]

On 25/01/2024 18:21, Corinna Vinschen wrote:
> On Jan 25 14:50, Jon Turney wrote:
>> On 24/01/2024 14:39, Corinna Vinschen wrote:
>>> On Jan 24 13:28, Jon Turney wrote:
>>>> On 23/01/2024 14:29, Corinna Vinschen wrote:
>>>>> On Jan 23 14:20, Jon Turney wrote:
[...]
>> So this situation with a JIT debugger is even stranger than my emendations
>> to the documentation say.
>>
>> Because we're hitting try_to_debug() in exception::handle(), which has some
>> code to replay the exception via ExceptionContinueExecution (which I guess
>> the debugger will catch as a first-chance) (and goes into a mode where it
>> ignores the next half-million exceptions... no idea what that's about!)
>>
>> That's not the same as signal_exit() with a coredumping signal (haven't
>> checked if those are all generated from exceptions, but seems probable, so
>> the try_to_debug() there maybe isn't doing anything?), where we're going to
>> exit thereafter.
> 
> try_to_debug() is only calling IsDebuggerPresent() as test, and that's
> nothing but a flag in the PEB which is set by the OS after a debugger
> attached to the process.  So the test is by definition extremely
> flaky, if the debugger is connecting and disconnecting, as you
> already pointed out.
> 
> I'm wondering if we can't define our own way to attach to a process,
> allowing to "WaitForDebugger" as long as the debugger is a Cygwin
> debugger.  If we define a matching function (along the lines of
> prctl(2) on Linux), we could change our debuggers, core dumpers
> and stracers to call this attach function.
> 
> The idea would be to define some shared mutex object, the inferior
> waits for and the debugger releases after having attached.
> 
> Is there really any need to support non-Cygwin debuggers?

idk

I think something like that used to exist a long time ago, see commit 
8abeff1ead5f3824c70111bc0ff74ff835dafa55

That long predates my involvement with cygwin so I've no idea why that 
was removed.

>> The practical upshot of this is if the JIT debugger doesn't terminate or fix
>> the erroring process, we'll just replay the faulting instruction and invoke
>> the JIT debugger again.
> 
> Hmm, ok.  This signal stuff *is* complicated and I'd be happy
> if anybody finds out how to fix that...

To be clear, not a problem with "core dumping signals", as the process 
now always end up exiting, one way or another.

It's only a problem when someone has set "CYGWIN=error_start:true" or 
something equally dumb.

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Corinna Vinschen]

On Jan 25 20:03, Jon Turney wrote:
> On 25/01/2024 18:21, Corinna Vinschen wrote:
> > On Jan 25 14:50, Jon Turney wrote:
> > > On 24/01/2024 14:39, Corinna Vinschen wrote:
> > > > On Jan 24 13:28, Jon Turney wrote:
> > > > > On 23/01/2024 14:29, Corinna Vinschen wrote:
> > > > > > On Jan 23 14:20, Jon Turney wrote:
> [...]
> > > So this situation with a JIT debugger is even stranger than my emendations
> > > to the documentation say.
> > > 
> > > Because we're hitting try_to_debug() in exception::handle(), which has some
> > > code to replay the exception via ExceptionContinueExecution (which I guess
> > > the debugger will catch as a first-chance) (and goes into a mode where it
> > > ignores the next half-million exceptions... no idea what that's about!)
> > > 
> > > That's not the same as signal_exit() with a coredumping signal (haven't
> > > checked if those are all generated from exceptions, but seems probable, so
> > > the try_to_debug() there maybe isn't doing anything?), where we're going to
> > > exit thereafter.
> > 
> > try_to_debug() is only calling IsDebuggerPresent() as test, and that's
> > nothing but a flag in the PEB which is set by the OS after a debugger
> > attached to the process.  So the test is by definition extremely
> > flaky, if the debugger is connecting and disconnecting, as you
> > already pointed out.
> > 
> > I'm wondering if we can't define our own way to attach to a process,
> > allowing to "WaitForDebugger" as long as the debugger is a Cygwin
> > debugger.  If we define a matching function (along the lines of
> > prctl(2) on Linux), we could change our debuggers, core dumpers
> > and stracers to call this attach function.
> > 
> > The idea would be to define some shared mutex object, the inferior
> > waits for and the debugger releases after having attached.
> > 
> > Is there really any need to support non-Cygwin debuggers?
> 
> idk
> 
> I think something like that used to exist a long time ago, see commit
> 8abeff1ead5f3824c70111bc0ff74ff835dafa55

Yeah, just, as was the default at the time, without any trace of a
*rational* why it has been removed.  Also, it was too simple anyway.

First, if we want to support WIndows debugger, the inferior has to check
if the debugger is a Cygwin or native debugger.  If a native debugger,
just stick to what we have today.  Otherwise:

- Create a named mutex with a reproducible name (no need to use
  the name as parameter) and immediately grab it.
- Call CreateProcess to start the debugger with CREATE_SUSPENDED
  flag.
- Create a HANDLE array with the mutex and the process HANDLE.
- Call ResumeThread on the primary debugger thread.
- Call WFMO with timeout.

Later on, the debugger either fails and exits or it calls
ReleaseMutex after having attached to the process.

- WFMO returns
- If the mutex has triggered, we're being debugged (but check
  IsDebuggerPresent() just to be sure)
- If the process has triggered, the debugger exited
- If the timeout triggers... oh well.

> That long predates my involvement with cygwin so I've no idea why that was
> removed.

It doesn't predate mine, but I know just as much as you do.
Maybe the mailing list archives help, but tht's no safe bet.

> > > The practical upshot of this is if the JIT debugger doesn't terminate or fix
> > > the erroring process, we'll just replay the faulting instruction and invoke
> > > the JIT debugger again.
> > 
> > Hmm, ok.  This signal stuff *is* complicated and I'd be happy
> > if anybody finds out how to fix that...
> 
> To be clear, not a problem with "core dumping signals", as the process now
> always end up exiting, one way or another.
> 
> It's only a problem when someone has set "CYGWIN=error_start:true" or
> something equally dumb.

Ok.


Corinna

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Corinna Vinschen]

On Jan 26 12:12, Corinna Vinschen wrote:
> On Jan 25 20:03, Jon Turney wrote:
> > On 25/01/2024 18:21, Corinna Vinschen wrote:
> > > On Jan 25 14:50, Jon Turney wrote:
> > > > On 24/01/2024 14:39, Corinna Vinschen wrote:
> > > > > On Jan 24 13:28, Jon Turney wrote:
> > > > > > On 23/01/2024 14:29, Corinna Vinschen wrote:
> > > > > > > On Jan 23 14:20, Jon Turney wrote:
> > [...]
> > > > So this situation with a JIT debugger is even stranger than my emendations
> > > > to the documentation say.
> > > > 
> > > > Because we're hitting try_to_debug() in exception::handle(), which has some
> > > > code to replay the exception via ExceptionContinueExecution (which I guess
> > > > the debugger will catch as a first-chance) (and goes into a mode where it
> > > > ignores the next half-million exceptions... no idea what that's about!)
> > > > 
> > > > That's not the same as signal_exit() with a coredumping signal (haven't
> > > > checked if those are all generated from exceptions, but seems probable, so
> > > > the try_to_debug() there maybe isn't doing anything?), where we're going to
> > > > exit thereafter.
> > > 
> > > try_to_debug() is only calling IsDebuggerPresent() as test, and that's
> > > nothing but a flag in the PEB which is set by the OS after a debugger
> > > attached to the process.  So the test is by definition extremely
> > > flaky, if the debugger is connecting and disconnecting, as you
> > > already pointed out.
> > > 
> > > I'm wondering if we can't define our own way to attach to a process,
> > > allowing to "WaitForDebugger" as long as the debugger is a Cygwin
> > > debugger.  If we define a matching function (along the lines of
> > > prctl(2) on Linux), we could change our debuggers, core dumpers
> > > and stracers to call this attach function.
> > > 
> > > The idea would be to define some shared mutex object, the inferior
> > > waits for and the debugger releases after having attached.
> > > 
> > > Is there really any need to support non-Cygwin debuggers?
> > 
> > idk
> > 
> > I think something like that used to exist a long time ago, see commit
> > 8abeff1ead5f3824c70111bc0ff74ff835dafa55
> 
> Yeah, just, as was the default at the time, without any trace of a
> *rational* why it has been removed.  Also, it was too simple anyway.
> 
> First, if we want to support WIndows debugger, the inferior has to check
> if the debugger is a Cygwin or native debugger.  If a native debugger,
> just stick to what we have today.  Otherwise:
> 
> - Create a named mutex with a reproducible name (no need to use
>   the name as parameter) and immediately grab it.
> - Call CreateProcess to start the debugger with CREATE_SUSPENDED
>   flag.
> - Create a HANDLE array with the mutex and the process HANDLE.

    On second thought, it might be a good idea to make this
    interruptible as well, but given this is called from the
    exception handler this may have weird results...
 
> - Call ResumeThread on the primary debugger thread.
> - Call WFMO with timeout.
> 
> Later on, the debugger either fails and exits or it calls
> ReleaseMutex after having attached to the process.
> 
> - WFMO returns
> - If the mutex has triggered, we're being debugged (but check
>   IsDebuggerPresent() just to be sure)
> - If the process has triggered, the debugger exited
> - If the timeout triggers... oh well.

Corinna

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney]

On 26/01/2024 11:52, Corinna Vinschen wrote:
> On Jan 26 12:12, Corinna Vinschen wrote:
>> On Jan 25 20:03, Jon Turney wrote:
>>> On 25/01/2024 18:21, Corinna Vinschen wrote:
>>>> On Jan 25 14:50, Jon Turney wrote:
>>>>> On 24/01/2024 14:39, Corinna Vinschen wrote:
>>>>>> On Jan 24 13:28, Jon Turney wrote:
>>>>>>> On 23/01/2024 14:29, Corinna Vinschen wrote:
>>>>>>>> On Jan 23 14:20, Jon Turney wrote:
>>> [...]
>>>>> So this situation with a JIT debugger is even stranger than my emendations
>>>>> to the documentation say.
>>>>>
>>>>> Because we're hitting try_to_debug() in exception::handle(), which has some
>>>>> code to replay the exception via ExceptionContinueExecution (which I guess
>>>>> the debugger will catch as a first-chance) (and goes into a mode where it
>>>>> ignores the next half-million exceptions... no idea what that's about!)
>>>>>
>>>>> That's not the same as signal_exit() with a coredumping signal (haven't
>>>>> checked if those are all generated from exceptions, but seems probable, so
>>>>> the try_to_debug() there maybe isn't doing anything?), where we're going to
>>>>> exit thereafter.
>>>>
>>>> try_to_debug() is only calling IsDebuggerPresent() as test, and that's
>>>> nothing but a flag in the PEB which is set by the OS after a debugger
>>>> attached to the process.  So the test is by definition extremely
>>>> flaky, if the debugger is connecting and disconnecting, as you
>>>> already pointed out.
>>>>
>>>> I'm wondering if we can't define our own way to attach to a process,
>>>> allowing to "WaitForDebugger" as long as the debugger is a Cygwin
>>>> debugger.  If we define a matching function (along the lines of
>>>> prctl(2) on Linux), we could change our debuggers, core dumpers
>>>> and stracers to call this attach function.
>>>>
>>>> The idea would be to define some shared mutex object, the inferior
>>>> waits for and the debugger releases after having attached.
>>>>
>>>> Is there really any need to support non-Cygwin debuggers?
>>>
>>> idk
>>>
>>> I think something like that used to exist a long time ago, see commit
>>> 8abeff1ead5f3824c70111bc0ff74ff835dafa55
>>
>> Yeah, just, as was the default at the time, without any trace of a
>> *rational* why it has been removed.  Also, it was too simple anyway.
>>
>> First, if we want to support WIndows debugger, the inferior has to check
>> if the debugger is a Cygwin or native debugger.  If a native debugger,
>> just stick to what we have today.  Otherwise:
>>
>> - Create a named mutex with a reproducible name (no need to use
>>    the name as parameter) and immediately grab it.
>> - Call CreateProcess to start the debugger with CREATE_SUSPENDED
>>    flag.
>> - Create a HANDLE array with the mutex and the process HANDLE.
> 
>      On second thought, it might be a good idea to make this
>      interruptible as well, but given this is called from the
>      exception handler this may have weird results...
>   
>> - Call ResumeThread on the primary debugger thread.
>> - Call WFMO with timeout.
>>
>> Later on, the debugger either fails and exits or it calls
>> ReleaseMutex after having attached to the process.
>>
>> - WFMO returns
>> - If the mutex has triggered, we're being debugged (but check
>>    IsDebuggerPresent() just to be sure)
>> - If the process has triggered, the debugger exited
>> - If the timeout triggers... oh well.

This seems like quite a lot of work, for very marginal benefit.

And doing lots of complex work inside the process when we're in the 
middle of handling a SEGV seems like asking for trouble.

I think I'll leave this alone for the moment, and we can see what (if 
any) problems surface.

Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Corinna Vinschen]

On Jan 27 15:12, Jon Turney wrote:
> On 26/01/2024 11:52, Corinna Vinschen wrote:
> > > - Create a named mutex with a reproducible name (no need to use
> > >    the name as parameter) and immediately grab it.
> > > - Call CreateProcess to start the debugger with CREATE_SUSPENDED
> > >    flag.
> > > - Create a HANDLE array with the mutex and the process HANDLE.
> > 
> >      On second thought, it might be a good idea to make this
> >      interruptible as well, but given this is called from the
> >      exception handler this may have weird results...
> > > - Call ResumeThread on the primary debugger thread.
> > > - Call WFMO with timeout.
> > > 
> > > Later on, the debugger either fails and exits or it calls
> > > ReleaseMutex after having attached to the process.
> > > 
> > > - WFMO returns
> > > - If the mutex has triggered, we're being debugged (but check
> > >    IsDebuggerPresent() just to be sure)
> > > - If the process has triggered, the debugger exited
> > > - If the timeout triggers... oh well.
> 
> This seems like quite a lot of work, for very marginal benefit.
> 
> And doing lots of complex work inside the process when we're in the middle
> of handling a SEGV seems like asking for trouble.
> 
> I think I'll leave this alone for the moment, and we can see what (if any)
> problems surface.

Ok, no worries.


Corinna