This is a great leap towards closing a very tedious piece of behaviour on Windows, thanks!

Just to expand on the problem: since Visual Studio 2005, the Microsoft CRT includes various calls (principally via abort) which clear the exception filter in order to guarantee that the runtime is torn down. MinGW in its default configuration is therefore unaffected because the core msvcrt included in Windows is based on .NET 2002/3's implementation. However, given its latest incarnation (the Universal CRT or UCRT) is now a Windows component again, that "stability" is not guaranteed to last and in fact I think mingw can already be configured to use other versions of the CRT if you wish.

I'm not (yet) 100% convinced about the equivalence of using a vectored exception handler instead of the unhandled filter - but I'm just putting that out there for now (the alternative is to hook SetUnhandledExceptionFilter and effectively disable the CRT from calling it)

I haven't delved into the history enough - was supporting Windows '9x the original reason for being unable to detect stack overflow?

I had a look at the sources of the CRT, trying to find out why SetUnhandledExceptionFilter isn't able to catch the exception — no success.

was supporting Windows '9x the original reason for being unable to detect stack overflow?

er what do you mean ? Stack overflow detection was working, just only on 32-bits and not with the recent MSVC CRT that prevent SetUnhandledExceptionFilter from working.
I simply removed the Win9x code because it's obsolete now, OCaml requires at least WinXP, right ?

OK, looked at this in a bit more detail (including building it). I did not realise that 32 bit native stack overflow detection actually worked because as it happens on my machine it's masked by the crash dialog (and possibly the fact that the machine I'm sat at has a slightly old OCaml by default) and had always assumed it wasn't available on 32 or 64-bit and, more importantly, the test for it has always been skipped (I'm very familiar with differences in the testsuite between the platforms......)

I think that it should be refactored to use HAS_STACK_OVERFLOW_DETECTION - the fact that the test was skipped before was a bug and I think that the bug is that s-nt.h should be defining that, rather than extra tests being included in the Makefile itself. The code in asmrun should be refactored so that the signals bit is under a further #ifdef and the Windows stuff under another but both activated by HAS_STACK_OVERFLOW_DETECTION. @shindere may have an opinion on this too - basically, changes should be moving towards allowing a clear configure route for the Windows ports where the features are similarly-named.

Could the relevant section in the manual be updated in this GPR as well (I don't think there's a policy of separating PRs - @gasche, @Octachron?)

On the contrary, having documentation changes in the same PR is better. (It helps reviewer who can read the doc, and also writing the doc can clarify things and prompt changes to the implementation.)

@dra27, OK waiting on some other opinion on this because I'm not too hot about the #ifdef twiddling.

@shindere - it could be made formal inasmuch as the s.h generated by configure could be converted into a Makefile include as well (or one could be generated at the same time), or potentially stack overflow detection should be included in the output of ocamlc -config... but that's possibly a separate matter? My main point is that the C side should be able to rely on the features detected in m.h and s.h being the same across platforms.

Given that only one (I think?) C file is affected, it doesn't seem too major a thing to need to do. @oandrieu - I'm not clear whether it's that you don't like the idea of the change or don't like the idea of making the change... if the latter, then I'm happy to push a commit showing it!

@shindere - the -4 - 1 corresponds to the Win64 calling convention: caller is supposed to allocate backing space on the stack for the 4 argument-passing registers ; the -1 is the space for the return address (unused here since we never return).

@dra27 - I don't quite see what you have in mind. HAS_STACK_OVERFLOW_DETECTION really means HAS_STACK_OVERFLOW_DETECTION_USING_UNIX_SIGSEGV_HANDLER. This PR is indeed the same feature, but implemented with completely different C code, in a different file. #define-ing it in s-nt.h will just pollute signals_asm.c which will have to exclude _WIN32 from all HAS_STACK_OVERFLOW sections.

I'm not enough of a Win32 guru to review this code, and I'm not sure we'll find enough experts who can. Still, my feeling is that the new code is probably better than the old one, even if not perfect, so we don't have anything to lose by using the new code.

I agree that AddVectoredExceptionHandler is a better alternative: I think that the culprit is gs_report.c in the CRT and that this comes about as part of the security features added in VS 2005 (related to the /GS option which my understanding is that release CRTs are compiled with). I couldn't quite get the hacky workaround which is meant to disable it completely to work in order to convince myself that was definitely the cause, annoyingly.

Having done more testing, I have three changes to propose:

1. I cannot see the motivation for commit 23ab217 (disabling the AppCrash dialog) - why is this necessary? At the moment, I would prefer this commit to be removed.
2. Either 3647699 or fab3b7a should make the following changes:

