[PATCH] Cygwin: Remove waitloop argument from try_to_debug()

[PATCH] Cygwin: Remove waitloop argument from try_to_debug()

[Jon Turney]

Currently, when using CYGWIN='error_start=dumper', the core dump written
in response to an exception is non-deterministic, as the faulting
process isn't stopped while the dumper is started (it even seems
possible in theory that the faulting process could have exited before
the dumper process attaches).

Remove the waitloop argument, only used in this case, so the faulting
process busy-waits until the dump starts.

Code archeology to determine why the code is this way didn't really turn
up any answers, but this seems a low-risk change, as this only changes
the behaviour when:

 - a debugger isn't already attached
 - an error_start is specified in CYGWIN env var
 - an exception has occured which will be translated to a signal

Future work: This probably can be further simplified to make it
completely synchronous by waiting for the dumper process to exit. This
would avoid the race condition of the dumper attaching and detaching
before we get around to checking for that (which we try to work around
by juggling thread priorities), and the failure state where the dumper
doesn't attach and we spin indefinitely.
---
 winsup/cygwin/exceptions.cc | 8 ++------
 winsup/cygwin/winsup.h      | 2 +-
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index bb7704f94..3ed2db1eb 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -461,10 +461,8 @@ cygwin_stackdump ()
   exc.dumpstack ();
 }
 
-#define TIME_TO_WAIT_FOR_DEBUGGER 10000
-
 extern "C" int
-try_to_debug (bool waitloop)
+try_to_debug ()
 {
   if (!debugger_command)
     return 0;
@@ -537,8 +535,6 @@ try_to_debug (bool waitloop)
     system_printf ("Failed to start debugger, %E");
   else
     {
-      if (!waitloop)
-	return dbg;
       SetThreadPriority (GetCurrentThread (), THREAD_PRIORITY_IDLE);
       while (!being_debugged ())
 	Sleep (1);
@@ -812,7 +808,7 @@ exception::handle (EXCEPTION_RECORD *e, exception_list *frame, CONTEXT *in,
   if (exit_state >= ES_SIGNAL_EXIT
       && (NTSTATUS) e->ExceptionCode != STATUS_CONTROL_C_EXIT)
     api_fatal ("Exception during process exit");
-  else if (!try_to_debug (0))
+  else if (!try_to_debug ())
     rtl_unwind (frame, e);
   else
     {
diff --git a/winsup/cygwin/winsup.h b/winsup/cygwin/winsup.h
index 79844cb87..0ffd8c5af 100644
--- a/winsup/cygwin/winsup.h
+++ b/winsup/cygwin/winsup.h
@@ -190,7 +190,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" int try_to_debug (bool waitloop = 1);
+extern "C" int try_to_debug ();
 
 void ld_preload ();
 void fixup_hooks_after_fork ();
-- 
2.28.0

Re: [PATCH] Cygwin: Remove waitloop argument from try_to_debug()

[Corinna Vinschen]

On Aug 29 15:43, Jon Turney wrote:
> Currently, when using CYGWIN='error_start=dumper', the core dump written
> in response to an exception is non-deterministic, as the faulting
> process isn't stopped while the dumper is started (it even seems
> possible in theory that the faulting process could have exited before
> the dumper process attaches).
> 
> Remove the waitloop argument, only used in this case, so the faulting
> process busy-waits until the dump starts.
> 
> Code archeology to determine why the code is this way didn't really turn
> up any answers, but this seems a low-risk change, as this only changes
> the behaviour when:
> 
>  - a debugger isn't already attached
>  - an error_start is specified in CYGWIN env var
>  - an exception has occured which will be translated to a signal
> 
> Future work: This probably can be further simplified to make it
> completely synchronous by waiting for the dumper process to exit. This
> would avoid the race condition of the dumper attaching and detaching
> before we get around to checking for that (which we try to work around
> by juggling thread priorities), and the failure state where the dumper
> doesn't attach and we spin indefinitely.
> ---
>  winsup/cygwin/exceptions.cc | 8 ++------
>  winsup/cygwin/winsup.h      | 2 +-
>  2 files changed, 3 insertions(+), 7 deletions(-)

I'm a bit fuzzy on the implications but it doesn't look like it
hurts a lot(*).  Let's get it in.


Thanks,
Corinna


(*) Famous last words alarm...

Re: [PATCH] Cygwin: Remove waitloop argument from try_to_debug()

[Jon Turney]

On 30/08/2020 13:47, Corinna Vinschen wrote:
> On Aug 29 15:43, Jon Turney wrote:
>> Currently, when using CYGWIN='error_start=dumper', the core dump written
>> in response to an exception is non-deterministic, as the faulting
>> process isn't stopped while the dumper is started (it even seems
>> possible in theory that the faulting process could have exited before
>> the dumper process attaches).
>>
>> Remove the waitloop argument, only used in this case, so the faulting
>> process busy-waits until the dump starts.
>>
>> Code archaeology to determine why the code is this way didn't really turn
>> up any answers, but this seems a low-risk change, as this only changes
>> the behaviour when:
>>
>>   - a debugger isn't already attached
>>   - an error_start is specified in CYGWIN env var
>>   - an exception has occurred which will be translated to a signal
>>
>> Future work: This probably can be further simplified to make it
>> completely synchronous by waiting for the dumper process to exit. This
>> would avoid the race condition of the dumper attaching and detaching
>> before we get around to checking for that (which we try to work around
>> by juggling thread priorities), and the failure state where the dumper
>> doesn't attach and we spin indefinitely.

So, on reflection, this idea is wrong, and it currently is the way it 
has to be.

If we use CYGWIN='error_start=gdb', we should be able to continue the 
thread which encountered an exception, which we can't do if it's blocked 
waiting for the error_start process to exit.

So I'll tweak the patch commentary before pushing.