set error mode to remove error popups #2445

emekoi · 2019-05-07T21:44:39Z

closes #2071

andrewrk · 2019-05-08T16:45:36Z

@Sahnvour if you have time can I get your opinion on this PR?

is this a good default?
should this go in the default panic handler rather than before main()?
if this is what windows expects from applications, why doesn't it make it the default?

Sahnvour · 2019-05-08T16:59:41Z

Looks good to me, just a couple things:

The documentation @emekoi linked in the issue mentions SetThreadErrorMode (https://msdn.microsoft.com/en-us/library/dd553630(v=vs.85).aspx) which is recommended under the Remarks: Windows 7 section.
What I understand is that it's preferred if you can use this over process-wide error mode. Then I would advise to use it if builtin.single_threaded is true.
As for the mention of W7, I assume it holds true for W10 and the documentation is just not perfectly up-to-date, as SetThreadErrorMode is available only since W7.
The process error mode is transmitted to child processes when invoked via CreateProcess, and we might want to disable that by default to avoid disturbing them. Raymond Chen mentions this and offers a solution at https://devblogs.microsoft.com/oldnewthing/20130626-00/?p=3983, I think we should follow his advice.

emekoi · 2019-05-09T02:27:58Z

i wasn't sure what the lowest version of windows that zig wanted to guarantee compatibility with was. because if SetThreadErrorMode is used then standard zig binaries wouldn't support anything before windows 7.

edit: perhaps we should add a minimum version for the windows entries on the support table?

daurnimator · 2019-05-09T03:35:22Z

Shouldn't we only do this for command-line applications? Perhaps change this based on SUBSYSTEM?

emekoi · 2019-05-09T04:23:08Z

@daurnimator zig doesn't expose the subsystem to user code.

Sahnvour · 2019-05-12T21:53:26Z

@emekoi, @andrewrk updated the support table, and Windows 7 is the minimal supported version. So it's OK to use SetThreadErrorMode for single-threaded builds.

https://ziglang.org/#Support-Table

emekoi · 2019-05-18T21:40:44Z

@Sahnvour what would the advantage be of using SetThreadErrorMode instead of SetErrorMode?

Sahnvour · 2019-05-18T22:32:23Z

I would mainly do it because the docs say so. But after some research, it seems the problem is that the error mode is a process global and multiple threads accessing/modifying it is not safe.
https://github.com/dotnet/corefx/issues/19738
https://lists.gnu.org/archive/html/libtool-patches/2010-03/msg00006.html

That's probably not a concern in this specific case because we know no other thread can mess with it, but as a rule of thumb I think using a more restricted API/feature should be preferred, when available.

emekoi · 2019-05-28T22:28:37Z

preferably merge #2462 before this one so, so we can switch on the subsystem and decide whether to enable error popups depending on that.

andrewrk · 2019-07-03T22:32:41Z

std/special/start.zig

@@ -61,6 +61,14 @@ extern fn WinMainCRTStartup() noreturn {
    if (!builtin.single_threaded) {
        _ = @import("start_windows_tls.zig");
    }
+
+    // TODO we need a userland function for detecting the subsystem


Is this comment still accurate? What's the issue? Can I help with it?

sorry i forgot to remove this. that fucntion is std.os.windows.getSubSystem

andrewrk · 2019-07-03T22:33:23Z

std/os/windows.zig

@@ -779,3 +779,18 @@ pub fn unexpectedError(err: DWORD) std.os.UnexpectedError {
    }
    return error.Unexpected;
 }
+
+pub fn getSubSystem() ?builtin.SubSystem {


What do you think about making this be a pub const subsystem = blk: { ... }; rather than a function? This way it will always be comptime known.

sure, but this implementation isn't neccessarily complete. if you have const wWinMain = 0 in your root file it will report the wrong susbystem.

andrewrk · 2019-07-03T22:57:42Z

* Raymond Chen mentions this and offers a solution at https://devblogs.microsoft.com/oldnewthing/20130626-00/?p=3983, I think we should follow his advice.

One thing I'm not clear on is why we should change from the default. Raymond Chen quotes from the documentation:

A child process inherits the error mode of its parent process.

So it seems then that the parent process has chosen the error mode, why should zig programs override that before main()?

I would mainly do it because the docs say so. But after some research, it seems the problem is that the error mode is a process global and multiple threads accessing/modifying it is not safe.

Note that this code is executing before main() and so the application has not had a chance to create threads yet. So a global function would not only work but probably be more appropriate.

andrewrk · 2019-07-03T23:04:46Z

One thing I'm not clear on is why we should change from the default. Raymond Chen quotes from the documentation:

Ah, @emekoi already answered this in #2071 (comment).

This does not say there should be a condition on the subsystem. Is there any reason to do this "best practice" thing based on the subsystem? Can anyone confirm that this is actually best practice?

emekoi · 2019-07-03T23:28:54Z

the condition on the subsystem is based on the reasoning that if you building an application under the windows subsystem error pop-ups are probably okay. if you're building a console application pop-ups probably aren't.

andrewrk · 2019-07-03T23:33:43Z

This reasoning implies that the "best practice" advice is not entirely correct, or at least not complete. Which makes me sort of question the whole thing, and brings me back to the question "So it seems then that the parent process has chosen the error mode, why should zig programs override that before main()?" e.g. cmd.exe in theory has the correct error mode set, why should child processes need to set it?

I think one data point that would be worth collecting here would be whether or not msvcrt calls SetErrorMode (or the threaded one) before calling main(). Is there a way to intercept Windows lib calls to find out?

andrewrk · 2019-07-03T23:46:51Z

The original issue is "When linked against windows' libc, calling abort opens a popup". It seems to me that:

The parent process has made a decision about what error mode to execute the child process in
The libc runtime has made a decision about whether or not to override the error mode before calling main.

I don't see why zig would then call SetErrorMode before main(). If you were to write a C program linked against msvcrt and then call assert(false), it would be the same behavior. Is assert() broken by default on windows when writing C code?

I mean, if this is a chance for zig to be better than C, great, let's do it. But there must be something I don't understand here, something smells fishy.

By the way, I don't get error popups when I run the zig test suite, does anyone else? As far as I remember, I didn't override any default config.

The original issue that #2445 wanted to fix was solved in the previous commit. However it also exposed the subsystem in the standard library, which is still useful. So that's done in this commit, and #2445 can be closed.

andrewrk requested a review from Sahnvour May 8, 2019 16:44

emekoi force-pushed the windows-abort branch from b0353d4 to d023eaf Compare May 19, 2019 05:36

emekoi force-pushed the windows-abort branch from c043dc6 to ddaab3e Compare May 28, 2019 22:16

andrewrk mentioned this pull request May 29, 2019

added subsystem to builtin.zig #2462

Closed

emekoi force-pushed the windows-abort branch 2 times, most recently from 93f99a5 to bf769f4 Compare May 29, 2019 23:21

emekoi changed the title ~~set error mode to remove error popups~~ [WIP] set error mode to remove error popups May 29, 2019

emekoi added 3 commits July 3, 2019 10:37

set error mode to remove error popups

322393e

switched to thread-safe errormode functions

1db53ea

added userland function for getting subsystem

90ea77e

emekoi force-pushed the windows-abort branch from bf769f4 to 90ea77e Compare July 3, 2019 16:44

emekoi changed the title ~~[WIP] set error mode to remove error popups~~ set error mode to remove error popups Jul 3, 2019

andrewrk reviewed Jul 3, 2019

View reviewed changes

make subsystem comptime known

d9c30df

andrewrk closed this Jul 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set error mode to remove error popups #2445

set error mode to remove error popups #2445

emekoi commented May 7, 2019

andrewrk commented May 8, 2019

Sahnvour commented May 8, 2019 •

edited

Loading

emekoi commented May 9, 2019 •

edited

Loading

daurnimator commented May 9, 2019

emekoi commented May 9, 2019

Sahnvour commented May 12, 2019

emekoi commented May 18, 2019 •

edited

Loading

Sahnvour commented May 18, 2019

emekoi commented May 28, 2019

andrewrk Jul 3, 2019

emekoi Jul 3, 2019 •

edited

Loading

andrewrk Jul 3, 2019

emekoi Jul 3, 2019

andrewrk commented Jul 3, 2019

andrewrk commented Jul 3, 2019

emekoi commented Jul 3, 2019

andrewrk commented Jul 3, 2019 •

edited

Loading

andrewrk commented Jul 3, 2019 •

edited

Loading

set error mode to remove error popups #2445

set error mode to remove error popups #2445

Conversation

emekoi commented May 7, 2019

andrewrk commented May 8, 2019

Sahnvour commented May 8, 2019 • edited Loading

emekoi commented May 9, 2019 • edited Loading

daurnimator commented May 9, 2019

emekoi commented May 9, 2019

Sahnvour commented May 12, 2019

emekoi commented May 18, 2019 • edited Loading

Sahnvour commented May 18, 2019

emekoi commented May 28, 2019

andrewrk Jul 3, 2019

Choose a reason for hiding this comment

emekoi Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

andrewrk Jul 3, 2019

Choose a reason for hiding this comment

emekoi Jul 3, 2019

Choose a reason for hiding this comment

andrewrk commented Jul 3, 2019

andrewrk commented Jul 3, 2019

emekoi commented Jul 3, 2019

andrewrk commented Jul 3, 2019 • edited Loading

andrewrk commented Jul 3, 2019 • edited Loading

Sahnvour commented May 8, 2019 •

edited

Loading

emekoi commented May 9, 2019 •

edited

Loading

emekoi commented May 18, 2019 •

edited

Loading

emekoi Jul 3, 2019 •

edited

Loading

andrewrk commented Jul 3, 2019 •

edited

Loading

andrewrk commented Jul 3, 2019 •

edited

Loading