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

Block rewriting error when there are no explicit break #76

Closed
alaviss opened this issue Apr 29, 2021 · 2 comments · Fixed by #85
Closed

Block rewriting error when there are no explicit break #76

alaviss opened this issue Apr 29, 2021 · 2 comments · Fixed by #85
Labels
bug Something isn't working

Comments

@alaviss
Copy link
Contributor

alaviss commented Apr 29, 2021

Found while messing with "shadow mission impossible"

import cps

type
  Cont* = ref object of RootObj
    fn*: proc(c: Cont): Cont {.nimcall.}

proc trampoline(c: Cont) =
  var c = c
  while c != nil and c.fn != nil:
    c = c.fn(c)

proc noop*(c: Cont): Cont {.cpsMagic.} = c

template doAssert(b: bool, msg: string) =
  if not b:
    raise newException(AssertionDefect, msg)

var r = 0
proc b() {.cps: Cont.} =
  inc r
  block:
    noop()
    echo "in block"
    inc r
  inc r
  echo "out block"

trampoline b()
doAssert r == 3, "expected 3, got: " & $r

Output:

=== .cps. on b(original)  === temp.nim(19, 0)
 19  proc b() =
 20    inc r, 1
 21    block:
 22      noop()
 23      echo ["in block"]
 24      inc r, 1
 25    inc r, 1
 26    echo ["out block"]
 27
storing type env_385876243
next type env_385876522
=== .cps. on b(transform) === temp.nim(19, 0)
 19
 20  type
 21    env_385876243 = ref object of Cont
 22
 23  proc blockBreak_385876482(continuation: Cont): Cont {.cpsLift.}
 24  proc afterCall_385876492(continuation: Cont): Cont {.cpsLift.}
 25  proc blockBreak_385876482(continuation: Cont): Cont {.cpsLift.} =
 26    ## saften at 21 of temp.nim
 27    inc r, 1
 28    echo "out block"
 29
 30  proc afterCall_385876492(continuation: Cont): Cont {.cpsLift.} =
 31    ## saften at 848 of cps.nim
 32    echo "in block"
 33    inc r, 1
 34
 35  proc b(continuation: Cont): Cont {.cpsCall.} =
 36    ## saften at 19 of temp.nim
 37    ## saften at 20 of temp.nim
 38    inc r, 1
 39    block:
 40      ## saften at 22 of temp.nim
 41      ## re-use the local continuation by setting the fn
 42      continuation.fn = afterCall_385876492
 43      return noop(continuation)
 44    ## add tail call for block-break proc
 45    ## verbatim tail call
 46    ## new tail call: blockBreak_385876482
 47    ## re-use the local continuation by setting the fn
 48    continuation.fn = blockBreak_385876482
 49    return continuation
 50    ## new tail call: blockBreak_385876482
 51    ## re-use the local continuation by setting the fn
 52    continuation.fn = blockBreak_385876482
 53    return continuation
 54
 55  proc b(): Cont =
 56    result = env_385876243(fn: b)
 57
Hint:  [Link]
Hint: 53164 lines; 1.007s; 76.262MiB peakmem; Debug build; proj: temp; out: temp [SuccessX]
Hint: temp  [Exec]
in block
temp.nim(16) temp
Error: unhandled exception: expected 3, got: 2 [AssertionDefect]
@alaviss alaviss added the bug Something isn't working label Apr 29, 2021
@alaviss
Copy link
Contributor Author

alaviss commented Apr 29, 2021

Similar bug for if statement:

var r = 0
proc b() {.cps: Cont.} =
  if true:
    noop()
    inc r
  inc r

trampoline b()
check r == 2, "expected 2, got: " & $r

The issue in here is that saften expects control flow continuation to happen after the scoped block:

https://github.com/disruptek/cps/blob/0ba9b8bbfa024464caba663399f55897966bea3a/cps.nim#L579-L597

The idea doesn't hold when splitting is performed.

@disruptek
Copy link
Contributor

We have similar behavior in several places and the "solution" we use thus far -- staggered inclusion in the stack -- is a hack that seems unlikely to weather extension.

We should probably just enumerate the different modes and add proper support into Scope.

But we should talk about whether we even want saften to work this way at all; it's vestigial at this point. I think it might make more sense (read: easier to test and debug) to process the input in passes instead of mixing these stages all at once.

However, it's not obvious: if we switch to stages, we may be moving a lot of the complexity into the type construction. Maybe those artifacts are simply accumulated across passes, I dunno.

alaviss added a commit to alaviss/cps that referenced this issue May 3, 2021
For-loops are inlined using block statements, which messes with their
control flow due to nim-works#76.

Breaks "for loop with continue, break" test. On the flip side,
splitting now functions on them.

Also breaks a test in tzevv due to defer being rewritten into
try-finally, which doesn't work.
alaviss added a commit to alaviss/cps that referenced this issue May 3, 2021
For-loops are inlined using block statements, which messes with their
control flow due to nim-works#76.

Breaks "for loop with continue, break" test. On the flip side,
splitting now functions on them.

Also breaks a test in tzevv due to defer being rewritten into
try-finally, which doesn't work.

Co-authored-by: Andy Davidoff <github@andy.disruptek.com>
@disruptek disruptek linked a pull request May 10, 2021 that will close this issue
alaviss added a commit to alaviss/cps that referenced this issue May 14, 2021
For-loops are inlined using block statements, which messes with their
control flow due to nim-works#76.

Breaks a test in tzevv due to defer being rewritten into
try-finally, which doesn't work.

Co-authored-by: Andy Davidoff <github@andy.disruptek.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants