Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix partially #13115 (now works for cpp; but still fails for js on openbsd) #16167

Merged
merged 6 commits into from
Dec 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions lib/system/excpt.nim
Original file line number Diff line number Diff line change
Expand Up @@ -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.})
Expand All @@ -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:
Expand All @@ -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.}
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -305,15 +314,15 @@ 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)
elif defined(nativeStackTrace) and nativeStackTraceSupported:
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):
Expand Down Expand Up @@ -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 :-)
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -543,7 +552,7 @@ proc callDepthLimitReached() {.noinline.} =
$nimCallDepthLimit & " function calls). You can change it with " &
"-d:nimCallDepthLimit=<int> but really try to avoid deep " &
"recursions instead.\n"
showErrorMessage(msg, msg.len)
showErrorMessage2(msg)
quit(1)

proc nimFrame(s: PFrame) {.compilerRtl, inl, raises: [].} =
Expand Down Expand Up @@ -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

Expand Down
46 changes: 36 additions & 10 deletions tests/exception/t13115.nim
Original file line number Diff line number Diff line change
@@ -1,12 +1,38 @@
discard """
exitcode: 1
targets: "c"
matrix: "-d:debug; -d:release"
outputsub: ''' and works fine! [Exception]'''
"""
const msg = "This char is `" & '\0' & "` and works fine!"

# bug #13115
# xxx bug: doesn't yet work for cpp
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":
# 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

var msg = "This char is `" & '\0' & "` and works fine!"
raise newException(Exception, msg)
# 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}"
timotheecour marked this conversation as resolved.
Show resolved Hide resolved
let (outp, exitCode) = execCmdEx(cmd)
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()
Copy link
Contributor

@Clyybber Clyybber Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please get rid of this abomination and use testament to accomplish what you want.

Copy link
Member Author

@timotheecour timotheecour Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testament spec DSL isn't flexible enough for this test, sorry. If you disagree, please show a complete example that tests VM, openbsd (for non-js case), the '\0' being part of the output message, allows avoiding combinatorial explosion (as i do in this PR) etc, while also avoiding generating N different test files (not DRY nor desirable).

testament spec may be good for the general case, but you'll always have special cases, which need things like: