nimRawSetJmp default on Win: light refactoring [backport]

[stefantalpalaru]

(small refactor of #19891)

Default Windows setting moved to isDefined() in "compiler/options.nim" because that's where it's already enabled by default for BSDs.

Documentation updated and a comment moved closer to the relevant code.

[cheatfate]

One more note, nimRawSetJmp must be set only for gcc on Windows, and should not be set for any other compilers.

[stefantalpalaru]

nimRawSetJmp must be set only for gcc on Windows

Won't hurt other compilers. It will actually improve stack unwinding performance (and implicitly exception raising) by a factor of 6: #19197

[stefantalpalaru]

CI failure seems to be Azure's fault:

"2022-06-24T11:39:33.5984627Z d:/a/1/s/dist/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: cannot open output file D:\a\1\s\tests\gc\thavlak.exe: Permission denied"

[ringabout]

CI failure seems to be Azure's fault:

Positive. I will rerun the failed CI after other CIs are settled.

[arnetheduck]

in general, nimRawSetJmp fails when compiling with vcc:

https://github.com/status-im/nim-stew/runs/6961489500?check_suite_focus=true

also, @if vcc is broken in .cfg files

[Menduist]

RawSetJmp for VCC has been fixed here: #19899

[cheatfate]

nimRawSetJmp must be set only for gcc on Windows

Won't hurt other compilers. It will actually improve stack unwinding performance (and implicitly exception raising) by a factor of 6: #19197

This option has only meaning for gcc, all other compilers could not have such builtin (and in such case it will fail to compile because of invalid arguments error), or have different name for this builtin (in such case it will fail too).

[stefantalpalaru]

This option has only meaning for gcc, all other compilers could not have such builtin

You're confusing again "__builtin_setjmp()", which is a compiler builtin, with "_setjmp()" which is a library function. We're talking about the latter here.

[arnetheduck]

You're confusing again "__builtin_setjmp()", which is a compiler builtin, with "_setjmp()" which is a library function. We're talking about the latter here.

@stefantalpalaru _setjmp has a different signature on visual studio, see the linked build (C:\Users\runneradmin\nimcache\all_tests_d\@m..@snim@slib@sstd@ssyncio.nim.c(756): error C2167: '_setjmp': too many actual parameters for intrinsic function)

[stefantalpalaru]

_setjmp has a different signature on visual studio

Yes, I saw that. No, it's not a problem, since VCC is the only compiler that can be trusted to correctly build the undocumented second buffer for SEH.

The whole point of "nimRawSetJmp", on Windows, is to pass NULL instead of a pointer to that second buffer. If VCC won't let us do that, we only lose the performance advantage, without losing any correctness.

If an actual VCC user wants to try and get back that exception raising performance, he should start by benchmarking "_setjmpex(jmpbuf, NULL)" which might do the trick, but that's obviously out of scope for this trivial PR.

[ringabout]

related:

Compilation with vcc or clang (msvc) on Windows fails by using nimRawSetjmp

ref #19957

See also #19959

[exelotl]

Since Nim 1.6.8 (presumably caused by #19959), cross compiling on Windows to a non-Windows platform causes linker errors due to _setjmp and _longjmp not existing.

Compiler options:

--cc:gcc
--cpu:arm
--gc:arc
--os:any
--exceptions:setjmp
-d:useMalloc
-d:noSignalHandler

Output:

c:/program files (x86)/gnu arm embedded toolchain/10 2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld.exe: C:/dev/goodboy-galaxy/nimcache/@m..@s..@s..@sUsers@sbonni@sscoop@sapps@snim@scurrent@slib@ssystem.nim.c.o: in function `nimLeaveFinally':
C:\dev\goodboy-galaxy\nimcache/@m..@s..@s..@sUsers@sbonni@sscoop@sapps@snim@scurrent@slib@ssystem.nim.c:639: undefined reference to `_longjmp'    
c:/program files (x86)/gnu arm embedded toolchain/10 2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld.exe: C:/dev/goodboy-galaxy/nimcache/@m..@s..@s..@sUsers@sbonni@sscoop@sapps@snim@scurrent@slib@ssystem.nim.c.o: in function `raiseExceptionAux__system_3479':
C:\dev\goodboy-galaxy\nimcache/@m..@s..@s..@sUsers@sbonni@sscoop@sapps@snim@scurrent@slib@ssystem.nim.c:797: undefined reference to `_longjmp'    
c:/program files (x86)/gnu arm embedded toolchain/10 2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld.exe: C:\dev\goodboy-galaxy\nimcache/@m..@s..@s..@sUsers@sbonni@sscoop@sapps@snim@scurrent@slib@ssystem.nim.c:797: undefined reference to `_longjmp'
c:/program files (x86)/gnu arm embedded toolchain/10 2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld.exe: C:/dev/goodboy-galaxy/nimcache/@mtoasts.nim.c.o: in function `showToast__toasts_85':
C:\dev\goodboy-galaxy\nimcache/@mtoasts.nim.c:218: undefined reference to `_setjmp'
c:/program files (x86)/gnu arm embedded toolchain/10 2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld.exe: C:/dev/goodboy-galaxy/nimcache/@mactors@schargeshot.nim.c.o: in function `checkShrinkPlayer__actorsZchargeshot_1411':
C:\dev\goodboy-galaxy\nimcache/@mactors@schargeshot.nim.c:3263: undefined reference to `_setjmp'
c:/program files (x86)/gnu arm embedded toolchain/10 2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld.exe: C:/dev/goodboy-galaxy/nimcache/@msystems@soverlay@sdialogue.nim.c.o: in function `update__systemsZoverlayZdialogue_238':
C:\dev\goodboy-galaxy\nimcache/@msystems@soverlay@sdialogue.nim.c:942: undefined reference to `_setjmp'
c:/program files (x86)/gnu arm embedded toolchain/10 2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld.exe: C:/dev/goodboy-galaxy/nimcache/@mscenes@spda@swidget_timer.nim.c.o: in function `eqdestroy___scenesZpdaZwidget95timer_172':
C:\dev\goodboy-galaxy\nimcache/@mscenes@spda@swidget_timer.nim.c:196: undefined reference to `_setjmp'
c:/program files (x86)/gnu arm embedded toolchain/10 2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld.exe: C:/dev/goodboy-galaxy/nimcache/@mscenes@snavigator.nim.c.o: in function `Navigator_update':
C:\dev\goodboy-galaxy\nimcache/@mscenes@snavigator.nim.c:1725: undefined reference to `_setjmp'
c:/program files (x86)/gnu arm embedded toolchain/10 2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld.exe: C:/dev/goodboy-galaxy/nimcache/@mactors@sbridge.nim.c.o:C:\dev\goodboy-galaxy\nimcache/@mactors@sbridge.nim.c:1001: more undefined references to `_setjmp' follow
collect2.exe: error: ld returned 1 exit status
Error: execution of an external program failed: '"C:\Program Files (x86)\GNU Arm Embedded Toolchain\10 2021.10\bin\arm-none-eabi-gcc.exe" @main_linkerArgs.txt'
stack trace: (most recent call last)
C:\Users\Me\scoop\apps\nim\current\lib\system\nimscript.nim(429, 18)
C:\dev\goodboy-galaxy\config.nims(93, 10) buildTask
C:\dev\goodboy-galaxy\config.nims(87, 12) romTask
C:\Users\Me\scoop\apps\nim\current\lib\system\nimscript.nim(293, 7) selfExec
C:\Users\Me\scoop\apps\nim\current\lib\system\nimscript.nim(293, 7) Error: unhandled exception: FAILED: C:\Users\Me\scoop\apps\nim\current\bin\nim.exe c -d:scene=Zandunia -o:goodboy.elf source/main.nim [OSError]

Is this considered a bug, or should I be explicitly choosing the setjmp implementation with -d:nimStdSetjmp ?

[stefantalpalaru]

Please read this abandoned PR's diff. It's the wrong one for such a complaint.

[exelotl]

Apologies, I asked here as it seemed like this was an open follow-up PR so it would be the appropriate place to add the fix. I didn't know it was abandoned. I'll make an issue.

[Araq]

@exelotl --gc:arc with --exceptions:setjmp produces terrible code and you should use --exceptions:goto instead.

[exelotl]

@Araq this hasn't been the case for me - I don't actually use or want any exceptions in my code at all (I just need my program to hang when an assertion or check fails so I can connect a debugger to it) but there's no way to outright disable them. --panics:on has no noticeable effect on codegen, --os:standalone is discouraged and comes with other limitations. What I ended up doing is to patch sysFatal to make it panic instead of raise, which works great.

Anyways, under --exceptions:goto the generated C code is extremely hard to read & debug because every other line is if (NIM_UNLIKELY(*nimErr_)) goto BeforeRet_;. Most procedures also contain a call to nimErrorFlag(). Maybe the C compiler is optimising most of this away, but overall there is a slight increase in executable size and slight decrease in performance.

Meanwhile --exceptions:setjmp produces 8 calls to setjmp in my whole codebase. They seem to be created around a small fraction of checks and assertions. I don't understand why, as the vast majority of checks & assertions don't have them (presumably due to patching sysFatal to not raise). In theory I could stub out setjmp and make longjmp = panic, to eliminate these unusual cases.

I don't really know what to do about all this though:

• Are those remaining calls to setjmp actually the compiler misbehaving? Nothing in my program can possibly raise. Should I make an issue?
• There is no "--exceptions:off", and --panics:on doesn't seem to do anything, or I'm misunderstanding it. Should I make an issue?
• Is my original problem (cross compiling with --exceptions:setjmp doesn't work out of the box on Windows) an actual issue or are users expected to configure it with -d:nimStdSetjmp?

Thanks

[Araq]

You want --exceptions:quirky then, I think. (Which will always be supported.)

[github-actions]

This pull request is stale because it has been open for 1 year with no activity. Contribute more commits on the pull request and rebase it on the latest devel, or it will be closed in 30 days. Thank you for your contributions.

[github-actions]

This pull request has been marked as stale and closed due to inactivity after 395 days.