Add SGX C types

[gnzlbg]

cc @jethrogb

[rust-highfive]

r? @alexcrichton

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

[gnzlbg]

This avoids duplicating all the common libc code, adds the SGX C types, and also avoids having wasm32-unknown-unknown as an exception. Basically, there is now just a top level cfg_if! that includes what's relevant for each target, and if no target matches, the library is almost empty.

Almost empty because we have to include the macros module for cfg_if, and we are also unconditionally including the dox module (which TBH don't really understand why all of it is necessary).

[alexcrichton]

I would personally prefer to land #1124 myself. I realize the libc crate is a bit backwards from traditional development, but I feel like it has really good reasons for doing so. Binding all platforms ever is basically an insanely hard task, and we want to keep things as absolutely simple as possible to optimize for readers of this crate.

The most common operation for libc is to add a new platform or add a new API. If users have to follow a long checklist of where to add an API it gets pretty difficult and very tangled very quickly. Having a dead-simple rule of thumb makes it trivial to know where to add an API, and with the verification on CI we can't get additions wrong.

Due to the nature of inheritance of APIs are you move up the module hierarchy to the root, adding a new platform is often a nontrivial task because APIs need to be duplicated into submodules as they can no longer be inherited on one platform or another. This duplication is fine, though, because it's still easy to read the crate and modify it, and all bindings are verified to be correct.

Historically I've found that 100% of the time any attempt to be more clever by sharing various subsets of APIs has ended in tears long-term. Sticking to a simple rule has been tried-and-true for quite some time now with libc.

For all that I'd prefer to go the route of #1124 which preserves the current organization of libc, where platforms inherit from modules as they move up the hierarchy, and that's basically it. We could reorganize the entire tree into a root node with "sgx" and "non-sgx" as child nodes to improve sharing as well, but I could go either way on that.

[gnzlbg]

I'd be fine with copy pasting the libc module into the submodules and remove it. But I'd prefer to have a single cfg_if at the top-level (like in this PR) to make things clearer.

For example, if you move things into submodules like in #1124 , all tests pass, but we don't have any tests checking for API removal, and IIUC #1124 removes all the stuff in the libc module of this PR from targets like emscripten, etc. These removals are not clear from the #1124 PR because what is compiled for which target is not clear (and nothing test for it on any target).

In fact, looking at this PR, I think it is wrong. We need an } else if wasm32-unknown-unknown { in the top-level cfg_if that is empty, and then the fallback case should include everything from the libc module as well. Otherwise we would be introducing a backwards incompatible change for all targets that do not match any of the ones in the cfg if, like #1124 does.

[alexcrichton]

Oh sorry yeah I'm not saying that we should take literally #1124, but I want to avoid adding multiple roots or multiple children for a submodule because it sets a very dangerous precedent that makes it very difficult to modify the library in the future.

It's true that we don't have tests for removal of APIs, because that's a major project that requires a huge amount of effort and really doesn't have that much benefit where we can otherwise just be vigilant during reviews.

[gnzlbg]

I'm not sure I fully understand what you mean by multiple roots and multiple children, but I don't really mind the duplication either as long as it is clear where to add what for which targets.

It's true that we don't have tests for removal of APIs, because that's a major project that requires a huge amount of effort and really doesn't have that much benefit where we can otherwise just be vigilant during reviews.

I'll check cargo semverver out and see if I can set it up for some targets at least. Should be able to detect the most common API breaking changes.

[alexcrichton]

Er sorry, I mean that with respect to the module hierarchy. Currently for any one platform there's just a straight line of a module hierarchy, which makes it easy to know what's included, but with a PR like this there's optionally two different child nodes of the root (the ctypes/libc modules)

[gnzlbg]

I am not sure I understand. On the root there is only a cfg_if for each group of targets, and then it goes down to the concrete targets. Almost all targets share the code in ctypes (e.g. c_void, ...) and also almost all targets share the most common C standard library functions.

We could copy paste these into each submodule, and also, in the "default" branch of the cfg_if. I don't mind much, but doing so makes PRs like #1130 much harde, where the author just added a bunch of libc functions to the common ones, and then after seeing the CI results left some there for all targets, and moved some others only to the targets that they support.

[jethrogb]

I think what @alexcrichton is saying is that there are multiple locations in lib.rs with this line:
mod libc;
This is not a very standard way of building your module hierarchy.

[alexcrichton]

Indeed yeah, but not only is it odd to have multiple mod libc; but this is setting a very dangerous precedent. What if in the future we have functions shared on wasm and unix, but not windows? Do we add a wasm_and_unix.rs and add the functions there?

The libc crate embraces duplication, it really is just easier to manage over time. Deduplication is a local optimum that is nigh impossible to maintain long term as more platforms are added

[gnzlbg]

So I'll update this PR with that, but I would prefer not to merge until we are sure that we are not removing APIs from any cfg node.

[alexcrichton]

Ok! What do you think about landing this in the meantime and we can verify afterwards?

Also FWIW I think that it's probably fine to move definitions like int8_t and c_void to the root of the crate (even for the wasm/sgx targets), as those type definitions, if they work on a C compiler, are fundamentally guaranteed to the be that definition

[gnzlbg]

@bors: r+

---

I'll send a different PR moving those to the root.

[bors]

📌 Commit 5c1a6b8 has been approved by gnzlbg

[bors]

⌛ Testing commit 5c1a6b8 with merge 5b40375...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: gnzlbg
Pushing 5b40375 to master...