On 8 August 2016 at 23:29, Stefan <[email protected]> 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 <[email protected]> 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);
