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

implement exception handling for Rust #1811

Merged
merged 12 commits into from
Sep 21, 2021
Merged

Conversation

matejcik
Copy link
Contributor

fixes #1598

@matejcik matejcik requested a review from prusnak as a code owner September 15, 2021 13:29
Copy link
Contributor

@jpochyla jpochyla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few codestyle comments. Overall I like it, nice!

core/embed/rust/src/micropython/list.rs Outdated Show resolved Hide resolved
core/embed/rust/src/micropython/obj.rs Outdated Show resolved Hide resolved
core/embed/rust/src/micropython/obj.rs Outdated Show resolved Hide resolved
core/embed/rust/src/micropython/runtime.rs Outdated Show resolved Hide resolved
core/embed/rust/src/micropython/runtime.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/defs.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/encode.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/encode.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/obj.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/obj.rs Outdated Show resolved Hide resolved
KeyError(Obj),
AttributeError(Qstr),
ValueError(&'static CStr),
ValueErrorParam(&'static CStr, Obj),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit unfortunate that this ties the Error type to MicroPython, and pollutes all the modules with it, but I suppose that a single error type is a bit of a (very convenient) anti-pattern anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, but it turns out that we don't do basically any error handling within Rust and the Error type is almost always supposed to fall through to uPy

@prusnak prusnak removed their request for review September 15, 2021 21:16
@matejcik matejcik force-pushed the matejcik/rust-exceptions branch 2 times, most recently from ba2d12c to 5c09fe5 Compare September 16, 2021 11:47
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Will approve after WIP commits are finished. Gonna be a bit tricky to track which upy functions might throw, but still massive improvement over no exception handling.

core/embed/rust/src/micropython/runtime.rs Show resolved Hide resolved
@matejcik matejcik force-pushed the matejcik/rust-exceptions branch from a4640be to 63b7936 Compare September 17, 2021 09:01
jpochyla and others added 8 commits September 17, 2021 13:51
@matejcik
Copy link
Contributor Author

@mmilata fixed the WIPs as 63b7936, PTAL

afterwards I'll rebase to master to fix conflicts

@matejcik matejcik force-pushed the matejcik/rust-exceptions branch from 63b7936 to 036d489 Compare September 17, 2021 12:19
@mmilata
Copy link
Member

mmilata commented Sep 17, 2021

Builds are failing because in micropython 1.17 the function mp_obj_new_exception_arg1 became static inline micropython/micropython@022b8a7.

@matejcik
Copy link
Contributor Author

i'll probably hand-call it where needed

@matejcik
Copy link
Contributor Author

it seems that the Rust unit tests are now broken because symbols are missing:
https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1599995792
@jpochyla can you take a look?

@mmilata
Copy link
Member

mmilata commented Sep 19, 2021

My understanding of the problem: previously we did not call any uPy code from unit tests but now we do so we need to link uPy objects to the test binary. I think we need to call cargo test from SCons and either:

  • pass all the required .o files (is that what's in obj_program? or can we just use trezor-emu-core somehow?) as parameters to it, or
  • build the test binary ourselves, like trezor-emu-core but with the test driver as main().

@jpochyla
Copy link
Contributor

The test feature is missing here: cargo test -- --test-threads=1

@mmilata
Copy link
Member

mmilata commented Sep 20, 2021

Nice, I forgot that the UI branch already had this using cc crate. Looks like it still needs some path tweaks: https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1605105556

@mmilata mmilata merged commit 8d7f3fb into master Sep 21, 2021
@mmilata mmilata deleted the matejcik/rust-exceptions branch September 21, 2021 10:43
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.

Add exception safety to Rust code
3 participants