forbid hook routines raising exceptions #1236

zerbina · 2024-03-13T00:32:27Z

Summary

This is enforced as follows:

if a hook routine potentially raises an exception, a compile-time
error is reported
if a hook routine raises a defect at run-time, the program panics

Exception effects of hooks side-steps effect tracking for
procedures, and if a hook does raise at run-time, behaviour was
previously undefined. Disallowing hooks to raise exceptions resolves
both issues.

Performance of the produced executables also improves significantly
(depending on the code), as destroy hooks not being able to raise
results in better code generation.

Details

The implementation is made up of three parts:

statically enforcing that no exceptions are raised by hooks (in
sempass2)
preventing exceptions from exiting hooks at run-time (in mirgen/
liftdestructors)
support in the runtime for panicking on unhandled exceptions

Static Detection

for hooks, identified by the presence of the sfOverriden flag,
sempass2 tests against an empty .raises specification, ensuring
that no (tracked) exceptions can be raised
the existing exception specification is always replaced
no error is reported when .raises: [] was explicitly specified, to
give precedence to the can raise unlisted exception error.
the symbol of hooks is marked with sfNeverRaises, to enforce at
run-time that no exceptions (defects) leave the routine

The meaning of sfNeverRaises is changed from being a hint/guarantee
to being a request.

Enforcement

mirgen wraps the body of sfNeverRaises procedure in a
try: ... except: nimUnhandledException()
elimination of unreachable code in cgirgen removes the except if
it's not used in practice
all synthesized hook procedure (liftdestructors) are flagged with
sfNeverRaises; the canRaise tracking is removed

Runtimes

