Support stable rust? - Githubissues

xu-cheng commented 3 years ago

Why nightly rust is needed? Using nightly rust has several major disadvantages:

Need to be constantly updated to keep sync with upstream. Currently, the pinned nightly version (1.49.0 nightly-2020-10-25) is already lagged behind the current stable rust (1.49.0 2020-12-29).
Nightly rust is of course not stable and may be error prone.

Can stable rust be supported, specially if the enclave code is written in no_std?

dingelish commented 3 years ago

Hi @xu-cheng ,

Rust builds itself by using beta toolchain with backdoor environment variable RUSTC_BOOTSTRAP=1, which enables all nightly feature gates in beta channel. It's almost equivalent to nightly compiler. As a consequence, using stable channel Rust compiler with RUSTC_BOOTSTRAP=1 feature is doable, but does not benefit at all.

dingelish commented 3 years ago

If you look at xargo and cargo-xbuild, you'll find that the community has reached to consensus to build stds with nightly toolchain

https://github.com/rust-osdev/cargo-xbuild/pull/53

xu-cheng commented 3 years ago

As far as I understand, there are two ways to use this SDK to write enclave codes: (a) build SGX's version of the std and use it normally; (b) write the enclave codes in no_std.

Can stable rust be used if the second way is used? Here we don't need to deal with xcargo or std aware cargo. Or am I understanding incorrectly?

dingelish commented 3 years ago

As far as I understand, there are two ways to use this SDK to write enclave codes: (a) build SGX's version of the std and use it normally; (b) write the enclave codes in no_std.

Yes you are correct. But both of them are logically building a sysroot, so I don't recommend to use stable Rust.

Can stable rust be used if the second way is used? Here we don't need to deal with xcargo or std aware cargo. Or am I understanding incorrectly?

Yes you can. Just make sure ${projectroot}/rust-toolchain 's content is something like stable-2020-xx-xx or stable and export RUSTC_BOOTSTRAP=1. I'm not sure if current code can be built successfully. But if you need, I can create a branch to support you. Please specify the stable toolchain you want to use and I'd be working on that.

xu-cheng commented 3 years ago

FYI, you may want to look at https://github.com/mobilecoinfoundation/mobilecoin/tree/master/sgx, which is essentially a fork of this project. However, it introduces several major differences in design.

Being lightweight and only support no_std.
It completely avoids forking and patching common third party crates. IMHO, these patched crates are red herring in terms of maintenance, stability, or even security (as bug fixes in upstream may not be applied in time). Not mentioning it splits the eco systems.
It replaces the Makefile build system with rust crate, which seems to be more idiomatic.

xu-cheng commented 3 years ago

Yes you can. Just make sure ${projectroot}/rust-toolchain 's content is something like stable-2020-xx-xx or stable and export RUSTC_BOOTSTRAP=1. I'm not sure if current code can be built successfully.

Thanks, I will try and report back.

But if you need, I can create a branch to support you. Please specify the stable toolchain you want to use and I'd be working on that.

I don't think creating a separated branch is a good idea. Would it be possible to officially support this no_std + stable rust approach? If so, a CI workflow can be created to test this approach and ensure it working in the future.

mssun commented 3 years ago

@xu-cheng @dingelish, I found that the mentioned mobilecoinfoundation/mobilecoin project is under GPLv3 License.

Please don't read and refer to any GPL-licensed code. Thank you so much.

Since we are an Apache Incubator project and under the Apache license, please read this article about GPL-compatibility: https://www.apache.org/licenses/GPL-compatibility.html

xu-cheng commented 3 years ago

Please don't read and refer to any GPL-licensed code. Thank you so much.

Sorry, I didn't realize there is a license incompatibility. I merely mention it to point a different design.

mssun commented 3 years ago

That's OK.

You may look at this issue #311 about a better implementation of the SDK using std-aware Cargo.

dingelish commented 3 years ago

thanks @xu-cheng for the pointer!