• asmrun/signals_asm.c L308-310 (the call to caml_win32_overflow_detection should be moved to asmrun/startup.c below the call to the caml_init_signals - this code never had any place here given, as you point out, that the handling has nothing to do with signals.
• The three places in asmrun/signals_asm.c where HAS_STACK_OVERFLOW_DETECTION is tested should be altered to #if defined(HAS_STACK_OVERFLOW_DETECTION) && !defined(_WIN32) (what you call pollution I call correct operation!).
• #define HAS_STACK_OVERFLOW_DETECTION should be added to config/s-nt.h. I'm not convinced it's necessary to split this into two feature defines (so both Windows and relevant Unices define HAS_STACK_OVERFLOW_DETECTION and the Unices also define HAS_STACK_OVERFLOW_DETECTION_SIGNAL) but I am concerned at how long the broken behaviour went unnoticed because of not setting features correctly in the first place.

3. Commit 84dc64e is then unnecessary

@xavierleroy, @shindere - what are your thoughts?

disabling the AppCrash dialog - why is this necessary?

Right, it's not. I just stumbled on the function in the documentation. I'm not sure what good is this crash dialog for an OCaml program, but yes it's unrelated to the whole stack overflow detection — I'll remove it.

the call to caml_win32_overflow_detection should be moved to asmrun/startup.c

OK, makes sense.

Commit 84dc64e is then unnecessary

Well the modification to the testcase is still useful to test that the system stack overflow detection has been properly re-set.

You are completely right about the new addition to the test case - sorry, I forgot that that there was too! Please do keep that :)

Could we restart discussion and come to a decision for this PR? Are there any pending issues? If not I'm in favor of using this code.

I've transposed the changes from commit 339bcbb. The stack probe is smaller (4K instead of 32K) but it still appropriately report a Stack_overflow on the testcase of PR#5064.

This is probably too late or too risky at this point for inclusion in 4.05, but I'm still in favor of merging in master soon, so that the code gets some exposure before ending in 4.06.

Does this code operate correctly if the fault happens in a thread that is not the main thread?

@mshinwell Yes I believe so. I found no indication that the protection mechanism is different in non-main threads and tests work as expected.

I think this is ready for merge, except: it looks like @oandrieu needs to sign a CLA first.

I tend to agree with @dra27 on the #define business. We need two pieces of information:

• the test suite needs to know whether stack overflow is detected, whatever the platform
• signals_asm.c needs to know whether unix-style stack overflow detection is available.

Since signals_asm.c already has some #ifdef _WIN32 I don't think it's a big deal to add a condition on the #ifdef HAS_STACK_OVERFLOW_DETECTION. I'm not quite sure how that helps with the test suite, though.

@oandrieu for the CLA, please refer to https://github.com/ocaml/ocaml/blob/trunk/CONTRIBUTING.md#contributor-license-agreement

@damiendoligez : @oandrieu sent me a signed CLA on March 15th, so everything is OK on the legal front. (My CLA notes that you might have consulted are not up to date, sorry about that, will update today.)

@damiendoligez @dra27 Could you please come to a clear statement as to what you would like to see changed, if anything, in this pull request before it is merged?

@oandrieu Please rebase to resolve conflicts.

@mshinwell done — not sure what the AppVeyor failure is about though.

@oandrieu I think if you rebase again now it will be ok.

About HAS_STACK_OVERFLOW_DETECTION, the code will probably have to be changed anyway when we get autoconf working on Windows, so I'm in favor of merging as-is.

OK, I'm going to merge this then; I think there's value in having this in sooner rather than later so as to get more testing. We can always tweak it at a later date.

The MSVC32 port does not compile for the current trunk.
Bisecting shows that the first bad commmit is
eb00976, which is part of this PR:

boot/ocamlrun ./ocamlopt -g -nostdlib -I stdlib -I otherlibs/dynlink -o ocamlc.opt
compilerlibs/ocamlcommon.cmxa compilerlibs/ocamlbytecomp.cmxa driver/main.cmx -cclib "advapi32.lib ws2_32.lib"
** Cannot resolve symbols for stdlib\libasmrun.lib(win32.obj):
_caml_system__code_begin
_caml_system__code_end
File "caml_startup", line 1:
Error: Error during linking
Makefile:862: recipe for target 'ocamlc.opt' failed

@oandrieu: can you see where the porblem comes from?

Are you able to submit a fix for it?

If required I'll be happy to help out with testing on the CI machines.

Thanks!

@shindere ah crap, sorry.
Yes, a fix is easy enough: oandrieu/ocaml@4ecd375
Should I submit a new PR ? or is there a way to amend/extend this one ?

@shindere OK, thanks. I've updated the commit message and rebased on trunk here: oandrieu/ocaml@1a111e1 . It would be great if you could arrange for CI to run on win32,win64 × msvc,mingw .