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

Add linear-handles feature to oak_io #1850

Merged
merged 4 commits into from
Feb 12, 2021

Conversation

wildarch
Copy link
Contributor

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
    • I have written tests that cover the code changes.
    • I have checked that these tests are run by
      Cloudbuild
    • I have updated documentation accordingly.
    • I have raised an issue to
      cover any TODOs and/or unfinished work.
  • Pull request includes prototype/experimental work that is under
    construction.

The feature enables what I hope will eventually be the default behaviour for handles. Roughly speaking, this does 3 things:

  • Make handles non-Copy
  • Add a Clone impl that calls oak_abi::handle_clone
  • Add a Drop impl that calls oak_abi::channel_close

This builds on #1782.
Related issue: #1686. It is step (3) in #1686 (comment).

@google-cla google-cla bot added the cla: yes label Jan 29, 2021
@wildarch wildarch force-pushed the oak_io_linear_handles_feature branch 2 times, most recently from c3e19dc to ac98c07 Compare January 29, 2021 13:30
@wildarch wildarch marked this pull request as ready for review January 29, 2021 13:30
Copy link
Contributor

@rbehjati rbehjati left a comment

Choose a reason for hiding this comment

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

LGTM. But can we have tests for it at this stage?

oak_io/src/handle.rs Outdated Show resolved Hide resolved
oak_io/src/handle.rs Outdated Show resolved Hide resolved
oak_io/src/handle.rs Outdated Show resolved Hide resolved
@wildarch
Copy link
Contributor Author

wildarch commented Feb 5, 2021

LGTM. But can we have tests for it at this stage?

I think unit tests would not add much value here, but I will try and set up some tests in abitest

@wildarch wildarch force-pushed the oak_io_linear_handles_feature branch from fe47bca to bd480d0 Compare February 5, 2021 18:05
@wildarch
Copy link
Contributor Author

wildarch commented Feb 5, 2021

I had to jump through a few hoops to set up ABI tests, mainly because we cannot enable the linear-handles feature in the existing modules of the abitest. I have added a new module that gets started and called by module_0 to get around that. It adds quite a bit of code, but it seems like it is worth it: I actually discovered a bug in the previous code.
The channel_close in the Drop impl was inside a trace! macro call, which meant that it was only called if a logger was set up (that would have been very hard to debug in an application, whoops!).

Copy link
Contributor

@rbehjati rbehjati left a comment

Choose a reason for hiding this comment

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

Nice. Thanks!

@wildarch
Copy link
Contributor Author

Another wrinkle: when running checks like clippy, cargo compiles the whole examples workspace in one go, and with apparently a single feature configuration for dependency in the tree. This caused the linear-handles feature to be enabled for other examples, failing to compile the SDK used there.
I've now added an additional no-linear-handles feature which suppresses the linear-handles feature, i.e. if both are enabled, linear handles are not available.
This is only an issue when running the workspace level checks, when compiling individual crates there is no problem.

The no-linear-handles feature is a bit hacky, but it's the best solution I could come up with. This should all be much nicer once we land linear handles support in the SDK.

@wildarch wildarch force-pushed the oak_io_linear_handles_feature branch from e2b6196 to 65e1e1d Compare February 12, 2021 12:03
This helps pass checks on the workspace level, where it would
erroneously enable the feature for all examples.

Also adding clippy linter suppression because it now checks the
linear_handles module without linear-handles enabled.
@wildarch wildarch force-pushed the oak_io_linear_handles_feature branch from 65e1e1d to 130dca6 Compare February 12, 2021 12:16
@wildarch wildarch merged commit 12c0e5d into project-oak:main Feb 12, 2021
@wildarch wildarch deleted the oak_io_linear_handles_feature branch February 12, 2021 16:27
@github-actions
Copy link

Reproducibility Index:

