[PATCH v2 0/1] MR1128: msvcrt: Display message box in abort() in debug CRT only.
As far as my testing goes, the message box is never shown on Windows when abort() is called with retail runtime is used, regardless of error mode and abort behaviour. That is not the case with _assert which still can show a dialog even in release mode. It happens rather often that an app meets the error condition during the process exit and that goes silent on Windows while we display the abort dialog on app's exit. The test program below shows that (and the same behaviour is, e. g., with ucrtbase vs ucrtbased). ``` #include <windows.h> #include <stdio.h> #include <stdlib.h> void CDECL _set_app_type(int app_type); #define D(name) static typeof(name) *p_##name #define L(name) p_##name = (void *)GetProcAddress(hdll, #name); if (!p_##name) printf("NULL %s.\n", #name) D(abort); D(_set_app_type); D(_set_abort_behavior); D(_set_error_mode); int main(int argc, char *argv[]) { HMODULE hdll = LoadLibraryA("msvcrt.dll"); printf("hdll %p.\n", hdll); L(_set_app_type); if (!p__set_app_type) if (!(p__set_app_type = (void *)GetProcAddress(hdll, "__set_app_type"))) printf("NULL p__set_app_type.\n"); L(abort); L(_set_abort_behavior); L(_set_error_mode); p__set_app_type(2); if (p__set_abort_behavior) p__set_abort_behavior(_WRITE_ABORT_MSG, _WRITE_ABORT_MSG); p__set_error_mode(_OUT_TO_MSGBOX); p_abort(); } ``` -- v2: msvcrt: Display message box in abort() for specific CRT versions only. https://gitlab.winehq.org/wine/wine/-/merge_requests/1128
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/msvcrt/exit.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/msvcrt/exit.c b/dlls/msvcrt/exit.c index 323d205b1bd..41637ed7a0f 100644 --- a/dlls/msvcrt/exit.c +++ b/dlls/msvcrt/exit.c @@ -250,6 +250,7 @@ void CDECL abort(void) { TRACE("()\n"); +#if _MSVCR_VER < 100 || _MSVCR_VER == 120 || defined(_DEBUG) if (MSVCRT_abort_behavior & _WRITE_ABORT_MSG) { if ((MSVCRT_error_mode == _OUT_TO_MSGBOX) || @@ -260,6 +261,7 @@ void CDECL abort(void) else _cputs("\nabnormal program termination\n"); } +#endif raise(SIGABRT); /* in case raise() returns */ _exit(3); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1128
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125247 Your paranoid android. === debian11 (build log) === Task: Could not create the win32 wineprefix: Failed to disable the crash dialogs: Task: WineTest did not produce the win32 report
I checked by installing various old redistributables that the message is always displayed before version 100, while version 120 is an exception where it is displayed as well. Given we currently have only msvcrtd debug version the '|| _DEBUG' condition is redundant, but I suppose it is better to have it at once as that is something easy to miss when adding, e. g., ucrtbased.dll. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1128#note_11716
Piotr Caban (@piotr) commented about dlls/msvcrt/exit.c:
{ TRACE("()\n");
+#if _MSVCR_VER < 100 || _MSVCR_VER == 120 || defined(_DEBUG) You're no longer changing it for msvcrt.dll. Also _DEBUG is not defined in msvcrtd.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1128#note_11721
On Fri Oct 21 14:42:11 2022 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125247 Your paranoid android. === debian11 (build log) === Task: Could not create the win32 wineprefix: Failed to disable the crash dialogs: Task: WineTest did not produce the win32 report ``` Eh, I am guessing this error might not be entirely unrelated, maybe Testbot looks for abnormal termination message in stderr which is now also intentionally removed in this patch version. But I am not sure what to do with it, that should be probably fixed on testbot?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1128#note_11722
On Fri Oct 21 14:48:23 2022 +0000, Piotr Caban wrote:
You're no longer changing it for msvcrt.dll. Also _DEBUG is not defined in msvcrtd. Yes, since msvcrt from old redist does display the message (unlike the one on up to date Win10 which I was testing with previously). Although we should match the new one probably?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1128#note_11723
On Fri Oct 21 14:52:08 2022 +0000, Paul Gofman wrote:
Yes, since msvcrt from old redist does display the message (unlike the one on up to date Win10 which I was testing with previously). Although we should match the new one probably? I guess we should match the new one unless it breaks an older app. BTW what's the motivation behind this patch?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1128#note_11724
On Fri Oct 21 14:50:08 2022 +0000, Paul Gofman wrote:
Eh, I am guessing this error might not be entirely unrelated, maybe Testbot looks for abnormal termination message in stderr which is now also intentionally removed in this patch version. But I am not sure what to do with it, that should be probably fixed on testbot? This error shows today on all MRs.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1128#note_11725
I guess we should match the new one unless it breaks an older app. BTW what's the motivation behind this patch? GitLab
I looked up a game which shows this dialog on exit and figured that the same abort() happens on Windows too and just doesn't result in the message box, so the app just exits cleanly. I think I came across more bugs concerning this error message on exit but didn't look at those hoping that maybe making this part behave like on Windows will just solve similar issues. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1128#note_11729
participants (4)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Piotr Caban (＠piotr)