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

Fix linux 'others' #1431

Closed
wants to merge 1 commit into from
Closed

Fix linux 'others' #1431

wants to merge 1 commit into from

Conversation

dingelish
Copy link
Contributor

Hi there,

We observed a bunch of CI failure due to a recent breaking change in commit 65f23e6 . It changed the behavior of linux but not musl or gnu. In src/unix/linux_like/linux/mod.rs

@@ -2721,9 +2647,16 @@ cfg_if! {
     if #[cfg(target_env = "musl")] {
         mod musl;
         pub use self::musl::*;
+    } else if #[cfg(target_env = "gnu")] {
+        mod gnu;
+        pub use self::gnu::*;
-    } else if #[cfg(any(target_arch = "mips",
-                        target_arch = "mips64"))] {
-        mod mips;
-        pub use self::mips::*;
-    } else if #[cfg(any(target_arch = "s390x"))] {
-        mod s390x;
-        pub use self::s390x::*;
-    } else {
-        mod other;
-        pub use self::other::*;
     }
 }

In the past, others catches everything except for musl mips mips64 s390x, but now it no longer exists. So this commit removes support to others. However, many platforms are linux but not in musl or gnu and they depends on this branch. This PR helps fix it. We'd like to see an immediate update on crates.io with version bumped to 0.2.60 to solve a lot of building failures... Thanks!

Best,
Yu

@rust-highfive
Copy link

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

In the past, others catches everything except for musl mips mips64 s390x, but now it no longer exists

We currently support the following C standard libraries: glibc, musl, newlib, sgx, and uclibc. All other libraries are still supported as long as they don't use anything that's specific to any of these.

Defaulting to any particular standard library is wrong.

We observed a bunch of CI failure due to a recent breaking change in commit 65f23e6

We support all tier-1 targets. We also support all tier-2 and tier-3 targets. We actually build all those targets to make sure that they build correctly, and for many of them, we also run our tests.

Anything that's not in there, is not supported, and therefore, breaking it is not a breaking change.

If you want your target to be supported, please fill a PR adding support for it to our CI environment.

However, many platforms are linux but not in musl or gnu and they depends on this branch.

We do test ~50 linux targets, using glibc, musl, uclibc, newlib, sgx, and others. So how many platforms are you targeting that broke, and could you list them ?


I'm closing this PR because this is not a proper fix. You might want to open an issue, to report your problem first, help us understand it, so that we can help you landing a proper fix, in such a way that your targets become properly supported and never break again.

@gnzlbg gnzlbg closed this Jul 9, 2019
@dingelish
Copy link
Contributor Author

Many of customized sysroot which only "slightly extend" the behavior of default gnu/musl depends on the official libc and leverage xargo to build their sysroot. You have the right to do not support them. But the breaking change would lead to fragmentation of libc crates -- every vendor needs to maintain a fork. This is absolutely making the whole Rust ecosystem very unhealthy.

Besides, this problem roots from the relationship between cargo and libstd/libc. cargo is tightly bounded to the default libstd/libc, and the std aware cargo RFC is being discussed here. As of today, the embedded guys use Rust+xargo on their platforms. And these targets may never be merged into the upstream librustc_target. So this official libc would no longer support them -- if this is what you mean.

If we have the std aware cargo ready, everything will be simple. However, the progress of that RFC is too slow, making people doubt on the Rust core team and the Rust ecosystem. You may know that Facebook abandoned cargo -- they only use rustc.

If I were you, I would not abandon the "feature" of the aforementioned support to 'others'. It benefits lots of Rust embedded system programmers who are reusing your codes, instead of maintaining a fork.

I can create an issue on this. But I wonder if needs a year to be discussed and solved.

Besides, my problem is to use this libc on a customized sysroot, which has target_env = "sgx", target_vendor = "mesalock", and target_os = "linux". So it is not a gnu platform. But it reuses all definitions of gnu. Many blockchain projects depend on this environment now.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

Many of customized sysroot which only "slightly extend" the behavior of default gnu/musl depends on the official libc and leverage xargo to build their sysroot. You have the right to do not support them. But the breaking change would lead to fragmentation of libc crates -- every vendor needs to maintain a fork. This is absolutely making the whole Rust ecosystem very unhealthy.

If you only slightly extend glibc, why isn't your target_env = "gnu" ?

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

Besides, my problem is to use this libc on a customized sysroot, which has target_env = "sgx", target_vendor = "mesalock", and target_os = "linux". So it is not a gnu platform. But it reuses all definitions of gnu. Many blockchain projects depend on this environment now.

We do support target_env = "sgx": https://github.com/rust-lang/libc/blob/master/src/lib.rs#L128

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

@jethrogb Do you think it would be problematic to move sgx up on the list, so that it overrides #[cfg(unix)] - that might fix this users' problem.

@dingelish
Copy link
Contributor Author

dingelish commented Jul 9, 2019

The 'sgx' you referred to is fortanix's Rust-SGX. Not ours.

You can see how many projects depends on our rust-sgx-sdk using xargo.

@dingelish
Copy link
Contributor Author

For example, you may have heard about tvm, the most popular deep learning compiler stack. Its SGX support depends on our rust-sgx-sdk and xargo.

Of course I can maintain a fork of libc and fix all of the above projects by adding a [patch] section to let them use my fork (only 4 lines added). But I think it's really unnecessary.

@dingelish
Copy link
Contributor Author

I recommend keeping this 'hidden feature' until the std aware RFC becomes true.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

@dingelish I understand that you have pushed an unsupported solution to production, which is now broken, and that this is causing you trouble. I'm sorry that your use case broke, and want to help you fix it.

