fix #10343

[timotheecour]

fix #10343
the test nim cpp -r tests/exception/t9657 now works for nim cpp and this PR re-enables it for nim cpp, see below for gory details of why it now works

in reply to #10343 (comment)

on posix it's easy to reproduce, and I have a pending branch to fix it (no PR yet)

ping @timotheecour for this PR.

@Araq it's a little bit complicated so please read carefully:

• after git bisect the underlying issue has been "accidentally" fixed in system refactorings #10559

• that PR is large; after investigating the relevant fix is cstderr.write => cstderr.rawWrite, indeed, after that PR we can re-introduce the bug as follows:

diff --git a/lib/system.nim b/lib/system.nim
index 7febde127..8db64dc8f 100644
--- a/lib/system.nim
+++ b/lib/system.nim
@@ -4171,6 +4171,7 @@ when defined(cpp) and appType != "lib" and
     hostOS != "standalone" and not defined(noCppExceptions):
   proc setTerminate(handler: proc() {.noconv.})
     {.importc: "std::set_terminate", header: "<exception>".}
+  proc write_D20200623T232814(s: string) {.importc.}
   setTerminate proc() {.noconv.} =
     # Remove ourself as a handler, reinstalling the default handler.
     setTerminate(nil)
@@ -4182,8 +4183,8 @@ when defined(cpp) and appType != "lib" and
       echo trace & "Error: unhandled exception: " & ex.msg &
                    " [" & $ex.name & "]\n"
     else:
-      cstderr.rawWrite trace & "Error: unhandled exception: " & ex.msg &
-                   " [" & $ex.name & "]\n"
+      write_D20200623T232814 trace & "Error: unhandled exception: " & ex.msg &
+                    " [" & $ex.name & "]\n"
     quit 1

 when not defined(js):
diff --git a/lib/system/io.nim b/lib/system/io.nim
index 408631db5..31e2e4f9c 100644
--- a/lib/system/io.nim
+++ b/lib/system/io.nim
@@ -189,6 +189,10 @@ proc writeChars*(f: File, a: openArray[char], start, len: Natural): int {.
 proc write*(f: File, s: string) {.tags: [WriteIOEffect], benign.} =
   if writeBuffer(f, cstring(s), s.len) != s.len:
     raiseEIO("cannot write string to file")
+
+when not defined(nimscript):
+  {.emit:"NIM_EXTERNC".}
+  proc write_D20200623T232814(s: string) {.exportc.} = write(stderr, s)
 {.pop.}

 when NoFakeVars:

• git bisect required skipping 1 commit (git bisect skip) 035134d because nim was not buildable in that commit (using either $nim_0196_X or bin/nim_csources)
  => that's just one of the reasons why we should ensure each commit should be buildable

here i ran the "run" part manually in git bisect inner loop because the test involves a hang; but more careful scripting would allow taking care of hangs (which is why it'd be nice to have a nimbisect tool taking care of all those difficulties; there's a small, finite number of them)

• {.emit:"NIM_EXTERNC".} was needed here because that commit happened before i fixed exportc to honor externc

• the "accidental" (or not...) fix makes sense: it uses rawWrite instead of write to avoid raising (raiseEIO) inside set_terminate handler

that's not the end of the story!

if i use a similar patch in git head to try to reproduce that bug in git head, it won't hang anymore, but will instead exit (with code !=0); after investigation, what happens is this:

• {.emit: "throw e;".} after __terminate calls abort_message => abort which sends a SIGABRT signal, which (because of registerSignalHandler) which calls signalHandler => rawWriteStackTrace => auxWriteStackTrace => addFrameEntry(s, tempFrames[j]) => add(s, f.filename)

prior to #12806 we had:

elif hasAlloc:
  {.push stack_trace:off, profiler:off.}
  proc add*(x: var string, y: cstring) =
    var i = 0
    while y[i] != '\0':
      add(x, y[i])
      inc(i)
  {.pop.}

after instrumentation, turns out y == nil right before then hang, for which y[i] caused a EXC_BAD_ACCESS (on OSX at least)

the PR I had

the PR I had back when I wrote #10343 (comment) was a different approach, based on checking whether a CFile is valid using fcntl:

{.emit:"""
NIM_EXTERNC int fd_is_valid(int fd) {
 return fcntl(fd, F_GETFD) != -1 || errno != EBADF;
}
""".}

I rebased it in timotheecour#324; even if not relevant anymore I think fd_is_valid is still useful, as a way to check whether a CFile is valid or not; not that c_feof does not work for that.

summary

• extended stacktrace for templates (treating templates as functions) would've helped diagnose this issue (refs: nkComesFrom seems deadcode (but used a lot); ok to remove? compilerdev#11 (comment)) since the stacktrace was pointing to an irrelevant frame in this case

• fix db_mysql getRow() when column is null error raised #12806 fixed 1 part of this issue (invalid nil dereference in add)

• system refactorings #10559 fixed another part of this issue (rawWrite vs write that throws)

• fd_is_valid (from old superseded fix for #10343 D20200623T225224 timotheecour/Nim#324) is useful and should be added somewhere