-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Upgrade x86_64-fortanix-unknown-sgx platform support to tier 2 #57130
Upgrade x86_64-fortanix-unknown-sgx platform support to tier 2 #57130
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
I assume that if and when this PR gets merged, it will end up riding the release train all the way to stable? Is there a way to prevent that and only produce |
c7f482f
to
0db5003
Compare
0db5003
to
d38d1e8
Compare
This was discussed in triage yesterday. There is currently no mechanism to prevent this from getting into stable. However, the infra team felt that artifact compatibility was not a Rust concern, but a platform concern. We agree and are fine with this making it into stable. This PR now adds a platform-specific toolchain version number that may used by the platform to detect incompatibilities. This PR is now ready for review. |
@Mark-Simulacrum Please let me know if there is anything blocking your review. |
|
||
# Clone LLVM and libunwind | ||
# libunwind cmake build needs cmake configuration from LLVM. | ||
git clone --depth 1 https://github.com/llvm-mirror/llvm.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, could we perhaps reuse the existing llvm submodule (src/llvm)? If not, a comment here why not would be good.
Cloning llvm twice wouldn't really be ideal (it takes a while to do so on CI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mark-Simulacrum
We found a way to use existing LLVM. Please let us know what you think about the current approach.
#Build Unwind | ||
mkdir -p build | ||
cd build | ||
cmake -DCMAKE_BUILD_TYPE="RELEASE" -DRUST_SGX=1 -G "Unix Makefiles" -DLLVM_PATH=../../llvm/ ../ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, this doesn't build LLVM, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. It does not build llvm. In fact, it needs only llvm/cmake.
9555642
to
24742f0
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Seems okay to me but I'm wondering if @alexcrichton might have a better idea for how to do it - loosely I believe we're already doing something like this somewhere but I don't remember where :) |
24742f0
to
b107c5b
Compare
Fixed tidy issues in my previous push. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me, but I think it's worthwhile to clean up the docker/scripts a bit.
To be clear, "tier 2" nowadays is "we produces binaries on a best-effort basis". As with all other platforms we reserve the right to turn this off at any point in time to land otherwise critical changes to the standard library. In practice we've only done this once or twice (adding u128 and disabling some targets is what comes to mind), but I just want to make sure that it's explicit that we're not going to provide a long-term guarantee that this target is available one every single nightly in the future.
|
||
ENV RUST_CONFIGURE_ARGS --enable-extended --enable-lld --disable-docs | ||
ENV SCRIPT python2.7 ../x.py dist --target $TARGETS | ||
ENV SCRIPT python2.7 ../x.py dist --target $TARGETS && \ | ||
cp /x86_64_fortanix_unknown_sgx/lib/* /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-fortanix-unknown-sgx/lib/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be done by rustbuild instead of having to manually add it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide any pointers on how to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can look around rustbuild for things like rsend.o and mingw, that platform and musl already copy files around like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my current push, I have followed the copy_thrid_party_objects
logic for musl
.
For this target, the Docker build process sets and ENV variable indicating source path of the libunwind.a. The boot strap code reads the same and copies the file to correct destination.
src/ci/docker/run.sh
Outdated
@@ -37,6 +37,9 @@ if [ -f "$docker_dir/$image/Dockerfile" ]; then | |||
echo "Downloaded containers:\n$loaded_images" | |||
fi | |||
|
|||
prepare_context="${docker_dir}/${image}/prepare-context.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit overly magical, could the LLVM sources just be downloaded in the container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. @Mark-Simulacrum specifically requested this change. Review c5c09da2c1f89f19554f3bfaff09f48decba25cb if you want that behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a comment about reusing git commits, which may be nice to use but I don't think it should require magical scripts like this. Everything about docker layers is already cached, so it's best to just go ahead and download separately here as it likely doesn't need the exact same revision of LLVM we use anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also missed that this was in the setup for the docker containers so thought it wouldn't be cached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have reverted the changes to copy existing llvm/cmake
into the container.
Also, 2 days back, llvm-project transitioned to monorepo. Accordingly, we have moved our libunwind changes to Fortanix's fork of the llvm-project.
In the recent push, we are using the same to build libunwind.a.
To optimise the docker build time, we are downloading source of the specific commit we need from the above port. The implementation to do the same is based on fetch_submodule
from init_repo.sh)
b107c5b
to
fb83884
Compare
Thanks for the review comments @alexcrichton and @Mark-Simulacrum.
|
@@ -65,6 +70,10 @@ ENV TARGETS=$TARGETS,wasm32-unknown-unknown | |||
ENV TARGETS=$TARGETS,x86_64-sun-solaris | |||
ENV TARGETS=$TARGETS,x86_64-unknown-linux-gnux32 | |||
ENV TARGETS=$TARGETS,x86_64-unknown-cloudabi | |||
ENV TARGETS=$TARGETS,x86_64-fortanix-unknown-sgx | |||
ENV TARGETS=x86_64-fortanix-unknown-sgx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. I had added it for faster testing. Forgot to remove it.
Pushed the fix.
src/bootstrap/compile.rs
Outdated
@@ -114,19 +108,39 @@ impl Step for Std { | |||
target, | |||
}); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in current push.
|
||
COPY scripts/sccache.sh /scripts/ | ||
RUN sh /scripts/sccache.sh | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in current push.
fb83884
to
dab6b89
Compare
dab6b89
to
99fbd1b
Compare
@bors: r+ |
📌 Commit 99fbd1b has been approved by |
…sgx-tier2_support, r=alexcrichton Upgrade x86_64-fortanix-unknown-sgx platform support to tier 2 ## Overview 1. This PR upgrades x86_64-fortanix-unknown-sgx platform support to tier 2 (std only) by setting up build automation for this target. 1. For supporting unwinding, this target needs to link to a port of LLVM's libunwind (more details could be found in #56979), which will be distributed along with the Rust binaries (similar to the extra musl objects) ### Building and copying libunwind: We have added a new build script (`build-x86_64-fortanix-unknown-sgx-toolchain.sh`) that will run while the container is built. This will build `libunwind.a` from git source. While the container is built, the persistent volumes where obj/ gets created aren't yet mapped. As a workaround, we copy the built `libunwind.a` to `obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-fortanix-unknown-sgx/lib/` after x.py runs. If any reviewer knows of a better solution, please do tell. r? @Mark-Simulacrum
☀️ Test successful - checks-travis, status-appveyor |
Pkgsrc changes: * Bump required rust version to build to 1.32.0. * Adapt patches to changed file locations. * Since we now patch some more vendor/ modules, doctor the corresponding .cargo-checksum.json files accordingly Upstream changes: Version 1.33.0 (2019-02-28) ========================== Language -------- - [You can now use the `cfg(target_vendor)` attribute.][57465] E.g. `#[cfg(target_vendor="apple")] fn main() { println!("Hello Apple!"); }` - [Integer patterns such as in a match expression can now be exhaustive.][56362] E.g. You can have match statement on a `u8` that covers `0..=255` and you would no longer be required to have a `_ => unreachable!()` case. - [You can now have multiple patterns in `if let` and `while let` expressions.][57532] You can do this with the same syntax as a `match` expression. E.g. ```rust enum Creature { Crab(String), Lobster(String), Person(String), } fn main() { let state = Creature::Crab("Ferris"); if let Creature::Crab(name) | Creature::Person(name) = state { println!("This creature's name is: {}", name); } } ``` - [You can now have irrefutable `if let` and `while let` patterns.][57535] Using this feature will by default produce a warning as this behaviour can be unintuitive. E.g. `if let _ = 5 {}` - [You can now use `let` bindings, assignments, expression statements, and irrefutable pattern destructuring in const functions.][57175] - [You can now call unsafe const functions.][57067] E.g. ```rust const unsafe fn foo() -> i32 { 5 } const fn bar() -> i32 { unsafe { foo() } } ``` - [You can now specify multiple attributes in a `cfg_attr` attribute.][57332] E.g. `#[cfg_attr(all(), must_use, optimize)]` - [You can now specify a specific alignment with the `#[repr(packed)]` attribute.][57049] E.g. `#[repr(packed(2))] struct Foo(i16, i32);` is a struct with an alignment of 2 bytes and a size of 6 bytes. - [You can now import an item from a module as an `_`.][56303] This allows you to import a trait's impls, and not have the name in the namespace. E.g. ```rust use std::io::Read as _; // Allowed as there is only one `Read` in the module. pub trait Read {} ``` - [You may now use `Rc`, `Arc`, and `Pin` as method receivers][56805]. Compiler -------- - [You can now set a linker flavor for `rustc` with the `-Clinker-flavor` command line argument.][56351] - [The mininum required LLVM version has been bumped to 6.0.][56642] - [Added support for the PowerPC64 architecture on FreeBSD.][57615] - [The `x86_64-fortanix-unknown-sgx` target support has been upgraded to tier 2 support.][57130] Visit the [platform support][platform-support] page for information on Rust's platform support. - [Added support for the `thumbv7neon-linux-androideabi` and `thumbv7neon-unknown-linux-gnueabihf` targets.][56947] - [Added support for the `x86_64-unknown-uefi` target.][56769] Libraries --------- - [The methods `overflowing_{add, sub, mul, shl, shr}` are now `const` functions for all numeric types.][57566] - [The methods `rotate_left`, `rotate_right`, and `wrapping_{add, sub, mul, shl, shr}` are now `const` functions for all numeric types.][57105] - [The methods `is_positive` and `is_negative` are now `const` functions for all signed numeric types.][57105] - [The `get` method for all `NonZero` types is now `const`.][57167] - [The methods `count_ones`, `count_zeros`, `leading_zeros`, `trailing_zeros`, `swap_bytes`, `from_be`, `from_le`, `to_be`, `to_le` are now `const` for all numeric types.][57234] - [`Ipv4Addr::new` is now a `const` function][57234] Stabilized APIs --------------- - [`unix::FileExt::read_exact_at`] - [`unix::FileExt::write_all_at`] - [`Option::transpose`] - [`Result::transpose`] - [`convert::identity`] - [`pin::Pin`] - [`marker::Unpin`] - [`marker::PhantomPinned`] - [`Vec::resize_with`] - [`VecDeque::resize_with`] - [`Duration::as_millis`] - [`Duration::as_micros`] - [`Duration::as_nanos`] Cargo ----- - [Cargo should now rebuild a crate if a file was modified during the initial build.][cargo/6484] Compatibility Notes ------------------- - The methods `str::{trim_left, trim_right, trim_left_matches, trim_right_matches}` are now deprecated in the standard library, and their usage will now produce a warning. Please use the `str::{trim_start, trim_end, trim_start_matches, trim_end_matches}` methods instead. - The `Error::cause` method has been deprecated in favor of `Error::source` which supports downcasting. [55982]: rust-lang/rust#55982 [56303]: rust-lang/rust#56303 [56351]: rust-lang/rust#56351 [56362]: rust-lang/rust#56362 [56642]: rust-lang/rust#56642 [56769]: rust-lang/rust#56769 [56805]: rust-lang/rust#56805 [56947]: rust-lang/rust#56947 [57049]: rust-lang/rust#57049 [57067]: rust-lang/rust#57067 [57105]: rust-lang/rust#57105 [57130]: rust-lang/rust#57130 [57167]: rust-lang/rust#57167 [57175]: rust-lang/rust#57175 [57234]: rust-lang/rust#57234 [57332]: rust-lang/rust#57332 [57465]: rust-lang/rust#57465 [57532]: rust-lang/rust#57532 [57535]: rust-lang/rust#57535 [57566]: rust-lang/rust#57566 [57615]: rust-lang/rust#57615 [cargo/6484]: rust-lang/cargo#6484 [`unix::FileExt::read_exact_at`]: https://doc.rust-lang.org/std/os/unix/fs/trait.FileExt.html#method.read_exact_at [`unix::FileExt::write_all_at`]: https://doc.rust-lang.org/std/os/unix/fs/trait.FileExt.html#method.write_all_at [`Option::transpose`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.transpose [`Result::transpose`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.transpose [`convert::identity`]: https://doc.rust-lang.org/std/convert/fn.identity.html [`pin::Pin`]: https://doc.rust-lang.org/std/pin/struct.Pin.html [`marker::Unpin`]: https://doc.rust-lang.org/stable/std/marker/trait.Unpin.html [`marker::PhantomPinned`]: https://doc.rust-lang.org/nightly/std/marker/struct.PhantomPinned.html [`Vec::resize_with`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.resize_with [`VecDeque::resize_with`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.resize_with [`Duration::as_millis`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_millis [`Duration::as_micros`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_micros [`Duration::as_nanos`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_nanos [platform-support]: https://forge.rust-lang.org/platform-support.html
Pkgsrc changes: * Bump required rust version to build to 1.32.0. * Adapt patches to changed file locations. * Since we now patch some more vendor/ modules, doctor the corresponding .cargo-checksum.json files accordingly Upstream changes: Version 1.33.0 (2019-02-28) ========================== Language -------- - [You can now use the `cfg(target_vendor)` attribute.][57465] E.g. `#[cfg(target_vendor="apple")] fn main() { println!("Hello Apple!"); }` - [Integer patterns such as in a match expression can now be exhaustive.][56362] E.g. You can have match statement on a `u8` that covers `0..=255` and you would no longer be required to have a `_ => unreachable!()` case. - [You can now have multiple patterns in `if let` and `while let` expressions.][57532] You can do this with the same syntax as a `match` expression. E.g. ```rust enum Creature { Crab(String), Lobster(String), Person(String), } fn main() { let state = Creature::Crab("Ferris"); if let Creature::Crab(name) | Creature::Person(name) = state { println!("This creature's name is: {}", name); } } ``` - [You can now have irrefutable `if let` and `while let` patterns.][57535] Using this feature will by default produce a warning as this behaviour can be unintuitive. E.g. `if let _ = 5 {}` - [You can now use `let` bindings, assignments, expression statements, and irrefutable pattern destructuring in const functions.][57175] - [You can now call unsafe const functions.][57067] E.g. ```rust const unsafe fn foo() -> i32 { 5 } const fn bar() -> i32 { unsafe { foo() } } ``` - [You can now specify multiple attributes in a `cfg_attr` attribute.][57332] E.g. `#[cfg_attr(all(), must_use, optimize)]` - [You can now specify a specific alignment with the `#[repr(packed)]` attribute.][57049] E.g. `#[repr(packed(2))] struct Foo(i16, i32);` is a struct with an alignment of 2 bytes and a size of 6 bytes. - [You can now import an item from a module as an `_`.][56303] This allows you to import a trait's impls, and not have the name in the namespace. E.g. ```rust use std::io::Read as _; // Allowed as there is only one `Read` in the module. pub trait Read {} ``` - [You may now use `Rc`, `Arc`, and `Pin` as method receivers][56805]. Compiler -------- - [You can now set a linker flavor for `rustc` with the `-Clinker-flavor` command line argument.][56351] - [The mininum required LLVM version has been bumped to 6.0.][56642] - [Added support for the PowerPC64 architecture on FreeBSD.][57615] - [The `x86_64-fortanix-unknown-sgx` target support has been upgraded to tier 2 support.][57130] Visit the [platform support][platform-support] page for information on Rust's platform support. - [Added support for the `thumbv7neon-linux-androideabi` and `thumbv7neon-unknown-linux-gnueabihf` targets.][56947] - [Added support for the `x86_64-unknown-uefi` target.][56769] Libraries --------- - [The methods `overflowing_{add, sub, mul, shl, shr}` are now `const` functions for all numeric types.][57566] - [The methods `rotate_left`, `rotate_right`, and `wrapping_{add, sub, mul, shl, shr}` are now `const` functions for all numeric types.][57105] - [The methods `is_positive` and `is_negative` are now `const` functions for all signed numeric types.][57105] - [The `get` method for all `NonZero` types is now `const`.][57167] - [The methods `count_ones`, `count_zeros`, `leading_zeros`, `trailing_zeros`, `swap_bytes`, `from_be`, `from_le`, `to_be`, `to_le` are now `const` for all numeric types.][57234] - [`Ipv4Addr::new` is now a `const` function][57234] Stabilized APIs --------------- - [`unix::FileExt::read_exact_at`] - [`unix::FileExt::write_all_at`] - [`Option::transpose`] - [`Result::transpose`] - [`convert::identity`] - [`pin::Pin`] - [`marker::Unpin`] - [`marker::PhantomPinned`] - [`Vec::resize_with`] - [`VecDeque::resize_with`] - [`Duration::as_millis`] - [`Duration::as_micros`] - [`Duration::as_nanos`] Cargo ----- - [Cargo should now rebuild a crate if a file was modified during the initial build.][cargo/6484] Compatibility Notes ------------------- - The methods `str::{trim_left, trim_right, trim_left_matches, trim_right_matches}` are now deprecated in the standard library, and their usage will now produce a warning. Please use the `str::{trim_start, trim_end, trim_start_matches, trim_end_matches}` methods instead. - The `Error::cause` method has been deprecated in favor of `Error::source` which supports downcasting. [55982]: rust-lang/rust#55982 [56303]: rust-lang/rust#56303 [56351]: rust-lang/rust#56351 [56362]: rust-lang/rust#56362 [56642]: rust-lang/rust#56642 [56769]: rust-lang/rust#56769 [56805]: rust-lang/rust#56805 [56947]: rust-lang/rust#56947 [57049]: rust-lang/rust#57049 [57067]: rust-lang/rust#57067 [57105]: rust-lang/rust#57105 [57130]: rust-lang/rust#57130 [57167]: rust-lang/rust#57167 [57175]: rust-lang/rust#57175 [57234]: rust-lang/rust#57234 [57332]: rust-lang/rust#57332 [57465]: rust-lang/rust#57465 [57532]: rust-lang/rust#57532 [57535]: rust-lang/rust#57535 [57566]: rust-lang/rust#57566 [57615]: rust-lang/rust#57615 [cargo/6484]: rust-lang/cargo#6484 [`unix::FileExt::read_exact_at`]: https://doc.rust-lang.org/std/os/unix/fs/trait.FileExt.html#method.read_exact_at [`unix::FileExt::write_all_at`]: https://doc.rust-lang.org/std/os/unix/fs/trait.FileExt.html#method.write_all_at [`Option::transpose`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.transpose [`Result::transpose`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.transpose [`convert::identity`]: https://doc.rust-lang.org/std/convert/fn.identity.html [`pin::Pin`]: https://doc.rust-lang.org/std/pin/struct.Pin.html [`marker::Unpin`]: https://doc.rust-lang.org/stable/std/marker/trait.Unpin.html [`marker::PhantomPinned`]: https://doc.rust-lang.org/nightly/std/marker/struct.PhantomPinned.html [`Vec::resize_with`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.resize_with [`VecDeque::resize_with`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.resize_with [`Duration::as_millis`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_millis [`Duration::as_micros`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_micros [`Duration::as_nanos`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_nanos [platform-support]: https://forge.rust-lang.org/platform-support.html
Overview
Building and copying libunwind:
We have added a new build script (
build-x86_64-fortanix-unknown-sgx-toolchain.sh
) that will run while the container is built. This will buildlibunwind.a
from git source.While the container is built, the persistent volumes where obj/ gets created aren't yet mapped. As a workaround, we copy the built
libunwind.a
toobj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-fortanix-unknown-sgx/lib/
after x.py runs.If any reviewer knows of a better solution, please do tell.
r? @Mark-Simulacrum