Panics can be caught from TCO loops #9705

Akirathan · 2024-04-15T15:25:51Z

Pull Request Description

In certain cases, when the action of Panic.catch is tail-call-optimized (via @Tail_Call) annotation, the panic is not caught. Fixed by ensuring that the action of Panic.catch is executed as NOT_TAIL rather than TAIL_DIRECT.

Important Notes

The handler parameter of Panic.catch is executed as NOT_TAIL as well, just to be sure.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

Akirathan · 2024-04-25T16:18:07Z

test/Base_Tests/src/Semantic/Error_Spec.enso

+            p . should_equal "fn_with_tco"
+
+        # The same test as the one before, but it does not use the `<|` function.
+        group_builder.specify "should be caught in a TCO loop with a wrapper fn" <|


As suggested by @JaroslavTulach, I have added this test. And as expected, this test succeeds, but the former ("should be caught in a TCO loop") fails. It seems that your suspicion, @JaroslavTulach, was right - this is probably caused by the fact that the <| function is inlineable and therefore there is no frame for it.

@JaroslavTulach EDIT: The test added in cce3c03 suggests otherwise. In that commit, I have introduced a test that doesn't use the <| method, but uses a method defined in a top scope, and it fails as well.

Akirathan · 2024-04-29T14:16:07Z

The only place where a TailCallException (thrown when a method call annotated with @Tail_Call is called) is caught, is in SimpleCallOptimiserNode.executeDispatch.

SimpleCallOptimiserNode is created in CurryNode.initializeCallNodes based on its TailStatus which is determined in TailCall compiler pass.

The two failing tests fail, because the TailCallException is caught in the stack above CatchPanicNode.doExecute thus, effectively discarding the CatchPanicNode.doExecute (which is the only place where a PanicException is caught) from the stack.

It is all about looking at the stack trace at the moment when TailCallException and PanicException are thrown.

Akirathan · 2024-04-29T15:45:22Z

Evaluating the following tmp.enso:

from Standard.Base import all

fn_tail x =
    if x == 1 then Panic.throw "my panic" else
        @Tail_Call fn_tail x+1

handler caught =
    IO.println <| "handler: caught = " + caught.to_text

main =
    Panic.catch Any handler=handler <| fn_tail 0

The stacktrace when TailCallException is thrown:
CurryNode.throwTco.txt
- Note that there is CatchPanicMethodGen on the stack.
The stacktrace when TailCallException is caught:
SimpleCallOptimiserNode.catchTco.txt
- Note that the CatchPanicMethodGen was discarded from the stack.
The stacktrace when PanicException is thrown:
ThrowPanicNode.txt

JaroslavTulach · 2024-05-01T08:54:37Z

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/CatchPanicNode.java

@@ -59,7 +59,7 @@ Object doExecute(
      @Cached BranchProfile otherExceptionBranchProfile,
      @CachedLibrary(limit = "3") InteropLibrary interop) {
    try {
-      return thunkExecutorNode.executeThunk(frame, action, state, BaseNode.TailStatus.TAIL_DIRECT);
+      return thunkExecutorNode.executeThunk(frame, action, state, BaseNode.TailStatus.NOT_TAIL);


Panic.catch cannot be in tail call position. I believe it has to be NOT_TAIL. All the tests in Error_Spec are fixed. Let's see what other collateral damage this change may have created. Btw. I have a feeling @hubertp was fighting with some similar issue a year back or so...

What a good observation! At the end, it seems that we will not have to dig in the IR phases. I have just added some comments and made sure that even the handler callback is not in a tail call position, just to be sure - in commit 8539997.

On contrary to the action invocation, handler can probably be in tail position. There is nothing in the CatchPanicNode that needs to run after handler...

Good note. Add a comment about that in d86c7b5 . handler is invoked with TAIL_DIRECT.

Btw. I have a feeling @hubertp was fighting with some similar issue a year back or so...

It's been a while so I can't remember but maybe you are referring to #7116

hubertp · 2024-05-01T10:35:14Z

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/CatchPanicNode.java

@@ -59,7 +59,7 @@ Object doExecute(
      @Cached BranchProfile otherExceptionBranchProfile,
      @CachedLibrary(limit = "3") InteropLibrary interop) {
    try {
-      return thunkExecutorNode.executeThunk(frame, action, state, BaseNode.TailStatus.TAIL_DIRECT);
+      return thunkExecutorNode.executeThunk(frame, action, state, BaseNode.TailStatus.NOT_TAIL);


Btw. I have a feeling @hubertp was fighting with some similar issue a year back or so...

It's been a while so I can't remember but maybe you are referring to #7116

Add a failing panic with TCO test

4553f8e

Akirathan self-assigned this Apr 15, 2024

Add the same test without using <|

ff5f913

Akirathan commented Apr 25, 2024

View reviewed changes

Akirathan added 3 commits April 25, 2024 18:21

Decrease the number of necessary iterations for the TCO tests

f91512b

Merge branch 'refs/heads/develop' into wip/akirathan/9251-panic-tco

f0d488b

Add another test with global wrapper function

cce3c03

Panic.catch is NOT_TAIL

c515188

JaroslavTulach reviewed May 1, 2024

View reviewed changes

Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label May 1, 2024

Make sure that even Panic handler is not a tail call

8539997

Akirathan marked this pull request as ready for review May 1, 2024 09:24

Akirathan requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley, 4e6 and hubertp as code owners May 1, 2024 09:24

Improve docs

d86c7b5

Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label May 1, 2024

hubertp approved these changes May 1, 2024

View reviewed changes

GregoryTravis approved these changes May 1, 2024

View reviewed changes

mergify bot merged commit 6b0361c into develop May 1, 2024
37 checks passed

mergify bot deleted the wip/akirathan/9251-panic-tco branch May 1, 2024 11:13

unfurl-links bot mentioned this pull request May 2, 2024

Problems catching a Panic thrown within a TCO loop #9251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Panics can be caught from TCO loops #9705

Panics can be caught from TCO loops #9705

Akirathan commented Apr 15, 2024 •

edited

Loading

Akirathan Apr 25, 2024

Akirathan Apr 29, 2024 •

edited

Loading

Akirathan commented Apr 29, 2024

Akirathan commented Apr 29, 2024

JaroslavTulach May 1, 2024 •

edited

Loading

Akirathan May 1, 2024

JaroslavTulach May 1, 2024

Akirathan May 1, 2024

hubertp May 1, 2024

hubertp May 1, 2024

Panics can be caught from TCO loops #9705

Panics can be caught from TCO loops #9705

Conversation

Akirathan commented Apr 15, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

Akirathan Apr 25, 2024

Choose a reason for hiding this comment

Akirathan Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Akirathan commented Apr 29, 2024

Akirathan commented Apr 29, 2024

JaroslavTulach May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Akirathan May 1, 2024

Choose a reason for hiding this comment

JaroslavTulach May 1, 2024

Choose a reason for hiding this comment

Akirathan May 1, 2024

Choose a reason for hiding this comment

hubertp May 1, 2024

Choose a reason for hiding this comment

hubertp May 1, 2024

Choose a reason for hiding this comment

Akirathan commented Apr 15, 2024 •

edited

Loading

Akirathan Apr 29, 2024 •

edited

Loading

JaroslavTulach May 1, 2024 •

edited

Loading