Link and use Rust runtime from C++ main #801
Conversation
|
Fixes #729 |
|
There's obviously a lot of stuff here, but reviewing commit-by-commit will help. They're also in chunks:
If it helps, I can split out the preparatory commits (the first two sections) into separate PRs. |
oak/server/rust/oak_abi/build.rs
Outdated
| @@ -29,6 +29,7 @@ fn main() { | |||
| } | |||
|
|
|||
| prost_build::Config::new() | |||
| .out_dir("src/proto/") | |||
There was a problem hiding this comment.
Doesn't this end up rebuilding everything every single time, because it generates files within the source tree directly, and then assume that things have changed and rebuild everything unconditionally? I think we had to change this in the past exactly to avoid this problem.
There was a problem hiding this comment.
It doesn't seem to, not sure why:
drysdale@ring:~/src/oak/oak/server/rust/oak_abi{rust-link}:cargo build --verbose
Fresh cfg-if v0.1.10
Fresh unicode-xid v0.2.0
Fresh ppv-lite86 v0.2.6
Fresh remove_dir_all v0.5.2
Fresh autocfg v1.0.0
Fresh either v1.5.3
Fresh bytes v0.5.4
Fresh same-file v1.0.6
Fresh fixedbitset v0.2.0
Fresh unicode-segmentation v1.6.0
Fresh autocfg v0.1.7
Fresh multimap v0.8.0
Fresh maplit v1.0.2
Fresh c2-chacha v0.2.3
Fresh itertools v0.8.2
Fresh walkdir v2.3.1
Fresh heck v0.3.1
Fresh libc v0.2.66
Fresh proc-macro2 v1.0.8
Fresh log v0.4.8
Fresh protobuf v2.10.1
Fresh anyhow v1.0.26
Fresh getrandom v0.1.14
Fresh quote v1.0.2
Fresh protobuf-codegen v2.10.1
Fresh which v3.1.0
Fresh protoc v2.10.1
Fresh rand v0.4.6
Fresh indexmap v1.3.2
Fresh syn v1.0.14
Fresh rand_core v0.5.1
Fresh grpc-compiler v0.7.0 (/usr/local/google/home/drysdale/src/oak/third_party/grpc-rust/grpc-compiler)
Fresh tempdir v0.3.7
Fresh petgraph v0.5.0
Fresh rand_chacha v0.2.1
Fresh prost-derive v0.6.1
Fresh proc-macro-hack v0.5.11
Fresh rand v0.7.3
Fresh prost v0.6.1
Fresh const-random-macro v0.1.8
Fresh tempfile v3.1.0
Fresh prost-types v0.6.1
Fresh const-random v0.1.8
Fresh protoc-rust v2.10.1
Fresh prost-build v0.6.1
Fresh ahash v0.2.18
Fresh protoc-rust-grpc v0.7.0 (/usr/local/google/home/drysdale/src/oak/third_party/grpc-rust/protoc-rust-grpc)
Fresh hashbrown v0.6.3
Fresh oak_utils v0.1.0 (/usr/local/google/home/drysdale/src/oak/sdk/rust/oak_utils)
Fresh oak_abi v0.1.0 (/usr/local/google/home/drysdale/src/oak/oak/server/rust/oak_abi)
Finished dev [unoptimized + debuginfo] target(s) in 0.29s
oak/server/oak_runtime.h
Outdated
| @@ -93,16 +95,17 @@ class OakRuntime : public BaseRuntime { | |||
| // to the initial Application Wasm Node. | |||
| Handle app_handle_; | |||
|
|
|||
| // Next index for node name generation. | |||
| mutable absl::Mutex mu_; // protects nodes_, next_index_; | |||
| // Next indexes for node name/ID generation. | |||
| @@ -796,9 +852,12 @@ impl super::Node for WasmNode { | |||
| { | |||
| let (abi, _) = | |||
| WasmInterface::new(self.config_name.clone(), self.runtime.clone(), self.reader); | |||
| let wasi_stub = WasiStub {}; | |||
| * limitations under the License. | ||
| */ | ||
|
|
||
| #ifndef OAK_SERVER_RUST_OAK_GLUE_H_ |
There was a problem hiding this comment.
Have you considered / looked into using https://github.com/rust-lang/rust-bindgen to generate Rust stubs from this header file? Not sure how well it would work with Bazel though.
There was a problem hiding this comment.
Nope, I just went for manual generation (I basically resurrected the old oak_tests host functions)
| // The following functions are analogous to those on the Oak ABI, with the | ||
| // addition of an initial node_id parameter that identifies the Node performing | ||
| // the operation. |
There was a problem hiding this comment.
(optional) Personally I think it would be more appropriate to say that the are the same as the Oak Rust API. "Oak ABI" really means Wasm host function calls, and the fact that we even have Wasm nodes seems an implementation detail here. If anything, we should have a doc comment on the Wasm ABI saying that the functions there are analogous to the Rust API :)
There was a problem hiding this comment.
I don't really agree.
- The Oak Rust API, as provided by the SDK, gives access to Oak functionality using normal Rust types (e.g. with no need to worry about raw memory).
- The Oak ABI gives access to Oak functionality using a defined arrangement of low-level bytes, linear memory offsets and host functions.
- The glue here gives access to Oak functionality using a defined arrangement of low-level bytes, raw memory pointers, and
extern "C"functions.
So "analogous to .. the ABI" seems the most helpful description for the glue.
oak/server/rust/oak_glue/src/lib.rs
Outdated
| let size = oak_abi::SPACE_BYTES_PER_HANDLE * count as usize; | ||
| let mut handle_data = Vec::<u8>::with_capacity(size); | ||
| std::ptr::copy_nonoverlapping(buf, handle_data.as_mut_ptr(), size); | ||
| handle_data.set_len(size); | ||
| let mut handles = Vec::with_capacity(count as usize); | ||
| let mut mem_reader = Cursor::new(handle_data); | ||
| for _ in 0..count { | ||
| let handle = mem_reader.read_u64::<byteorder::LittleEndian>().unwrap(); | ||
| let _b = mem_reader.read_u8().unwrap(); | ||
| debug!("{{{}}}: wait_on_channels handle {:?}", node_id, handle); | ||
| handles.push(Some(oak_runtime::Handle(handle))); | ||
| } |
There was a problem hiding this comment.
Is from_raw_parts not good here because of potentially different endianness? Either way, perhaps add a comment?
There was a problem hiding this comment.
Not sure I follow – from_raw_parts isn't relevant here because we're not building a slice from a contiguous sequence of elements in memory (every ninth byte gets skipped). Added a comment.
| .unwrap_or(OakStatus::ErrInternal as u32) | ||
| } | ||
|
|
||
| /// # Safety |
There was a problem hiding this comment.
Perhaps add a see [`Runtime::channel_read`] or similar "redirect" comment (also elsewhere).
cargo/fake_lib.rs
Outdated
| @@ -0,0 +1 @@ | |||
|
|
|||
| impl super::Node for PseudoNode { | ||
| fn start(&mut self) -> Result<(), OakStatus> { | ||
| // Clone or copy all the captured values and move them into the closure, for simplicity. | ||
| let config_name = self.config_name.clone(); | ||
| let runtime = self.runtime.clone(); | ||
| let reader = self.reader; | ||
| let thread_handle = thread::Builder::new() | ||
| .name(format!("external={}", config_name)) | ||
| .spawn(move || { | ||
| let pretty_name = format!("{}-{:?}:", config_name, thread::current()); | ||
| let factory_fn: NodeFactory = FACTORY | ||
| .read() | ||
| .expect("unlock failed") | ||
| .expect("no registered factory"); | ||
| info!( |
There was a problem hiding this comment.
I'm not a big fan of all this two-way registering protocol, and also the fact that the Runtime does not actually know what's going to happen when creating an unrecognized pseudo-node. I think it's fine to assume that if we add a new pseudo-node type in C++, we can manage adding a bit of code to the Rust runtime to connect things in a more idiomatic way, and avoid the External configuration enum type too. Perhaps just keep an ExternalNode struct here that wraps the thread creation, and forwards to the actual C method that executes the main node event loop. WDYT?
There was a problem hiding this comment.
Yeah, there's a certain amount of path-of-least-resistance:
- With
mainin C++, we only want explicit linkage from C++ to Rust, not vice versa (to avoid cycles) - Rust needs to up-call to C++ for pseudo-Node creation, which therefore has to happen via a function pointer.
- Any such function pointer then has to be registered via a call from C++ to Rust.
- A single such function pointer can currently be used for all pseudo-Node types, because the C++ runtime has a full copy of the configuration and so the config name is all that's needed, regardless of pseudo-Node type (no need to pass pseudo-Node-specific configuration values extracted from the
ApplicationConfiguration).
A lot of the above will change once main is in Rust, so I don't think it's worth changing things here – #724 will require a re-work of the C++ pseudo-Node registration stuff anyway (and I've added a reminder TODO).
oak/server/oak_node.cc
Outdated
| OAK_LOG(INFO) << "Dropping " << channel_halves_.size() << " handles for Node"; | ||
| channel_halves_.clear(); | ||
| int count = statuses->size(); | ||
| std::vector<uint8_t> space(9 * count); |
There was a problem hiding this comment.
Could you give 9 a name?
There was a problem hiding this comment.
Now that #772 has been merged, this now has an awkward clash:
- Rust gRPC server pseudo-node #772 added a dependence from
oak_runtimetooak, which pulls in a gazilion dependencies which we don't really want tocargo razeify. - That dependency on
oakwas really just to get theGrpcRequestprotos, which would be better located inoak_abianyway. - Move ABI protobuf code to
oak_abicrate #764 covers that move of generated proto files, but that's blocked on - Switch from
rust_protobuftoprostfor protobuf generation #668, which aims to sort outprostvsrust_protobuf
cargo/fake_lib.rs
Outdated
| @@ -0,0 +1 @@ | |||
|
|
|||
oak/server/rust/oak_abi/build.rs
Outdated
| @@ -29,6 +29,7 @@ fn main() { | |||
| } | |||
|
|
|||
| prost_build::Config::new() | |||
| .out_dir("src/proto/") | |||
There was a problem hiding this comment.
It doesn't seem to, not sure why:
drysdale@ring:~/src/oak/oak/server/rust/oak_abi{rust-link}:cargo build --verbose
Fresh cfg-if v0.1.10
Fresh unicode-xid v0.2.0
Fresh ppv-lite86 v0.2.6
Fresh remove_dir_all v0.5.2
Fresh autocfg v1.0.0
Fresh either v1.5.3
Fresh bytes v0.5.4
Fresh same-file v1.0.6
Fresh fixedbitset v0.2.0
Fresh unicode-segmentation v1.6.0
Fresh autocfg v0.1.7
Fresh multimap v0.8.0
Fresh maplit v1.0.2
Fresh c2-chacha v0.2.3
Fresh itertools v0.8.2
Fresh walkdir v2.3.1
Fresh heck v0.3.1
Fresh libc v0.2.66
Fresh proc-macro2 v1.0.8
Fresh log v0.4.8
Fresh protobuf v2.10.1
Fresh anyhow v1.0.26
Fresh getrandom v0.1.14
Fresh quote v1.0.2
Fresh protobuf-codegen v2.10.1
Fresh which v3.1.0
Fresh protoc v2.10.1
Fresh rand v0.4.6
Fresh indexmap v1.3.2
Fresh syn v1.0.14
Fresh rand_core v0.5.1
Fresh grpc-compiler v0.7.0 (/usr/local/google/home/drysdale/src/oak/third_party/grpc-rust/grpc-compiler)
Fresh tempdir v0.3.7
Fresh petgraph v0.5.0
Fresh rand_chacha v0.2.1
Fresh prost-derive v0.6.1
Fresh proc-macro-hack v0.5.11
Fresh rand v0.7.3
Fresh prost v0.6.1
Fresh const-random-macro v0.1.8
Fresh tempfile v3.1.0
Fresh prost-types v0.6.1
Fresh const-random v0.1.8
Fresh protoc-rust v2.10.1
Fresh prost-build v0.6.1
Fresh ahash v0.2.18
Fresh protoc-rust-grpc v0.7.0 (/usr/local/google/home/drysdale/src/oak/third_party/grpc-rust/protoc-rust-grpc)
Fresh hashbrown v0.6.3
Fresh oak_utils v0.1.0 (/usr/local/google/home/drysdale/src/oak/sdk/rust/oak_utils)
Fresh oak_abi v0.1.0 (/usr/local/google/home/drysdale/src/oak/oak/server/rust/oak_abi)
Finished dev [unoptimized + debuginfo] target(s) in 0.29s
oak/server/oak_node.cc
Outdated
| OAK_LOG(INFO) << "Dropping " << channel_halves_.size() << " handles for Node"; | ||
| channel_halves_.clear(); | ||
| int count = statuses->size(); | ||
| std::vector<uint8_t> space(9 * count); |
| /// Caller must ensure that the memory range [config_buf, config_buf+config_len) is | ||
| /// accessible and holds a protobuf-serialized ApplicationConfiguration message. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn glue_start( |
There was a problem hiding this comment.
That's not what I understood unsafe to mean, in this context – an unsafe function has "requirements we need to uphold when we call this function, because Rust can’t guarantee we’ve met these requirements".
| #[no_mangle] | ||
| pub unsafe extern "C" fn glue_start( | ||
| config_buf: *const u8, | ||
| config_len: u32, |
There was a problem hiding this comment.
I prefer an explicitly sized type here so there's no need to think about whether the C++ and Rust int/pointer sizes match.
(I may be slightly contradicting myself with #279, but the more important part of that was the linear-memory offset integer type, which isn't relevant here because there are actual pointer types available.)
| // The following functions are analogous to those on the Oak ABI, with the | ||
| // addition of an initial node_id parameter that identifies the Node performing | ||
| // the operation. |
There was a problem hiding this comment.
I don't really agree.
- The Oak Rust API, as provided by the SDK, gives access to Oak functionality using normal Rust types (e.g. with no need to worry about raw memory).
- The Oak ABI gives access to Oak functionality using a defined arrangement of low-level bytes, linear memory offsets and host functions.
- The glue here gives access to Oak functionality using a defined arrangement of low-level bytes, raw memory pointers, and
extern "C"functions.
So "analogous to .. the ABI" seems the most helpful description for the glue.
| * limitations under the License. | ||
| */ | ||
|
|
||
| #ifndef OAK_SERVER_RUST_OAK_GLUE_H_ |
There was a problem hiding this comment.
Nope, I just went for manual generation (I basically resurrected the old oak_tests host functions)
| .unwrap_or(OakStatus::ErrInternal as u32) | ||
| } | ||
|
|
||
| /// # Safety |
oak/server/rust/oak_glue/src/lib.rs
Outdated
| let size = oak_abi::SPACE_BYTES_PER_HANDLE * count as usize; | ||
| let mut handle_data = Vec::<u8>::with_capacity(size); | ||
| std::ptr::copy_nonoverlapping(buf, handle_data.as_mut_ptr(), size); | ||
| handle_data.set_len(size); | ||
| let mut handles = Vec::with_capacity(count as usize); | ||
| let mut mem_reader = Cursor::new(handle_data); | ||
| for _ in 0..count { | ||
| let handle = mem_reader.read_u64::<byteorder::LittleEndian>().unwrap(); | ||
| let _b = mem_reader.read_u8().unwrap(); | ||
| debug!("{{{}}}: wait_on_channels handle {:?}", node_id, handle); | ||
| handles.push(Some(oak_runtime::Handle(handle))); | ||
| } |
There was a problem hiding this comment.
Not sure I follow – from_raw_parts isn't relevant here because we're not building a slice from a contiguous sequence of elements in memory (every ninth byte gets skipped). Added a comment.
| impl super::Node for PseudoNode { | ||
| fn start(&mut self) -> Result<(), OakStatus> { | ||
| // Clone or copy all the captured values and move them into the closure, for simplicity. | ||
| let config_name = self.config_name.clone(); | ||
| let runtime = self.runtime.clone(); | ||
| let reader = self.reader; | ||
| let thread_handle = thread::Builder::new() | ||
| .name(format!("external={}", config_name)) | ||
| .spawn(move || { | ||
| let pretty_name = format!("{}-{:?}:", config_name, thread::current()); | ||
| let factory_fn: NodeFactory = FACTORY | ||
| .read() | ||
| .expect("unlock failed") | ||
| .expect("no registered factory"); | ||
| info!( |
There was a problem hiding this comment.
Yeah, there's a certain amount of path-of-least-resistance:
- With
mainin C++, we only want explicit linkage from C++ to Rust, not vice versa (to avoid cycles) - Rust needs to up-call to C++ for pseudo-Node creation, which therefore has to happen via a function pointer.
- Any such function pointer then has to be registered via a call from C++ to Rust.
- A single such function pointer can currently be used for all pseudo-Node types, because the C++ runtime has a full copy of the configuration and so the config name is all that's needed, regardless of pseudo-Node type (no need to pass pseudo-Node-specific configuration values extracted from the
ApplicationConfiguration).
A lot of the above will change once main is in Rust, so I don't think it's worth changing things here – #724 will require a re-work of the C++ pseudo-Node registration stuff anyway (and I've added a reminder TODO).
|
This is going to be a pain to rebase, so I've pulled the preparatory commits into #836 to try to make things easier. |
|
Merged up to latest master. Things it would be nice still to fix:
|
|
Notes on This is needed as a prerequisite for generating proto code via Bazel rules, which in turn would be needed so that we don't have to check in the auto-generated Rust files, as done in this PR. In a normal values. These are inserted into the environment for the Under Bazel there's no immediate mechanism to get these environment variables set automatically. Adding the Trying to set them manually using the |
| @@ -25,5 +25,6 @@ fn main() { | |||
| "../../../../third_party/google/rpc/status.proto", | |||
| ], | |||
| &["../../../.."], | |||
| "src/proto/", | |||
There was a problem hiding this comment.
Doesn't this re-introduce the issue that everything gets rebuilt every time now, even if nothing actually changed? #510
Either way, since this seems to be against the cargo book guidelines I think we should have a TODO (and issue) to restore it to just use OUT_DIR.
| [package] | ||
| name = "oak_glue" | ||
| version = "0.1.0" | ||
| authors = ["David Drysdale <[email protected]>"] | ||
| edition = "2018" | ||
| license = "Apache-2.0" |
There was a problem hiding this comment.
Do you think it would be possible to just nest this stuff under oak_runtime::glue instead of creating another top-level crate?
There was a problem hiding this comment.
That would be possible, but I deliberately made this a separate crate to make sure this would be easy to reverse and/or remove when the C++->Rust linkage flips to a Rust->C++ linkage.
| /// Caller must ensure that the memory range [config_buf, config_buf+config_len) is | ||
| /// accessible and holds a protobuf-serialized ApplicationConfiguration message. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn glue_start( |
There was a problem hiding this comment.
(genuine question) By your definition, should any function that accepts a raw pointer and uses it for anything (which it is forced to do in an unsafe block anyways) be marked as unsafe? I think I agree with that view, though I could see arguments for both sides I guess.
| module: Arc<wasmi::Module>, | ||
| }, | ||
|
|
||
| External, |
There was a problem hiding this comment.
Could you add a comment?
Allow override of the out_dir for the code generation.
Make the generated files visible in the source tree to make it easier to perform Bazel / BUILD file integration.
Followed the instructions at https://github.com/google/cargo-raze#remote-dependency-mode for setting up cargo raze in remote dependency mode. Manual steps: - Edit cargo/Cargo.toml to list a set of [dependencies] that match the combination of those in oak_abi and oak_runtime crates, plus: - the `std` feature for `log` (and force `atomic_cas` to be set when building it) - the `simple_logger` crate - the `lazy_static` crate - Run `cd cargo && cargo raze` to generate `cargo/BUILD` and all the files in `cargo/remote/` - Check the build works with `bazel build //cargo:all` - Adjustments to versions and build flags for problematic crates.
Add BUILD files for the oak_abi and oak_runtime crates, mirroring the dependencies from Cargo.toml into "//cargo:<dep>" equivalents in the BUILD file. Check that `bazel build //oak/server/rust/oak_abi` and `bazel build //oak/server/rust/oak_runtime` work afterwards.
Use a mutable global to hold references to the Rust runtime, and to the C++ pseudo-Node factory function that's registered. Check builds with both: - cargo build --manifest-path oak/server/rust/oak_glue/Cargo.toml - bazel build //oak/server/rust/oak_glue
Convert OakRuntime to be a proxy for the Rust runtime, and convert the OakNode base class to proxy on to the C++->Rust FFI glue code.
Slim down the C++ runtime now that the Rust runtime handles messaging and Wasm nodes.
- Use a Create() factory method. - Make methods const, given that data fields don't change after construction - Don't start the Rust runtime (and its threads) until after the OakRuntime object's contents are filled in.
The BUILD files for Rust dependencies that are generated by `cargo raze` are specific to a particular configured `target` architecture triple, in our case "x86_64-unknown-linux-gnu". This means that cross-compiles don't work, in particular prost and proc-macro fail to build.
Checklist
cover any TODOs and/or unfinished work.
construction.