system/excpt: check if the exception is not nil before pop

[alaviss]

In CPS we would consume an exception in the except branch by stashing it
into a local then remove the exception from Nim environment so as not to
leak it to other code that would be running before the continuation
continues

However since popCurrentException() assumes that the exception always
exist, removing the exception from an except branch will cause a
SIGSEGV to happen. This commit fixes that.

[Araq]

You pop the entire stack via setCurrentException(nil), not just a single entry. Is that really what you want?

[alaviss]

Yes, since we control that stack in CPS.

[timotheecour]

I don't think setCurrentException(nil) should be allowed, it breaks assumptions; as @Araq mentioned the entire stack is popped, which isn't reflected in the name setCurrentException; likewise popCurrentException is now also misleading (it could pop 1 or 0 instead of 1)

for eg, if I wrap tests/exception/tsetexceptions.nim inside a proc main (not far from what megatest does), it results in SIGSEGV in below:

when true:
  proc main =
    # code from tests/exception/tsetexceptions.nim:
    let ex = newException(CatchableError, "test")
    setCurrentException(ex)
    doAssert getCurrentException().msg == ex.msg
    doAssert getCurrentExceptionMsg() == ex.msg
    setCurrentException(nil)
    try:
      raise newException(CatchableError, "test2")
    except:
      setCurrentException(nil)
    doAssert getCurrentException() == nil

  try:
    raise newException(CatchableError, "test3")
  except:
    try:
      main()
    except Exception as e:
      echo "caught main: ", e.msg
    echo "caught top"
    let e2 = getCurrentException()
    echo e2.msg # SIGSEGV

yes, there is a Warning: Only use this if you know what you are doing. in its doc comments, but what's the spec there?

Yes, since we control that stack in CPS.

can you give more details? And doesn't that prevent using this as library code?

instead of setCurrentException(nil), can't we use a Dummy exception type that CPS is made aware of, so that popCurrentException and setCurrentException(nil) can remain sane ?

[alaviss]

I don't think setCurrentException(nil) should be allowed, it breaks assumptions

It should still be allowed, just with a warning in the docs regarding its influence on an exception handler.

can you give more details?

In CPS, an exception handler might not be run in one go (ie. they perform a yield in the middle), thus we have to store and control what the "current" exception is in the view of the handler so they can resume their work.

Imagine:

proc foo() {.cps: Continuation.} =
  try:
    raise newException(CatchableError, "test")
  except CatchableError:
    pause() # pause the continuation until resumed
    doAssert getCurrentExceptionMsg() == "test"

To scope the exception to the handler in the continuation, CPS set the global "current" exception to what is captured at the site of except on resume and remove it (via setCurrentException(nil)) once the handler finishes or pauses so that operations outside of CPS don't get access to the exception object inside CPS.

Thanks to your sample, I found that there is a bug in CPS' exception implementation: the removal of the global current exception would cause outside code that was handling an exception to no longer have it.

[Araq]

instead of setCurrentException(nil), can't we use a Dummy exception type that CPS is made aware of, so that popCurrentException and setCurrentException(nil) can remain sane ?

Yeah, that seems a much better solution.