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

Fix throwing non-embedded builtin exceptions #2663

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SquidDev
Copy link
Contributor

ARTIQ Pull Request

Description of Changes

comm_kernel currently encodes all (Python) builtin exceptions with an id of 0. However, lookup for these exceptions happens from the artiq.coredevice.exceptions module. This means if you try to throw a builtin exception not in the embedding map, then it crashes when receiving it on the host. This also prevents you catching these exceptions in kernel code.

We now encode only use an exn id of 0 if the exception is in the exceptions module (and thus already in the embedding map).

I believe this is a relatively recent regression — I noticed this while in the process of updating to latest ARTIQ. This was possibly introduced in #2526, but afraid I have not bisected this.

Type of Changes

Type
🐛 Bug fix

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

  • Use correct spelling and grammar.
  • Check the copyright situation of your changes and sign off your patches (git commit --signoff, see copyright).

Code Changes

comm_kernel currently encodes all builtin exceptions with an id of 0.
However, lookup for these exceptions happens from the
artiq.coredevice.exceptions module.

This means if you try to throw a builtin exception not in the embedding
map, then it crashes when receiving it on the host. This also prevents
you catching these exceptions in kernel code.

We now encode only use an exn id of 0 if the exception is in the
exceptions module (and thus already in the embedding map).

Signed-off-by: Jonathan Coates <[email protected]>
@AR-PyT
Copy link
Contributor

AR-PyT commented Jan 30, 2025

I was under the impression that we predefined the set of builtin exceptions we would use in artiq.coredevice.exceptions and that these were the only exceptions we would see in a normal use-case (the PR fixes this assumption).
On the kernel end however, if we had to raise a builtin exception for instance the IsADirectoryError, we would still need to add it to the EmbeddingMap otherwise the lookup in ksupport::eh_artiq::get_exception_id will fail.
Either we can either store other builtin exceptions as objects (done in this PR) if we do not intend to raise them from kernel or add the definitions of other builtin exceptions to the EmbeddingMap (currently 13 added out of 52 in python builtins module included).

@SquidDev
Copy link
Contributor Author

SquidDev commented Jan 30, 2025

So get_exception_id is only used for firmware, and not for exceptions thrown in the kernel. So as far as the firmware goes, the embedding map only needs to include any exceptions thrown there (CacheError, DMAError, I2CError, RTIODestinationUnreachable, RTIOOverflow, RTIOUnderflow, SPIError, SubkernelError).

However, looking at this further, I think this is more complex/broken then I initially realised.

1) When we compile a kernel:

  • If we refer to one of the exceptions in the prelude by name, we use 0:<exn name> as the exception name.
  • For other exceptions, we either set the ID to 0 (for ARTIQ exceptions) or lookup/allocate a new ID, and use {id}:{exn.__module__}.{exn.__qualname__} as the exception name.

2) When an RPC method throws, we perform the following translation:

  • ARTIQ exceptions are mapped to 0:{exn.__module__}.{exn.__qualname__}
  • Other exceptions in the aq.cd.exceptions module are mapped to 0:{exn.__qualname__}.
  • We lookup/allocate a new ID, and use {id}:{exn.__module__}.{exn.__qualname__} as the exception name.

3) When translating a kernel exception back to a host exception, we do the following:

  • If the exception starts with 0:, then we look up the exception in `aq.cd.exceptions.
  • Otherwise we look up the id in the embedding map.

The main problem here comes from the mismatch between 1) and 2). Not all exceptions in aq.cd.exceptions are included in the prelude. This means, for the following code:

from builtins import AssertionError
from artiq.language.core import kernel

def throw():
    assert False

@kernel
def entrypoint():
    try:
        throw()
    except AssertionError:
        print("Caught")
  • In the compiled code, AssertionError is mapped to 3:builtins.AssertionError.
  • In the RPC-served exception, AssertionError is mapped to 0:AssertionError.

In fact, we don't even need RPCs for this! When the ARTIQ IR generator generates assertions, it doesn't go through the embedding map, and so also generates different exception ids:

from builtins import AssertionError
from artiq.language.core import kernel

@kernel
def entrypoint():
    try:
        assert False # 0:AssertionError
    except AssertionError: # 2:builtins.AssertionError
        print("Caught")

I feel the cleanest fix here would be just to drop the strings mapping and special casing of ARTIQ exceptions/builtin exceptions (outside of the initial population of the embedding map) and lookup/store the type in the embedding map directly. I don't know — not familiar enough with the history and design decisions behind the current implementation to know if there are reasons against this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants