On 8 August 2016 at 23:29, Stefan <luke1...@posteo.de> wrote: > On 8/8/2016 14:53, Stefan Hett wrote: >> On 8/8/2016 12:22 PM, Ivan Zhakov wrote: >> >>> On 7 August 2016 at 23:14, Stefan <luke1...@posteo.de> wrote: >>> >>> Btw what are the problems with approach to disable watson reports on >>> abort(), except backporting? I mean we already override Windows Error >>> Reporting by installing our own SEH exception handler, so it looks >>> natural to disable abort() reporting also. >>> >> I agree with you on this one. I wasn't aware that SVN does already >> alter the default exception handling on Windows (and therefore >> effectively disable the Watson crash dump reports in case of an >> unhanled exception). So disabling the Watson crash dumps on abort >> calls does indeed sound like increasing the system/design consistency >> in SVN. >> You got my non-binding +1 on that proposal (which obviously would be a >> replacement for my proposed patch). >> > I've been thinking more about your arguments and since I agree with you, > I'd like to replace my previously proposed patch with this updated > version based on your idea. > Due to the behavior change it imposes, I'm not going to propose this for > backporting to 1.8 or 1.9, though. > > Note that I kept the design to only disable Watson crash reports, unless > SVN_CMDLINE_USE_DIALOG_FOR_ABORT is set. The reasoning is as given in my > previous reply as in as far as I understand the design of that > environment-variable-setting, it should control whether an abort > produces a popup or not. Disabling the Watson crash reports would > prevent that popup (in certain cases) and therefore negate the setting. > Hi Stefan,
I've tested your patch, but it seems that calling _set_abort_behavior(0, _CALL_REPORTFAULT) is not enough. The problem with this approach that abort() uses RaiseFailFastException() that bypasses all exception handlers [1]. So Subversion doesn't write crash dump and even doesn't print error message on abort() failure. One solution would be to register signal handler() and use RaiseException(STATUS_FATAL_APP_EXIT). This exception would be handled by our own exception handler. Subversion already uses such approach for handling out-of-memory errors (see r1724784 [2]). I've tested this approach (see attached patch) and it seems to be working fine, but I want to test it more before committing. [1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd941688(v=vs.85).aspx [2] https://svn.apache.org/r1724784 -- Ivan Zhakov
Index: subversion/libsvn_subr/cmdline.c =================================================================== --- subversion/libsvn_subr/cmdline.c (revision 1751923) +++ subversion/libsvn_subr/cmdline.c (working copy) @@ -91,6 +91,12 @@ static svn_boolean_t shortcut_stderr_to_console = FALSE; #endif +#if defined(WIN32) && defined(SVN_USE_WIN32_CRASHHANDLER) +static void __cdecl win32_sigabrt_handler(int sig) +{ + RaiseException(STATUS_FATAL_APP_EXIT, EXCEPTION_NONCONTINUABLE, 0, NULL); +} +#endif int svn_cmdline_init(const char *progname, FILE *error_stream) @@ -163,6 +169,13 @@ /* In release mode: Redirect abort() errors to stderr */ _set_error_mode(_OUT_TO_STDERR); + /* In release mode: Disable Watson crash reports on abort(). */ + _set_abort_behavior(0, _CALL_REPORTFAULT); + + /* Register abort() signal handler to create crash dump on + abort(). */ + apr_signal(SIGABRT, win32_sigabrt_handler); + /* In _DEBUG mode: Redirect all debug output (E.g. assert() to stderr. (Ignored in release builds) */ _CrtSetReportFile( _CRT_WARN, _CRTDBG_FILE_STDERR); Index: subversion/tests/svn_test_main.c =================================================================== --- subversion/tests/svn_test_main.c (revision 1751923) +++ subversion/tests/svn_test_main.c (working copy) @@ -879,6 +879,9 @@ /* In release mode: Redirect abort() errors to stderr */ _set_error_mode(_OUT_TO_STDERR); + /* In release mode: Disable Watson crash reports on abort(). */ + _set_abort_behavior(0, _CALL_REPORTFAULT); + /* In _DEBUG mode: Redirect all debug output (E.g. assert() to stderr. (Ignored in releas builds) */ _CrtSetReportFile( _CRT_ASSERT, _CRTDBG_FILE_STDERR);