d98ae5b7966e55da498af66fd0dad2aeeb60e7f10b06bf7167fc6e7256a61498  ./examples/abitest/bin/abitest_0_frontend.wasm
adf1581e2523f13c4c3e10cc978df15eee01635fa7e0daaf4e609f73a5547875  ./examples/abitest/bin/abitest_1_backend.wasm
a5b271718d45f17bad093efc9dff2749d7f26ceadfd81120be6c20b169bc6dfb  ./examples/abitest/bin/abitest_linear_handles.wasm
a7aa5e97e39676283dbc1f925085c9956a69d3ec57699c91a94643fd61f9ab21  ./examples/aggregator/bin/aggregator.wasm
468647aaeb8fe108cd3759a9965a1c3e625b23e9ab0e0b50281497950973272b  ./examples/chat/bin/chat.wasm
5da46a357f4f04ff91e2a2d3bc67fe0fe066ffb28f5abfd1fc4795d940f1f3c8  ./examples/hello_world/bin/hello_world.wasm
26be34602512f67fe7779b99ed23e9116ead7459b71a09ea0b21ead2e24bdb14  ./examples/hello_world/bin/translator.wasm
fe26d65887b1ccd0f2aad016443dd18b6b06df0f69934f3bda511f7b134cc421  ./examples/http_server/bin/http_server.wasm
ad3fe40db1e73e91b394ed17d7eacccadbd3bcb99ae0f2dae5c50e97fb90103e  ./examples/injection/bin/injection.wasm
a3703389adaebd6938b7b89030c5ed9430aa72ca9d43655a0c42fd7fafed84d7  ./examples/private_set_intersection/bin/private_set_intersection.wasm
d450b6759999c6f32cd21baa222831d3fbb41e2f2da381ce65c5571548fedb6b  ./examples/private_set_intersection/bin/private_set_intersection_handler.wasm
1c9f4d2514223527bce653bb153c43a4f80843cd732efdc07e41af9d987a338b  ./examples/proxy_attestation/bin/proxy_attestation_example.wasm
26be34602512f67fe7779b99ed23e9116ead7459b71a09ea0b21ead2e24bdb14  ./examples/translator/bin/translator.wasm
f617131354132538a0028959acf0db90f82954469f4e72a62e01c075d48ea672  ./examples/trusted_database/bin/trusted_database.wasm
ab3bc68ef7f8ca524279c826a02a8c84355fcc141ee283c624098d375961857e  ./oak_loader/bin/oak_loader

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index 785f4fd..5674081 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,14 +1,15 @@
-a38a6077739f975b3ab13b6e1383402f7a1402152469cc660687d4a546c3edaf  ./examples/abitest/bin/abitest_0_frontend.wasm
-ba8998cfb93ef6f5eaa56f2c68f252cbb52314a0fe218e11bee9ca67906e0cc9  ./examples/abitest/bin/abitest_1_backend.wasm
-108864ffe7e2e478429dd29ea832b34ff59d2ad311f5b2cb8355767614172919  ./examples/aggregator/bin/aggregator.wasm
-a507ddc9397f99732c5f2a93b0cc67329ec75ec93cca1c1764b735e0551c1ce6  ./examples/chat/bin/chat.wasm
-6c998f3e14e09638d9fe7ea35cd8fde0242ffeb8fcda6976632bfc0e689421e9  ./examples/hello_world/bin/hello_world.wasm
-18c1587164a465bfb060b51092419a81e341be58807624f35053291121eee922  ./examples/hello_world/bin/translator.wasm
-ddd753b53270d23ca0717a2bbcb404ecd91cdb81f0d3918a856d0b383ce8f479  ./examples/http_server/bin/http_server.wasm
-19c63f06e2648dfbd3aa57b6c866edaa5a9e33971b9e6082f3607e8b9bff4c97  ./examples/injection/bin/injection.wasm
-bd603a81ca1d3c51504116803753b148b24e0363f883a4719d85aff4620c231a  ./examples/private_set_intersection/bin/private_set_intersection.wasm
-40a1795df36da525d78d872bba92685fb78adaaf509d86e6d6edf310c86af348  ./examples/private_set_intersection/bin/private_set_intersection_handler.wasm
-5d446e200c0a96092c946db8bc36f0a1347dc223d13dac79f7b36f5cfbbdc671  ./examples/proxy_attestation/bin/proxy_attestation_example.wasm
-18c1587164a465bfb060b51092419a81e341be58807624f35053291121eee922  ./examples/translator/bin/translator.wasm
-d35e7c4f85d875ef443b75097e1ef39d025e8df334a60ca5d5cfdde4d0cac1b9  ./examples/trusted_database/bin/trusted_database.wasm
-9e7e9594a2e6d44e65ad9c570b088329307f9b842c24a27d9f7961d37bb390ef  ./oak_loader/bin/oak_loader
+d98ae5b7966e55da498af66fd0dad2aeeb60e7f10b06bf7167fc6e7256a61498  ./examples/abitest/bin/abitest_0_frontend.wasm
+adf1581e2523f13c4c3e10cc978df15eee01635fa7e0daaf4e609f73a5547875  ./examples/abitest/bin/abitest_1_backend.wasm
+a5b271718d45f17bad093efc9dff2749d7f26ceadfd81120be6c20b169bc6dfb  ./examples/abitest/bin/abitest_linear_handles.wasm
+a7aa5e97e39676283dbc1f925085c9956a69d3ec57699c91a94643fd61f9ab21  ./examples/aggregator/bin/aggregator.wasm
+468647aaeb8fe108cd3759a9965a1c3e625b23e9ab0e0b50281497950973272b  ./examples/chat/bin/chat.wasm
+5da46a357f4f04ff91e2a2d3bc67fe0fe066ffb28f5abfd1fc4795d940f1f3c8  ./examples/hello_world/bin/hello_world.wasm
+26be34602512f67fe7779b99ed23e9116ead7459b71a09ea0b21ead2e24bdb14  ./examples/hello_world/bin/translator.wasm
+fe26d65887b1ccd0f2aad016443dd18b6b06df0f69934f3bda511f7b134cc421  ./examples/http_server/bin/http_server.wasm
+ad3fe40db1e73e91b394ed17d7eacccadbd3bcb99ae0f2dae5c50e97fb90103e  ./examples/injection/bin/injection.wasm
+a3703389adaebd6938b7b89030c5ed9430aa72ca9d43655a0c42fd7fafed84d7  ./examples/private_set_intersection/bin/private_set_intersection.wasm
+d450b6759999c6f32cd21baa222831d3fbb41e2f2da381ce65c5571548fedb6b  ./examples/private_set_intersection/bin/private_set_intersection_handler.wasm
+1c9f4d2514223527bce653bb153c43a4f80843cd732efdc07e41af9d987a338b  ./examples/proxy_attestation/bin/proxy_attestation_example.wasm
+26be34602512f67fe7779b99ed23e9116ead7459b71a09ea0b21ead2e24bdb14  ./examples/translator/bin/translator.wasm
+f617131354132538a0028959acf0db90f82954469f4e72a62e01c075d48ea672  ./examples/trusted_database/bin/trusted_database.wasm
+ab3bc68ef7f8ca524279c826a02a8c84355fcc141ee283c624098d375961857e  ./oak_loader/bin/oak_loader

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

Successfully merging this pull request may close these issues.

None yet

3 participants