SIGSEGV in closure iterator with try/except not at top level

[iffy]

Description

UPDATED: The following code produces a SIGSEGV:

try:
  proc myFunc() =
    iterator myFuncIter(): int {.closure.} =
      if false:
        try:
          yield 5
        except:
          discard
    var nameIterVar = myFuncIter
    discard nameIterVar()
  try:
    raise ValueError.newException("foo")
  finally:
    myFunc()
except ValueError as exc:
  if exc.msg == "foo":
    echo "21235 ok"
  else:
    echo "21235 fail"

I now believe it happens because the try/except block is not the top block in the closure iterator.

---

Original report:

import std/asyncdispatch

proc myFunc*() {.async.} =
  if false:
    try:
      await newFuture[void]("Thing.myFunc")
    except:
      discard

try:
  raise ValueError.newException("foo")
finally:
  waitFor myFunc()

Traceback (most recent call last)
/Users/matt/tmp/t21235.nim(13) t21235
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

On my macOS machine, this minimal code produces a SIGSEGV.

import chronos

type
  Obj = ref object
    queue: AsyncQueue[string]

proc newQueue(o: Obj): AsyncQueue[string] =
  o.queue = newAsyncQueue[string]()
  return o.queue

block:
  var o = Obj()
  var oq = o.newQueue()
  defer: waitFor o.queue.addLast("foo")
  raise ValueError.newException("foo")

Taking any line out prevents the segfault.

Nim Version

Nim Compiler Version 1.6.10 [MacOSX: amd64]
Compiled at 2022-11-29
Copyright (c) 2006-2021 by Andreas Rumpf

active boot switches: -d:release

Current Output

Traceback (most recent call last)
/tmp/test_tmp.nim(14) test_tmp
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

Expected Output

/tmp/test_tmp.nim(15) test_tmp
Error: unhandled exception: foo [ValueError]

Possible Solution

No response

Additional Information

If it's helpful, here's a portion of the core dump:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   test_tmp                      	0x0000000108c0af3e reportUnhandledErrorAux__system_3392 + 30
1   test_tmp                      	0x0000000108c0b2d5 reportUnhandledError__system_3409 + 53
2   test_tmp                      	0x0000000108c103fd nimLeaveFinally + 77
3   test_tmp                      	0x0000000108c25fbc NimMainModule + 636
4   test_tmp                      	0x0000000108c25d39 NimMainInner + 9
5   test_tmp                      	0x0000000108c2600a NimMain + 42
6   test_tmp                      	0x0000000108c2604e main + 62
7   libdyld.dylib                 	0x00007fff647d03d5 start + 1

Original issue I filed with chronos is here: status-im/nim-chronos#342 which includes this other version of the issue:

import chronos

type
  Obj = ref object
    queue: AsyncQueue[string]

proc newQueue(o: Obj): AsyncQueue[string] =
  o.queue = newAsyncQueue[string]()
  return o.queue

try:
  var o = Obj()
  var oq = o.newQueue()
  defer: waitFor o.queue.addLast("foo")
  raise (ref ValueError)(msg: "test")
except ValueError:
  echo "test"

[iffy]

I've done a little poking.

• I believe it is not a bug with defer. I tested this by writing my own defer macro and it had the same error.
• After adding debugging statements throughout lib/system/excpt.nim I believe it is a bug in https://github.com/nim-lang/Nim/blob/devel/compiler/closureiters.nim#L809

If I can figure out how to debug that code, I will, but if someone knows off-hand what's wrong, I'd gladly defer.

[iffy]

Here's the result of enabling some of the debug statements in closureiters.nim. There's a closureIterSetupExc(nil) call within this that I think might be responsible. But I don't really know what closureIterSetupExc is supposed to do.