the sad fact is that: people wants std here :-( that's why things are so complicated. and std-aware cargo is sometimes buggy :-( only no_std make things clean and tidy. but we don't have this choice.

I'd try to support both stable and nightly in the same branch. but I do have users who wants to always keep updated with latest nightly. and recent frequent breaking changes in libcore::alloc make things much more complicated than before. I want to make every one happy but it's bit of hard :-(

xu-cheng commented 3 years ago

Thanks, I will try and report back.

Ok, I tested a bit and got the following errors:

error[E0658]: auto traits are experimental and possibly buggy
   --> /<snip>/rust-sgx-sdk/sgx_tstd/src/panic.rs:118:1
    |
118 | pub auto trait UnwindSafe {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #13231 <https://github.com/rust-lang/rust/issues/13231> for more information
    = help: add `#![feature(optin_builtin_traits)]` to the crate attributes to enable

error[E0658]: auto traits are experimental and possibly buggy
   --> /<snip>/rust-sgx-sdk/sgx_tstd/src/panic.rs:131:1
    |
131 | pub auto trait RefUnwindSafe {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #13231 <https://github.com/rust-lang/rust/issues/13231> for more information
    = help: add `#![feature(optin_builtin_traits)]` to the crate attributes to enable

error: aborting due to 2 previous errors

$ rustc --version
rustc 1.49.0 (e1884a8e3 2020-12-29)

I'd try to support both stable and nightly in the same branch. but I do have users who wants to always keep updated with latest nightly. and recent frequent breaking changes in libcore::alloc make things much more complicated than before. I want to make every one happy but it's bit of hard :-(

Understood. Thanks for creating this SDK. If you want to support both versions of Rust, it may be a good idea to setup CI to test them periodically.

xu-cheng commented 3 years ago

Rust builds itself by using beta toolchain with backdoor environment variable RUSTC_BOOTSTRAP=1, which enables all nightly feature gates in beta channel. It's almost equivalent to nightly compiler. As a consequence, using stable channel Rust compiler with RUSTC_BOOTSTRAP=1 feature is doable, but does not benefit at all.

Thinking a bit more. I think this is right that stable + RUSTC_BOOTSTRAP=1 is not truly stable rust. The point of stable rust is the assurance on its stability that updating rust will not break the codes. So I guess it defeats my original purpose.

dingelish commented 3 years ago

Hi @xu-cheng

please checkout beta/stable support at https://github.com/apache/incubator-teaclave-sgx-sdk/tree/stable-support

xu-cheng commented 3 years ago

please checkout beta/stable support at https://github.com/apache/incubator-teaclave-sgx-sdk/tree/stable-support

Thanks. It works.

xu-cheng commented 3 years ago

@dingelish Any chance to merge https://github.com/apache/incubator-teaclave-sgx-sdk/tree/stable-support into the master branch?

Furthermore, is it possible to bump the compiler support to the latest nightly?

I saw the following error and warning when compiling with the latest nightly.

error[E0432]: unresolved import `core::alloc::AllocRef`
  --> /[snip]/incubator-teaclave-sgx-sdk/sgx_alloc/src/system.rs:26:17
   |
26 |     AllocError, AllocRef, GlobalAlloc, Layout,
   |                 ^^^^^^^^ no `AllocRef` in `alloc`

warning: use of deprecated associated function `std::sync::atomic::AtomicI32::compare_and_swap`: Use `compare_exchange` or `compare_exchange_weak` instead
  --> /[snip]/incubator-teaclave-sgx-sdk/sgx_urts/src/event.rs:42:32
   |
42 |                     self.event.compare_and_swap(-1, 0, Ordering::SeqCst);
   |                                ^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

celaus commented 3 years ago

Hey - just to emphasize, a compiler update for the SDK is really, really crucial moving forward.

https://github.com/rust-lang/rust/releases 1.50 introduced const generics which - since it's considered stable - will make its way into all other crates down the line, since it's a very handy upgrade. Case in point, arrayvec already made the switch: https://docs.rs/arrayvec/0.7.0/arrayvec/#rust-version

This means that whether inside or outside the enclave, we'd be locked to crate versions that build with 1.49. What's blocking the move to stable Rust exactly?

xu-cheng commented 3 years ago

Hi @dingelish,

I am wondering whether it is possible to update this SDK to support the latest version of stable or nightly Rust. This SDK really needs update to support mini const generics.

celaus commented 3 years ago

Hey everyone - I created a quick fix for using Rust 1.55 nightly. Use at your own peril 😅 PR #349

mssun commented 3 years ago

Another solution to supporting stable rust: https://www.crowdsupply.com/sutajio-kosagi/precursor/updates/adding-rust-stable-libstd-support-for-xous

In a nutshell, we can put compiled std rlib to sysroot.

apache / incubator-teaclave-sgx-sdk

Support stable rust? #315