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

[WIP] Introduce a libtock-platform crate under libtock-core #211

Closed
wants to merge 1 commit into from

Conversation

jrvanwhy
Copy link
Collaborator

@jrvanwhy jrvanwhy commented Jul 2, 2020

I intended to complete the libtock-platform crate before sending this PR for review, but I think it is growing too large to send without first discussing the design. Instead, I am sending it as a draft PR so we can discuss the design before I send a polished PR out.

This PR is part of a larger reorganization of libtock-core that I am trying to work on. In general, I would like to do the following:

  1. Split up the functionality of libtock-core into multiple crates that live in the core/ directory. libtock-core would remain as a crate that aggregates all the functionality, so that applications only need to use one crate directly. Examples and integration tests would live in core/examples and core/tests, so they can depend on libtock-core's cumulative functionality.
  2. Allow for unit tests for each sub-crate that run on the host system. libtock-platform has unit tests that run on the host system via a trivial cargo test. These tests are easier and faster to run that on-device tests, and can be run under cargo miri for undefined behavior detection. This is the idea discussed at [RFC] Reorganize libtock-rs for testability #132 (comment)
  3. Solve the soundness issues with the syscall API/platform layer (previously discussed in allow() is unsound #129 and Subscriptions and shared memory have a loophole which can lead to UB #143).

The libtock-platform crate I introduce here contains the following:

  1. A trait (Syscalls) representing raw system calls. There is no implementation of this trait in libtock-platform, as libtock-platform is OS-independent. Instead, separate crates (probably libtock-runtime and a testing crate) will provide implementations of Syscalls.
  2. A Platform struct that provides a higher-level interface over the Syscalls trait, supporting an asynchronous execution model similar to that used by the Tock kernel. Platform is not implemented yet. Its API is presented in the form of the PlatformApi trait, which is currently partially written, so that other libtock-core components can easily be generic over the platform type.
  3. ErrorCode and ReturnCode types that form efficient abstractions around the kernel's ReturnCode type.
  4. Traits based on the prototyping I documented at https://github.com/tock/design-explorations/tree/master/zst_pointer_async for asynchronous APIs: StaticCallback, AsyncCall, and Callback. These should allow for libtock-core to provide a generic adapter for synchronous APIs and a futures-based adapter for synchronous APIs, plus prevent a recursion issue the Tock kernel has experienced.

In a later PR, I will implement libtock-runtime, which will provide the real Syscalls implementation and entry point for libtock-core. I will probably pull the panic_handler, start implementation, and allocator into separate crates so that applications can switch between them. libtock-core will pull in everything by default for clients that want the convenience.

Btw, please ignore that I commented out the -D warnings flag in .cargo/config, that flag was getting in the way of my development and testing. I'll remove it when I'm done implementing the major functionality.

@alistair23
Copy link
Collaborator

Wow! There are over 1000 lines in a single commit. Can this be split up a little more?

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Jul 2, 2020

Wow! There are over 1000 lines in a single commit. Can this be split up a little more?

I think I can split this into the following parts:

  1. Syscalls
  2. Allowed, then AllowedSlice
  3. ReturnCode, then ErrorCode
  4. The async traits, then PlatformApi, then Platform.

My concern is that if I send those PRs in isolation, the context in which they are written will be very unclear. I'm not really sure how to approach this. I could keep this draft around for reference while re-implementing it in a more piecewise fashion.

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Jul 6, 2020

Does anyone have input on how I should proceed with this? I can:

  1. Continue with the 1kLOC+ change as-is.
  2. Split it into the parts I mentioned in my previous comment.
  3. Lead with documentation describing the intended design, then split it into the parts in my previous comment.
  4. Something else

@alistair23
Copy link
Collaborator

A single large patch is very difficult to review (making it more likely reviewers miss things) and makes bisecting future regressions more cumbersome. The PR should be split up into as small as patches as possible. It doesn't really matter if the docs for first or last, but they should also probably be split up.

@alistair23
Copy link
Collaborator

I noticed an update here, do you have an update on this PR?

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Sep 1, 2020

I noticed an update here, do you have an update on this PR?

I'm using this PR to develop libtock_platform and the components it interacts with. When something in this PR stabilizes (i.e. I'm confident it is approximately correct), I'll split it out into a separate PR.

That is where #220, #221, #222, #223, and #231 have come from.

@alistair23
Copy link
Collaborator

Just checking to see if you need anything else reviewed, I'll keep an eye out for future PRs

@jrvanwhy
Copy link
Collaborator Author

Closing because the design has drifted away from this PR's design, as documented in #256

@jrvanwhy jrvanwhy closed this Nov 26, 2020
@jrvanwhy jrvanwhy deleted the platform branch January 12, 2021 20:01
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