You are going to have to fix this yourself, and that fix needs to satisfy a minimum quality bar. It isn't obvious to me that this fix is it. We can help you shipping a proper fix, but as mentioned, for this we have to understand your problem, which requires a detailed bug report of all the many targets that got broken due to this, and what those targets are, how are they being build, what do your custom target specification files look like, etc.

I recommend you to focus on that so that we can deliver a good fix quickly. I also recommend you to stop trying to argue that, because a lot of Rust code is now broken, we have to push a poor-quality fix. Other Rust users trust us not to do that, and I don't think sacrificing that trust is worth it. Since you are a Rust user yourself, I hope you are able to sympathize with this, even though right now this policy might be causing you a headache.

If you need a temporary solution, I'd recommend fixing your dependency to an older libc version that is known to work.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

FWIW, we'd love to come up with a solution that turns your target into a fully supported Rust target, just like it has been done for the fortanix-sgx target. That would benefit you and your users significantly.

@dingelish
Copy link
Contributor Author

Thanks for your advice.

I really like your idea of "a fully supported Rust target". Could I have some more comments from you on this?

I've been trying to make SGX "a fully supported Rust target" for years. But it seems impossible because SGX has no ability to make syscall and perhaps never support "creating new threads". The libstd requires every platform to provide thread/process, even if it is not supported (using unsupported!. In the current design of libstd/libc/cargo combination, we can always make a Rust crate compile using fortanix's solution, but it conflicts with SGX's guarantees on trusted computing. Nobody would know if any codes in a large and deep dependency tree triggers fork in runtime, which finally causes a panic in SGX, violating the concept of trusted computing.

Our SDK solves it by providing a limited libstd, which depends on your libc using that "hidden feature". In this customized libstd, unsupported functions are removed. So if any codes invoke something like "create a thread" would not compile. In this way, we avoid runtime panic triggered by unsupported!.

Appreciate your recommendations.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

The plan is for the portability lint to warn when calling an unimplemented! API, and those warnings can be turned into compile-time errors.

Adding a new target typically starts with adding a target-description file, and then adding libcore support. After that, adding libc and liballoc support can be done. Whether you then want to add libstd support or not, is up to you. Many targets don't do this.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

Also, a run-time panic is safe. If you don't want unwinding on fatal errors, you can just use panic=abort. And if you want to require that something doesn't panic at compile-time, you can use the no_panic crate for that.

@dingelish
Copy link
Contributor Author

Yes. It is safe. But it is unexpected. Trusted computing is something about "expectation". Wikipedia says

With Trusted Computing, the computer will consistently behave in expected ways, and those behaviors will be enforced by computer hardware and software.

When porting something into SGX, runtime panic on unsupported! is considered to be unexpected, because we want it to work, instead of knowing that it is "panicking because it is not supported". I think no_panic is not a solution because it just "not panic".

Let's look at the tier 1 definition. It says "Tier 1 platforms can be thought of as "guaranteed to work". This is absolutely not enough for trusted computing, which should be something like "Tier 0 platforms can be thought of as “guaranteed to work as expected in any circumstances".

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

This is absolutely not enough for trusted computing, which should be something like "Tier 0 platforms can be thought of as “guaranteed to work as expected in any circumstances".

Do you have an example of a system that is guaranteed to work as expected in any circumstances ?

@dingelish
Copy link
Contributor Author

we just open sourced the solution MesaTEE, which guarantees to work as expected in any circumstances.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

Is it formally verified? How does it work around hardware bugs in intel sgx implementations, the formal verifiers, the models their build upon, etc. ?


Anyways, your current solution isn't even Tier 4, and right now, it doesn't even build (too soon? :P). My recommendation is for you to work your target up the tiers, first by adding libtarget support, and then libcore support. That would be enough to make it Tier 2, and ship a rust-src with libcore via rustup. That would allow us to build it here without even have to use xargo, which would allow you to add libc support, and guarantee that it builds. With a bit more work, you can also run libc tests, so that you can guarantee that the FFI bindings are at least correct. From there, you could add liballoc support, and stop there. If you want to maintain your own libstd, you can do that, but doing so is easier when you don't have to also maintain all the things that libstd builds on.

Even if you don't ship it via rustup, libtarget support would allow you to test to this crate, using xargo. Guaranteeing that future changes here do not break your crate.

@dingelish
Copy link
Contributor Author

I don't think there is an end on the discussion of "expected" or "formal verification". I have years of experience in verification and validation. And the destination of such questions would always be discussing the free will -- is the code is exactly the code I want to write? If we cannot prove it, how could we get closer to trust?

Yes. My target is yet not even Tier 4. Because the current structure of libstd is even farther away from being trusted. Everyone who uses the structure of the default libstd is facing the same problem. Higher tier always means tighter bindings to the default libstd, which contradicts with the basics of trusted computing.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

It is possible to have a Tier-1 target that doesn't have a libstd.

@dingelish
Copy link
Contributor Author

I really really would like to see a tier-1 no_std target.

My opinion would be: libcore + liballoc based environment is much more reliable than libstd based environment. Since libstd could be tier1, its subset should be tier1 as well.

@jethrogb
Copy link

jethrogb commented Jul 9, 2019

@jethrogb Do you think it would be problematic to move sgx up on the list, so that it overrides #[cfg(unix)] - that might fix this users' problem.

Not sure if I still need to answer this. I don't think it's problematic to the fortanix-sgx target in particular, but it seems strange to me to have a target that doesn't have basic std support be cfg(unix). cfg(unix) implies certain OS support and it sounds like that's not going to be there.

portability lint

Just FYI, this is RFC 1868, which has been accepted, but I don't think there's any implementation work done.

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

Successfully merging this pull request may close these issues.

4 participants