From 1cceff66db9eb0bc9e775b4a2c9bbd4a89254ae4 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Fri, 27 Nov 2020 20:26:06 -0800 Subject: [PATCH 1/6] fix partially #13115 properly (works for c,js,cpp,vm; still fails for js on openbsd) --- lib/system/excpt.nim | 32 ++++++++++++++++++++++---------- tests/exception/t13115.nim | 10 ++++++---- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/lib/system/excpt.nim b/lib/system/excpt.nim index f73a88e306ac5..aec25e3f3e989 100644 --- a/lib/system/excpt.nim +++ b/lib/system/excpt.nim @@ -13,6 +13,8 @@ import std/private/miscdollars import stacktraces +const noStacktraceAvailable = "No stack traceback available\n" + var errorMessageWriter*: (proc(msg: string) {.tags: [WriteIOEffect], benign, nimcall.}) @@ -36,6 +38,10 @@ else: proc writeToStdErr(msg: cstring, length: int) = discard MessageBoxA(nil, msg, nil, 0) +proc writeToStdErr(msg: string) {.inline.} = + # fix bug #13115: handles correctly '\0' unlike default implicit conversion to cstring + writeToStdErr(msg.cstring, msg.len) + proc showErrorMessage(data: cstring, length: int) {.gcsafe, raises: [].} = var toWrite = true if errorMessageWriter != nil: @@ -51,6 +57,9 @@ proc showErrorMessage(data: cstring, length: int) {.gcsafe, raises: [].} = else: writeToStdErr(data, length) +proc showErrorMessage2(data: string) {.inline.} = + showErrorMessage(data.cstring, data.len) + proc chckIndx(i, a, b: int): int {.inline, compilerproc, benign.} proc chckRange(i, a, b: int): int {.inline, compilerproc, benign.} proc chckRangeF(x, a, b: float): float {.inline, compilerproc, benign.} @@ -123,11 +132,11 @@ proc popSafePoint {.compilerRtl, inl.} = proc pushCurrentException(e: sink(ref Exception)) {.compilerRtl, inl.} = e.up = currException currException = e - #showErrorMessage "A" + #showErrorMessage2 "A" proc popCurrentException {.compilerRtl, inl.} = currException = currException.up - #showErrorMessage "B" + #showErrorMessage2 "B" proc popCurrentExceptionEx(id: uint) {.compilerRtl.} = discard "only for bootstrapping compatbility" @@ -305,7 +314,7 @@ when hasSomeStackTrace: auxWriteStackTraceWithOverride(s) elif NimStackTrace: if framePtr == nil: - add(s, "No stack traceback available\n") + add(s, noStacktraceAvailable) else: add(s, "Traceback (most recent call last)\n") auxWriteStackTrace(framePtr, s) @@ -313,7 +322,7 @@ when hasSomeStackTrace: add(s, "Traceback from system (most recent call last)\n") auxWriteStackTraceWithBacktrace(s) else: - add(s, "No stack traceback available\n") + add(s, noStacktraceAvailable) proc rawWriteStackTrace(s: var seq[StackTraceEntry]) = when defined(nimStackTraceOverride): @@ -363,7 +372,7 @@ proc reportUnhandledErrorAux(e: ref Exception) {.nodestroy.} = if onUnhandledException != nil: onUnhandledException(buf) else: - showErrorMessage(buf, buf.len) + showErrorMessage2(buf) `=destroy`(buf) else: # ugly, but avoids heap allocations :-) @@ -504,16 +513,16 @@ proc writeStackTrace() = when hasSomeStackTrace: var s = "" rawWriteStackTrace(s) - cast[proc (s: cstring, length: int) {.noSideEffect, tags: [], nimcall, raises: [].}](showErrorMessage)(s, s.len) else: - cast[proc (s: cstring, length: int) {.noSideEffect, tags: [], nimcall, raises: [].}](showErrorMessage)("No stack traceback available\n", 32) + let s = noStacktraceAvailable + cast[proc (s: string) {.noSideEffect, tags: [], nimcall, raises: [].}](showErrorMessage2)(s) proc getStackTrace(): string = when hasSomeStackTrace: result = "" rawWriteStackTrace(result) else: - result = "No stack traceback available\n" + result = noStacktraceAvailable proc getStackTrace(e: ref Exception): string = if not isNil(e): @@ -543,7 +552,7 @@ proc callDepthLimitReached() {.noinline.} = $nimCallDepthLimit & " function calls). You can change it with " & "-d:nimCallDepthLimit= but really try to avoid deep " & "recursions instead.\n" - showErrorMessage(msg, msg.len) + showErrorMessage2(msg) quit(1) proc nimFrame(s: PFrame) {.compilerRtl, inl, raises: [].} = @@ -627,13 +636,16 @@ when not defined(noSignalHandler) and not defined(useNimRtl): var buf = newStringOfCap(2000) rawWriteStackTrace(buf) processSignal(sign, buf.add) # nice hu? currying a la Nim :-) - showErrorMessage(buf, buf.len) + showErrorMessage2(buf) when not usesDestructors: GC_enable() else: var msg: cstring template asgn(y) = msg = y processSignal(sign, asgn) + # xxx use string for msg instead of cstring, and here use showErrorMessage2(msg) + # unless there's a good reason to use cstring in signal handler to avoid + # using gc? showErrorMessage(msg, msg.len) quit(1) # always quit when SIGABRT diff --git a/tests/exception/t13115.nim b/tests/exception/t13115.nim index 53e078076a252..1ffd9617b2dca 100644 --- a/tests/exception/t13115.nim +++ b/tests/exception/t13115.nim @@ -1,12 +1,14 @@ discard """ exitcode: 1 - targets: "c" + targets: "c js cpp" matrix: "-d:debug; -d:release" outputsub: ''' and works fine! [Exception]''' """ # bug #13115 -# xxx bug: doesn't yet work for cpp -var msg = "This char is `" & '\0' & "` and works fine!" -raise newException(Exception, msg) +template fn = + var msg = "This char is `" & '\0' & "` and works fine!" + raise newException(Exception, msg) +# static: fn() # would also work +fn() From 0765ed7172a8d47fcc30369150f3260e0350da86 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Fri, 27 Nov 2020 21:19:03 -0800 Subject: [PATCH 2/6] disbled: openbsd (fails for js) --- tests/exception/t13115.nim | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/exception/t13115.nim b/tests/exception/t13115.nim index 1ffd9617b2dca..3fd61b20a8a5f 100644 --- a/tests/exception/t13115.nim +++ b/tests/exception/t13115.nim @@ -3,7 +3,11 @@ discard """ targets: "c js cpp" matrix: "-d:debug; -d:release" outputsub: ''' and works fine! [Exception]''' + disabled: openbsd """ +#[ +disabled: openbsd: just for js +]# # bug #13115 From f623ea1de191938024389dceb0a76bf6305b2795 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Thu, 3 Dec 2020 11:06:19 -0800 Subject: [PATCH 3/6] improve test coverage --- tests/exception/t13115.nim | 39 +++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/tests/exception/t13115.nim b/tests/exception/t13115.nim index 3fd61b20a8a5f..cd1d3158bc621 100644 --- a/tests/exception/t13115.nim +++ b/tests/exception/t13115.nim @@ -1,18 +1,23 @@ -discard """ - exitcode: 1 - targets: "c js cpp" - matrix: "-d:debug; -d:release" - outputsub: ''' and works fine! [Exception]''' - disabled: openbsd -""" -#[ -disabled: openbsd: just for js -]# +const msg = "This char is `" & '\0' & "` and works fine!" -# bug #13115 - -template fn = - var msg = "This char is `" & '\0' & "` and works fine!" - raise newException(Exception, msg) -# static: fn() # would also work -fn() +when defined nim_t13115: + # bug #13115 + template fn = + raise newException(Exception, msg) + when defined nim_t13115_static: + static: fn() + fn() +else: + import std/[osproc,strformat,os,strutils] + proc main = + const nim = getCurrentCompilerExe() + const file = currentSourcePath + for b in "c js cpp".split: + when defined(openbsd): + if b == "js": continue # xxx bug: pending #13115 + for opt in ["-d:nim_t13115_static", ""]: + let cmd = fmt"{nim} r -b:{b} -d:nim_t13115 {opt} --hints:off {file}" + let (outp, exitCode) = execCmdEx(cmd) + doAssert msg in outp, cmd & "\n" & msg + doAssert exitCode == 1 + main() From 38b8e0e0a4ab0268a926d7f1337e56dd93c75370 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Thu, 3 Dec 2020 17:39:23 -0800 Subject: [PATCH 4/6] fix for windows --- tests/exception/t13115.nim | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/exception/t13115.nim b/tests/exception/t13115.nim index cd1d3158bc621..ce7fadac7e2a7 100644 --- a/tests/exception/t13115.nim +++ b/tests/exception/t13115.nim @@ -18,6 +18,10 @@ else: for opt in ["-d:nim_t13115_static", ""]: let cmd = fmt"{nim} r -b:{b} -d:nim_t13115 {opt} --hints:off {file}" let (outp, exitCode) = execCmdEx(cmd) - doAssert msg in outp, cmd & "\n" & msg + when defined windows: + # `\0` not preserved on windows + doAssert "` and works fine!" in outp, cmd & "\n" & msg + else: + doAssert msg in outp, cmd & "\n" & msg doAssert exitCode == 1 main() From 7f83b11deb061f8fd87844c2f84f5acd5cd5b293 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Thu, 3 Dec 2020 17:43:49 -0800 Subject: [PATCH 5/6] fixup --- tests/exception/t13115.nim | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/exception/t13115.nim b/tests/exception/t13115.nim index ce7fadac7e2a7..c2b71f25518cf 100644 --- a/tests/exception/t13115.nim +++ b/tests/exception/t13115.nim @@ -14,7 +14,11 @@ else: const file = currentSourcePath for b in "c js cpp".split: when defined(openbsd): - if b == "js": continue # xxx bug: pending #13115 + if b == "js": + # xxx bug: pending #13115 + # remove special case once nodejs updated >= 12.16.2 + # refs https://github.com/nim-lang/Nim/pull/16167#issuecomment-738270751 + continue for opt in ["-d:nim_t13115_static", ""]: let cmd = fmt"{nim} r -b:{b} -d:nim_t13115 {opt} --hints:off {file}" let (outp, exitCode) = execCmdEx(cmd) From d394231df1d3f3a8dc8e322d8a6aa2b2e7470317 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Fri, 4 Dec 2020 14:33:12 -0800 Subject: [PATCH 6/6] address comment: also test with -d:danger, -d:debug --- tests/exception/t13115.nim | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/exception/t13115.nim b/tests/exception/t13115.nim index c2b71f25518cf..ee1daed26800c 100644 --- a/tests/exception/t13115.nim +++ b/tests/exception/t13115.nim @@ -19,7 +19,14 @@ else: # remove special case once nodejs updated >= 12.16.2 # refs https://github.com/nim-lang/Nim/pull/16167#issuecomment-738270751 continue - for opt in ["-d:nim_t13115_static", ""]: + + # save CI time by avoiding mostly redundant combinations as far as this bug is concerned + var opts = case b + of "c": @["", "-d:nim_t13115_static", "-d:danger", "-d:debug"] + of "js": @["", "-d:nim_t13115_static"] + else: @[""] + + for opt in opts: let cmd = fmt"{nim} r -b:{b} -d:nim_t13115 {opt} --hints:off {file}" let (outp, exitCode) = execCmdEx(cmd) when defined windows: