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

Compile with stable Rust #1372

Open
6 of 12 tasks
bstrie opened this issue Feb 11, 2022 · 15 comments
Open
6 of 12 tasks

Compile with stable Rust #1372

bstrie opened this issue Feb 11, 2022 · 15 comments
Assignees
Labels
enhancement New feature or request Epic

Comments

@bstrie
Copy link
Contributor

bstrie commented Feb 11, 2022

Make enarx and all dependencies compile on a stable release of Rust, using only stable Rust features. This may involve several approaches:

  1. Remove the usage of any unstable features that are not necessary for functioning.
  2. Pursue upstream stabilization for any unstable features that are necessary.
  3. In cases where unstable feature usage is platform-specific, use conditional compilation to limit the need for a nightly compiler to only certain platforms.
  4. Offer our own feature flags to allow users to opt-in to the use of nightly features.

Feature status

  • -Z bindeps (Cargo feature): high risk. This feature (a.k.a. "artifact dependencies") represents a substantial amount of new code in Cargo and a handful of issues have already surfaced: https://github.com/rust-lang/cargo/issues?q=is%3Aissue+is%3Aopen+artifact+label%3AZ-bindeps . Silver lining is that the fact that others are finding issues means that other people are eager to use this in earnest, which is important for eventual stabilization. The Cargo team may be reluctant to stabilize as long as outstanding issues exist. Furthermore, the Cargo team is relatively understaffed and may not have the energy necessary to pursue stabilization without significant external assistance.
  • -Z sparse-registry (Cargo feature): low risk. This is a nice-to-have for achieving free speedups in CI and during ordinary development, but not strictly necessary. It also is likely on track to stabilize in the short term. Resolved by Stabilize sparse-registry rust-lang/cargo#11224 .
  • asm_const: medium risk. The transitive blocker for this, inline_consts, is blocked on a relatively minor syntactic question. However, even this is enough to delay stabilization indefinitely. Fortunately, if syntax is truly the only remaining blocker for inline_const, then we may be able to remove it as a blocker for asm_const. The path forward here is to either resolve the syntactic question for inline_const or to convince Amanieu to decouple it from asm_const, which will require getting assurance from those behind the const machinery that the semantics are set in stone.
  • asm_sym: medium risk. Blocked on rewriting sym support for global_asm!, which is underway (Implement sym operands for global_asm! rust-lang/rust#94468) but we need to clarify whether or not that alone will suffice to unblock asm_sym. Even if so, it's unclear when that work will be completed. Resolved by build: bump Rust toolchain to 2022-11-04 #2303 .
  • naked_functions: low risk. The lang team has all but greenlit the stabilization of this feature, contingent on the resolution of a small number of open questions that should not be difficult to resolve. We are actively contributing towards stabilization here: [Tracking]: Pursue stabilization for Rust RFC 2972: "Constrained Naked Functions" #1269 Resolved by feat: remove naked functions #2350 .
  • Tier 2 x86_64-unknown-none (as a way to avoid the need for -Z build-std): low risk. This has a prominent advocate in Josh Triplett, and we can help him out by doing the labor of initiating the process. We are actively contributing towards stabilization here: Pursue promoting x86_64-unknown-none to a Tier 2 Rust platform #1528 Resolved by Promote x86_64-unknown-none target to Tier 2 and distribute build artifacts rust-lang/rust#95705 .
  • generic_const_exprs and const_mut_refs: low risk. The code making use of these features needs to be rewritten for SGX2, which is a deliverable for Enarx 0.4, so we just need to ensure that the eventual rewrite doesn't make use of these. Resolved by refactor(shim-sgx): remove unnecessary feature flags #1655 .
  • c_size_t: low risk. These features provide trivial nice-to-have type aliases. I foresee little resistance to stabilizing them, but even if they are not stabilized we could trivially remove our usage of them without difficulty.
  • core_ffi_c: low risk. Same as above. Resolved by Stabilize core::ffi:c_* and re-export in std::ffi rust-lang/rust#98315 .
  • nonnull_slice_from_raw_parts: low risk. This is a minor library addition that is already on the verge of stabilization. All that is required is for a few more libs team members to check their boxes and for a stabilization report to be filed, and the latter is something that we can contribute to move this along.
  • slice_ptr_len: low risk. At first glance this appears problematic as concerns have been raised about the feature in general, however this feature provides APIs on three types, and the outstanding concerns only apply to two of those types, whereas the third type is fine, and we're exclusively using this API on that third type. Other users have already noticed this and have split out the non-problematic API into its own feature, which is well on its way towards stabillization (Partially stabilize (const_)slice_ptr_len feature by stabilizing NonNull::len rust-lang/rust#94640) and just needs a nudge to push it over the finish line, which we can aid with.
  • slice_ptr_get: medium risk. This API is currently being employed as a way to avoid potential UB in one place in sallyport (see fix(sallyport): avoid UB by removing implicit reference from indexing with range #2069). There may be other ways to work around the issue, although given the security implications I would prefer to use the standard API if able. Stabilization of this API is highly motivated by it being possibly the only safe way to perform certain obscure unsafe code patterns, but whether or not there is appetite to stabilize it quickly is unknown. It's also possible that the language itself will change to make the use of this API unnecessary.
@bstrie bstrie added enhancement New feature or request triage Issues that need to be triaged. Epic and removed triage Issues that need to be triaged. labels Feb 11, 2022
@bstrie
Copy link
Contributor Author

bstrie commented Feb 11, 2022

For a first step we can focus on getting Enarx onto stable, although we'll eventually need to get all of its dependencies onto stable as well (after a cursory examination I think the only dependency currently making use of unstable features is Sallyport).

Here's a list of all the unstable features currently in use by Enarx. These should all eventually have their own tracking issue once some cursory investigation has been done to determine the best course of action for each feature.

  • naked_functions: The ability to use the #[naked] attribute on functions. This is very close to stabilization, which we are pursuing in [Tracking]: Pursue stabilization for Rust RFC 2972: "Constrained Naked Functions" #1269
  • asm_const: The ability to use const operands in asm blocks. In some cases workarounds may be possible without much trouble, as many of these consts are just ordinary known values like size_of::<u32>. However in other cases the workarounds would be more annoying, e.g. doing compile-time bitwise arithmetic on const bitflags. And it wouldn't be utterly trivial to just convert these to ordinary input operands, since often these asm blocks are in naked functions. We'd have to modify the naked functions to pass these values in as arguments, which is a little annoying. On the Rust side, the asm_const feature is blocked exclusively on the stabilization of the inline_const feature, and that feature is essentially complete implementation-wise but there are some remaining questions regarding how the inline const syntax interacts with macros. However, this syntactic question almost certainly doesn't apply to const operands, so I currently suspect that we could decouple asm_const from inline_const and stabilize the former relatively quickly. Rust issue: Tracking Issue for asm_const rust-lang/rust#93332
  • asm_sym: The ability to refer to Rust symbols from within asm blocks. Workarounds for this involve using #[no_mangle] and calling the unmangled symbols directly from the asm. However, in some cases we're defining the relevant symbols inside of macros, so we'd need to do hacky concat_idents shenanigans in order to get that to work properly. On the Rust side, this feature is waiting on implementation work, currently unclear on the amount of effort and time this work would take or whether anyone else is prioritizing this work. Rust issue: Tracking Issue for asm_sym rust-lang/rust#93333
  • generic_const_exprs: Nowhere near stabilizing for the foreseeable future. See this recent status update from the relevant Rust issue: Tracking issue for const generics (RFC 2000) rust-lang/rust#44580 (comment) . We will need to remove our usage of this feature. Fortunately, it only gets used in one place, and that place will likely be going away entirely when the upgrade to SGX2 is complete.
  • const_mut_refs: On the Rust side, might be possible to stabilize soon. However, it gets used in exactly the one single place that generic_const_exprs is used, so we might as well just wait for that bit of code to be removed to get rid of this feature as well.

@jarkkojs
Copy link
Contributor

jarkkojs commented Feb 17, 2022

For me this is a mandatory requirement to include Enarx to my regular kernel testing flow. Not having this makes it extremely hard to compile Enarx into initramfs bundled with the kernel image, when compiling a minimal rootfs with BuildRoot. I already have in-progress BuildRoot fork waiting for this: https://github.com/jarkkojs/buildroot-enarx.

@jarkkojs
Copy link
Contributor

jarkkojs commented Feb 17, 2022

Could we take the following soft path:

  1. If a patch is doable with stable rust with some reasonable steps, even if slightly less elegant, then that can be used as an argument in a code review.
  2. If a patch is non-doable with stable rust, or requires unreasonable amount of work, then we use unstable rust.

This way we converge to the goal but it does not require any major rework at once.

To summarize: we should consider stable Rust implementation "better" in a code review when it is possible in the first place.

@jarkkojs
Copy link
Contributor

jarkkojs commented Feb 17, 2022

For a first step we can focus on getting Enarx onto stable, although we'll eventually need to get all of its dependencies onto stable as well (after a cursory examination I think the only dependency currently making use of unstable features is Sallyport).

Here's a list of all the unstable features currently in use by Enarx. These should all eventually have their own tracking issue once some cursory investigation has been done to determine the best course of action for each feature.

Can you provide some short plain English descriptions what these are (by editing the original comment)?

@bstrie
Copy link
Contributor Author

bstrie commented Feb 18, 2022

One more thing to note for the future: as soon as rust-lang/cargo#9992 lands we'll begin using a new, unstable -Z bindeps Cargo flag, tracked at rust-lang/cargo#9096 . It's too soon to say how long it will take to stabilize once implemented, but I don't think it will take an extremely long time to stabilize.

@npmccallum
Copy link
Member

@jarkkojs can you use RUSTC_BOOTSTRAP for your kernel flow?

@jarkkojs
Copy link
Contributor

@jarkkojs can you use RUSTC_BOOTSTRAP for your kernel flow?

What is it?

@jarkkojs
Copy link
Contributor

One more thing to note for the future: as soon as rust-lang/cargo#9992 lands we'll begin using a new, unstable -Z bindeps Cargo flag, tracked at rust-lang/cargo#9096 . It's too soon to say how long it will take to stabilize once implemented, but I don't think it will take an extremely long time to stabilize.

I fully understand this. Not having that causes some awkwardness in the current codebase.

@bstrie
Copy link
Contributor Author

bstrie commented Feb 21, 2022

RUSTC_BOOTSTRAP is a secret implementation detail of the Rust compiler that allows nightly Rust to pretend to be stable Rust, which is used for the purpose of bootstrapping the standard library during the release process. It is not to be used by anyone other than the Rust compiler in any production capacity; if I were a code auditor I would reject any codebase that used it.

@bstrie
Copy link
Contributor Author

bstrie commented Mar 1, 2022

Regarding -Z bindeps, one of our prerequisites for using artifact dependencies is to port the shims to x86_64-unknown-none (#1461, #1462), and that target itself is essentially unstable as it currently requires users to use the unstable -Z build-std Cargo flag. Thus compiling with stable Rust will require elevating that target to Tier 2 ( https://doc.rust-lang.org/nightly/rustc/platform-support.html#tier-2 ); this currently has no tracking issue in the Rust repo but I'll ping some people about it.

@bstrie
Copy link
Contributor Author

bstrie commented Mar 3, 2022

We may also begin relying on a new feature, core_ffi_c. See the rationale at #1462 (comment) .

@bstrie
Copy link
Contributor Author

bstrie commented Apr 5, 2022

I have confirmed that the proposed #1608 removes all uses of the generic_const_exprs and const_mut_refs feature flags.

@jarkkojs
Copy link
Contributor

jarkkojs commented Apr 6, 2022

I have confirmed that the proposed #1608 removes all uses of the generic_const_exprs and const_mut_refs feature flags.

Not using them. Heap is created dynamically.It allows to manager virtual memory areas with their real addresses. @npmccallum i.e. I'm using ledger with real coordinates now.

@bstrie
Copy link
Contributor Author

bstrie commented Apr 7, 2022

I've also just noticed that one of the tests has #![cfg_attr(target_os = "wasi", feature(wasi_ext))]. I'll inquire as to the status of stabilization for the wasi_ext feature, but for now this is only a minor concern since it's just a test.

@bstrie
Copy link
Contributor Author

bstrie commented Apr 7, 2022

While poking around in our dependencies it appears as though the only external dependency (i.e. not in the Enarx repo) that makes use of unstable features is sallyport. I've identified two new unstable features used by sallyport, nonnull_slice_from_raw_parts and slice_ptr_len. Both of these features appear to be low risk. I've updated the checklist in the main issue to add these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Epic
Projects
Status: Backlog
Development

No branches or pull requests

3 participants