C runtime: nimUnhandledException displays the exception and quits
the process
node.js JS runtime: nimUnhandledException displays the exception
and quits the process
non-node.js JS runtime: nimUnhandledException re-raises the
exception (there's currently no way to terminate the program)
VM: nimUnhandledException is a vmop that raises a
vmEvtUnhandledException event, which aborts execution

Standard Library and Tests

fix the Task =destroy hook being inferred to raise exceptions
fix GC_fullCollect being inferred to raise exceptions

Tests

Three individual tests need to be adjusted to the language change:

tnew and gctest use debugEcho instead of write (the former
has no effects)
tarcmisc has to cast away the raise effects of Stream.close for
now

Specification

The beginnings of a specification test category for hook routines is
added. At the moment, it only covers the exception behaviour.

The beginning of a language specification test category dedicated to hooks is added. At the moment, only the exception-effect-related behaviour is covered.

The procedure symbol is also marked with the `sfNeverRaises` flag, so that defects are prevented from escaping at run-time (they're not tracked at compile-time). A new report is added: `rsemHooksCannotRaise`.

Lifting/synthesizing hooks no longer needs to track whether an exception is possible.

They're guaranteed to not require exception propagation, so a normal `mnkCall` can always be used. The comment detailing the now-resolved problem with hooks that raise is also removed.

If a procedure is flagged with `sfNeverRaises`, the body is wrapped in a catch-all exception handler, where the new `nimUnhandledException` runtime procedure is invoked from the handler. If the handler is not actually used, `cgirgen`'s unreachable code elimination automatically eliminates it again. Implementing this in `mirgen` ensures consistent behaviour across all backends.

For the VM, it's not as simple as for the other two, so the implementation is still missing.

The trace hooks were considered to be able to raise exceptions and have effects, but this is no longer the case, so the pointer can be safely cast to procedural values without.

The callback is only used internally, so this does not constitute a breaking change of the `Task` API.

Three individual tests need to be adjusted to the language change: * `tnew` and `gctest use `debugEcho` instead of `write` (the former has no effects) * `tarcmisc` has to cast away the raise effects of `Stream.close` for now

Hooks not being considered as potentially raising is visible in the pretty-printed MIR output.

Reporting of unhandled exceptions doesn't include a trailing ':' in the displayed message if there's no exception message. A message is now set on the raised defect, which, besides resulting in consistent output, also helps makes sure that the correct defect is reported.

The procedure is implemented as a syscall (`vmops`), which raises the existing `vmEvtUnhandledException` VM event, thus terminating the VM right away. Since syscall implementations are currently unable to create a stack- trace, no stack-trace is passed along, which `compilerbridge.buildError` now accounts for (by creating the stack- trace itself). This is somewhat hack-y.

While not possible directly with JavaScript, node.js provides `process.exit`, which allows terminating in case of an unhandled exception.

zerbina · 2024-03-15T21:30:46Z

For user-defined effects (tags) the same reasoning applies, meaning that .tags: [] should too be enforced for hooks, I think. That's what I had implemented originally, but leaving it to a separate PR (and possible discussion) seemed better, so I removed it again.

zerbina · 2024-03-15T21:33:44Z

In line with what @saem started for the specification test cleanup, I've used a s99 prefix for the hook category, to signal that its placement needs some further thought / likely isn't final.

saem · 2024-03-16T00:59:13Z

The quoted bullet point below is incomplete, want write sure what you meant to write:

the symbol of hooks is marked with sfNeverRaises, to enforce the

zerbina · 2024-03-16T01:40:27Z

The quoted bullet point below is incomplete, want write sure what you meant to write:

Oh, indeed. I've completed the sentence now.

saem

Very cool, there is the bit in the PR body, but this is good to merge afterwards.

saem · 2024-03-16T03:08:50Z

/merge

github-actions · 2024-03-16T03:09:17Z

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:

To-Do

implement nimUnhandledException for the VM

~~make a separate PR for making the "unhandled exception" message on the JS backend consistent with that of the other backends~~

add tests for the error reporting behaviour

write a proper commit message

Notes for Reviewers

hooks being able to raise is also a problem for bringing goto-based control-flow to the MIR (the control-flow paths through user code need to be known at AST-to-MIR translation time)

zerbina added 10 commits March 12, 2024 19:43

specify that hooks must not raise exceptions

c4b6fb3

The beginning of a language specification test category dedicated to hooks is added. At the moment, only the exception-effect-related behaviour is covered.

sempass2: ensure that hooks don't raise exceptions

3e52eaa

The procedure symbol is also marked with the `sfNeverRaises` flag, so that defects are prevented from escaping at run-time (they're not tracked at compile-time). A new report is added: `rsemHooksCannotRaise`.

liftdestructors: remove "raises" tracking

65adcae

Lifting/synthesizing hooks no longer needs to track whether an exception is possible.

injectdestructors: always use unchecked calls for hooks

7786e11

They're guaranteed to not require exception propagation, so a normal `mnkCall` can always be used. The comment detailing the now-resolved problem with hooks that raise is also removed.

implement nimUnhandledException for C and JavaScript

8110d6a

For the VM, it's not as simple as for the other two, so the implementation is still missing.

orc: GC_fullCollect doesn't raise

e8a94e7

The trace hooks were considered to be able to raise exceptions and have effects, but this is no longer the case, so the pointer can be safely cast to procedural values without.

tasks: mark destroy callback as not raising

6482de5

The callback is only used internally, so this does not constitute a breaking change of the `Task` API.

tests: adjust tests

0cb2a24

Three individual tests need to be adjusted to the language change: * `tnew` and `gctest use `debugEcho` instead of `write` (the former has no effects) * `tarcmisc` has to cast away the raise effects of `Stream.close` for now

tests: update --expandArc-using tests

9b22cf4

Hooks not being considered as potentially raising is visible in the pretty-printed MIR output.

zerbina added this to the MIR phase milestone Mar 13, 2024

zerbina added 4 commits March 15, 2024 01:36

tests: make sure a .raises: [] error takes precedence

dd4db85

jssys: properly terminate when using node.js

0643d4d

While not possible directly with JavaScript, node.js provides `process.exit`, which allows terminating in case of an unhandled exception.

zerbina requested a review from saem March 15, 2024 21:30

zerbina marked this pull request as ready for review March 15, 2024 21:30

saem approved these changes Mar 16, 2024

View reviewed changes

chore-runner bot added this pull request to the merge queue Mar 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 16, 2024

zerbina added this pull request to the merge queue Mar 16, 2024

Merged via the queue into nim-works:devel with commit f98e817 Mar 16, 2024
40 checks passed

zerbina deleted the disallow-hook-raising-exceptions branch March 16, 2024 17:12

zerbina mentioned this pull request Mar 17, 2024

destructor failing should not lead to leaks #1012

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

forbid hook routines raising exceptions #1236

forbid hook routines raising exceptions #1236

zerbina commented Mar 13, 2024 •

edited by github-actions bot

Loading

zerbina commented Mar 15, 2024

zerbina commented Mar 15, 2024

saem commented Mar 16, 2024

zerbina commented Mar 16, 2024

saem left a comment •

edited

Loading

saem commented Mar 16, 2024

github-actions bot commented Mar 16, 2024

To-Do

Notes for Reviewers

forbid hook routines raising exceptions #1236

forbid hook routines raising exceptions #1236

Conversation

zerbina commented Mar 13, 2024 • edited by github-actions bot Loading

Summary

Details

Static Detection

Enforcement

Runtimes

Standard Library and Tests

Tests

Specification

zerbina commented Mar 15, 2024

zerbina commented Mar 15, 2024

saem commented Mar 16, 2024

zerbina commented Mar 16, 2024

saem left a comment • edited Loading

Choose a reason for hiding this comment

saem commented Mar 16, 2024

github-actions bot commented Mar 16, 2024

To-Do

Notes for Reviewers

zerbina commented Mar 13, 2024 •

edited by github-actions bot

Loading

saem left a comment •

edited

Loading