TRANSFORM TO STATES: 
while true:
  block :stateLoop:
    try:
      closureIterSetupExc(:envP.`:curExc1`)
      goto :envP.`:state`8
      state 0:
      block :tmp:
        template result(): auto {.used.} =
          {.fatal: "You should not reference the `result` variable inside" &
              " a void async proc".}

        ## Put an ``item`` to the end of the queue ``aq``. If the queue is full,
        ## wait until a free slot is available before adding item.
        block :tmp:
          :envP.`:state` = 1
          break :stateLoop
      state 1:
      if full(aq):
        var putter =
          newFutureImpl(srcLocImpl("AsyncQueue.addLast", "asyncsync.nim", 351))
        add(aq.putters, putter)
        :envP.`:state` = 2
        break :stateLoop
      else:
        :envP.`:state` = 6
        break :stateLoop
      state 2:
      var chronosInternalTmpFuture: FutureBase = FutureBase(putter)
      chronosInternalRetFuture.child = chronosInternalTmpFuture
      :envP.`:state` = 5
      return chronosInternalTmpFuture
      state 3:
      :envP.`:curExc1` = nil
      if of(getCurrentException(), CatchableError):
        let exc = getCurrentException()
        if not full(aq) and not cancelled(FutureBase(putter)):
          wakeupNext(aq.putters)
        raise exc
      else:
        :envP.`:unrollFinally3` = true
        :envP.`:curExc1` = getCurrentException()
        :envP.`:state` = 4
        break :stateLoop
      :envP.`:state` = 4
      break :stateLoop
      state 4:
      if :envP.`:unrollFinally3`:
        if `==`(:envP.`:curExc1`, nil):
          :envP.`:state` = -1
          return result = :envP.`:tmpResult2`
        else:
          closureIterSetupExc(nil)
          raise :envP.`:curExc1`
      :envP.`:state` = 1
      break :stateLoop
      state 5:
      chronosInternalRetFuture.child = nil
      if chronosInternalRetFuture.mustCancel:
        raise (ref CancelledError)(msg: "Future operation cancelled!")
      internalCheckComplete(chronosInternalTmpFuture)
      :envP.`:state` = 4
      break :stateLoop
      state 6:
      addLastNoWait(aq, item)
      :envP.`:state` = 7
      break :stateLoop
      state 7:
      complete(chronosInternalRetFuture, srcLocImpl("", "asyncsync.nim", 348))
      :envP.`:state` = 8
      break :stateLoop
      state 8:
      :envP.`:state` = -1
      break :stateLoop
    except:
      :envP.`:state` = [0, 0, -3, 4, 0, -3, 0, 0, 0][:envP.`:state`]
      if `==`(:envP.`:state`, 0):
        raise
      :envP.`:unrollFinally3` = `<`(0, :envP.`:state`)
      if `<`(:envP.`:state`, 0):
        :envP.`:state` = `-`(:envP.`:state`)
      :envP.`:curExc1` = getCurrentException()
exception table:
0 -> 0
1 -> 0
2 -> -3
3 -> 4
4 -> 0
5 -> -3
6 -> 0
7 -> 0
8 -> 0

[iffy]

Here's an even smaller demonstration:

import chronos

var oq = newAsyncQueue[string]()
try:
  raise ValueError.newException("foo")
finally:
  waitFor oq.addLast("foo")

I now suspect AsyncQueue.addLast. Let me investigate that. I hope no one's getting an email for each of these comments -- if so, let me know and I'll do my debugging more quietly.

[iffy]

Here's an example that doesn't use AsyncQueue or chronos:

import std/asyncdispatch

proc myFunc*() {.async.} =
  if false:
    try:
      await newFuture[void]("Thing.myFunc")
    except:
      discard

try:
  raise ValueError.newException("foo")
finally:
  waitFor myFunc()

Traceback (most recent call last)
/Users/matt/tmp/t21235.nim(13) t21235
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

[iffy]

More minimal, no imports, no macros. I think it has something to do with yielding inside an if branch that is never taken maybe?

proc myFunc() =
  iterator myFuncIter(): int {.closure.} =
    if false:
      try:
        yield 5
      except:
        discard
  var nameIterVar = myFuncIter
  discard nameIterVar()

try:
  raise ValueError.newException("foo")
finally:
  myFunc()

[iffy]

Alright, I need to sleep now, but I think the issue is that when a closure iterator has a try/except within it, the whole closure is wrapped in special try/except code here:

Nim/compiler/closureiters.nim

Line 1283 in b34412f

blockBody = ctx.wrapIntoTryExcept(blockBody)

I think one of these would be a good fix:

1. Make the try/except wrapping more narrow (only wrapping the original actual try/except code)
2. Keep wrapping the whole thing, but instead of calling closureIterSetupExc at the top, wait to call it until the point corresponding to where each try is.
3. Call closureIterSetupExc(nil) at the beginning of any state that happens after a finally block.

I've made a failing test here: iffy@d6dd8f8 If I get around to making a fix, I'll submit both the test and fix as a PR.

[iffy]

For instance, this fixes my issue, but since there's not much test coverage for closure iterators with try/except in tests/iter/tclosureiters.nim, I imagine this change would break a lot of code:

diff --git a/compiler/closureiters.nim b/compiler/closureiters.nim
index 5c048db2b..bc5832143 100644
--- a/compiler/closureiters.nim
+++ b/compiler/closureiters.nim
@@ -1243,11 +1243,11 @@ proc newCatchBody(ctx: var Ctx, info: TLineInfo): PNode {.inline.} =
       ctx.g.callCodegenProc("getCurrentException")))
 
 proc wrapIntoTryExcept(ctx: var Ctx, n: PNode): PNode {.inline.} =
-  let setupExc = newTree(nkCall,
-    newSymNode(ctx.g.getCompilerProc("closureIterSetupExc")),
-    ctx.newCurExcAccess())
+  # let setupExc = newTree(nkCall,
+  #   newSymNode(ctx.g.getCompilerProc("closureIterSetupExc")),
+  #   ctx.newCurExcAccess())
 
-  let tryBody = newTree(nkStmtList, setupExc, n)
+  let tryBody = newTree(nkStmtList, n)
   let exceptBranch = newTree(nkExceptBranch, ctx.newCatchBody(ctx.fn.info))
 
   result = newTree(nkTryStmt, tryBody, exceptBranch)

[iffy]

After looking it over more, I don't think I'm the person to implement this fix :) But hopefully the failing testcases are helpful?