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.

Sign up for GitHub

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

Jump to bottom

[wasm] Exception filter clause evaluated at wrong time #52394

Closed
radekdoulik opened this issue May 6, 2021 · 3 comments
Closed

[wasm] Exception filter clause evaluated at wrong time #52394

radekdoulik opened this issue May 6, 2021 · 3 comments
Assignees
@BrzVlad
Labels
arch-wasm WebAssembly architecture area-Codegen-Interpreter-mono
Milestone
6.0.0

Comments

@radekdoulik
Copy link
Member

Repro: radekdoulik@0a454d2

The modified sample above tests the ordering of exception filter clause evaluation. When built with AOT, the clause is evaluated at wrong time.

Looks like we are unwinding the stack (and thus running finnaly block) before evaluating the filter clause. That leads to unhandled exception in this test, as the finally block modifies the state (answer variable) and the clause is evaluated as false.

Test output built with AOT:

going to throw
finally: answer = 1
check: 1 == 42 => False
test_exit: 1
dotnet.js:6509 Unhandled exception in _finalize_startup Error: System.Exception: Throw
   at :64251/Sample.Test.TestExcFilter()
    at Object._convert_exception_for_method_call (dotnet.js:8196)
    at Object._handle_exception_for_call (dotnet.js:8258)
    at Object._handle_exception_and_produce_result_for_call (dotnet.js:8267)
    at Object._call_method_with_converted_args (dotnet.js:8295)
    at Object.call_method (dotnet.js:8254)
    at Object.call_static_method (dotnet.js:8483)
    at Object.init (localhost/:39)
    at Object.config.loaded_cb (runtime.js:8)
    at Object._finalize_startup (dotnet.js:6470)
    at onPendingRequestComplete (dotnet.js:6507)
onPendingRequestComplete @ dotnet.js:6509
dotnet.js:8196 Uncaught (in promise) Error: System.Exception: Throw
   at :64251/Sample.Test.TestExcFilter()
    at Object._convert_exception_for_method_call (dotnet.js:8196)
    at Object._handle_exception_for_call (dotnet.js:8258)
    at Object._handle_exception_and_produce_result_for_call (dotnet.js:8267)
    at Object._call_method_with_converted_args (dotnet.js:8295)
    at Object.call_method (dotnet.js:8254)
    at Object.call_static_method (dotnet.js:8483)
    at Object.init (localhost/:39)
    at Object.config.loaded_cb (runtime.js:8)
    at Object._finalize_startup (dotnet.js:6470)
    at onPendingRequestComplete (dotnet.js:6507)

When built without AOT, it works as expected and we don't get unhandled exception. Output with interpreter only:

going to throw
check: 42 == 42 => True
finally: answer = 1
catch in TestExcFilter, answer: 1

Assigning to @BrzVlad as the filtering happens in interpreter. The origin of the issue might be in AOT though, as the handling of the exception starts in AOT code.

/cc @vargaz
@radekdoulik radekdoulik added this to the 6.0.0 milestone May 6, 2021
@ghost
Copy link

ghost commented May 6, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Repro: radekdoulik@0a454d2

The modified sample above tests the ordering of exception filter clause evaluation. When built with AOT, the clause is evaluated at wrong time.

Looks like we are unwinding the stack (and thus running finnaly block) before evaluating the filter clause. That leads to unhandled exception in this test, as the finally block modifies the state (answer variable) and the clause is evaluated as false.

Test output built with AOT:

going to throw
finally: answer = 1
check: 1 == 42 => False
test_exit: 1
dotnet.js:6509 Unhandled exception in _finalize_startup Error: System.Exception: Throw
   at :64251/Sample.Test.TestExcFilter()
    at Object._convert_exception_for_method_call (dotnet.js:8196)
    at Object._handle_exception_for_call (dotnet.js:8258)
    at Object._handle_exception_and_produce_result_for_call (dotnet.js:8267)
    at Object._call_method_with_converted_args (dotnet.js:8295)
    at Object.call_method (dotnet.js:8254)
    at Object.call_static_method (dotnet.js:8483)
    at Object.init (localhost/:39)
    at Object.config.loaded_cb (runtime.js:8)
    at Object._finalize_startup (dotnet.js:6470)
    at onPendingRequestComplete (dotnet.js:6507)
onPendingRequestComplete @ dotnet.js:6509
dotnet.js:8196 Uncaught (in promise) Error: System.Exception: Throw
   at :64251/Sample.Test.TestExcFilter()
    at Object._convert_exception_for_method_call (dotnet.js:8196)
    at Object._handle_exception_for_call (dotnet.js:8258)
    at Object._handle_exception_and_produce_result_for_call (dotnet.js:8267)
    at Object._call_method_with_converted_args (dotnet.js:8295)
    at Object.call_method (dotnet.js:8254)
    at Object.call_static_method (dotnet.js:8483)
    at Object.init (localhost/:39)
    at Object.config.loaded_cb (runtime.js:8)
    at Object._finalize_startup (dotnet.js:6470)
    at onPendingRequestComplete (dotnet.js:6507)

When built without AOT, it works as expected and we don't get unhandled exception. Output with interpreter only:

going to throw
check: 42 == 42 => True
finally: answer = 1
catch in TestExcFilter, answer: 1

Assigning to @BrzVlad as the filtering happens in interpreter. The origin of the issue might be in AOT though, as the handling of the exception starts in AOT code.

/cc @vargaz

Author: radekdoulik
Assignees: BrzVlad
Labels:

arch-wasm, area-Codegen-Interpreter-mono
Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 6, 2021
@marek-safar marek-safar removed the untriaged New issue has not been triaged by the area owner label May 7, 2021
@radekdoulik radekdoulik mentioned this issue May 18, 2021
Merged
@BrzVlad
Copy link
Member

BrzVlad commented Jun 14, 2021

@radekdoulik I assume this was fixed by #52883 right ?

@radekdoulik
Copy link
Member Author

Yes, it is fixed now.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@BrzVlad BrzVlad

Labels
arch-wasm WebAssembly architecture area-Codegen-Interpreter-mono
Projects
None yet
Milestone
6.0.0
Development

No branches or pull requests
3 participants
@marek-safar @radekdoulik @BrzVlad