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

std: Remove rust_builtin C support library #30175

Merged
merged 1 commit into from
Dec 22, 2015

Conversation

alexcrichton
Copy link
Member

All these definitions can now be written in Rust, so do so!

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@alexcrichton
Copy link
Member Author

r? @brson

I have a feeling this will break android, but I'm curious what CI will say. Also cc @dhuseby and @semarie, I tried to make sure that this doesn't break the BSD builds but it may as we don't have CI there.

@rust-highfive rust-highfive assigned brson and unassigned aturon Dec 2, 2015
@brson
Copy link
Contributor

brson commented Dec 2, 2015

@alexcrichton Can you link me to the libc changes this pulls in? I'm curious what was involved.

@alexcrichton
Copy link
Member Author

So far (although this is likely to grow to be a longer list) it required 2 changes:

(the latter of which I now realize still needs to land...)

@steveklabnik
Copy link
Member

😍

@semarie
Copy link
Contributor

semarie commented Dec 3, 2015

@alexcrichton thanks to Cc me. currently, it breaks at least openbsd.

I have manually applied the patch (so I could also had done mistakes).
And while building libstd:

CFG_LLVM_LINKAGE_FILE=/home/builder/build/openbsd-master/build/x86_64-unknown-openbsd/rt/llvmdeps.rs LD_LIBRARY_PATH=/home/builder/build/openbsd-master/build/x86_64-unknown-openbsd/stage0/lib:$LD_LIBRARY_PATH   x86_64-unknown-openbsd/stage0/bin/rustc --cfg stage0 -Z print-link-args  -O --cfg rtopt -C prefer-dynamic -C no-stack-check --target=x86_64-unknown-openbsd  -W warnings -L "x86_64-unknown-openbsd/rt" -L native="/opt/llvm-cde1ed3196ba9b39bcf028e06e08a8722113a5cb/lib"     -L "/usr/local/lib/gcc/x86_64-unknown-openbsd5.8/4.9.3/../../../" --out-dir x86_64-unknown-openbsd/stage0/lib/rustlib/x86_64-unknown-openbsd/lib -C extra-filename=-71b07a99 ../src/libstd/lib.rs
../src/libstd/sys/unix/fs.rs:220:13: 220:34 error: failed to resolve. Use of undeclared type or module `slice` [E0433]
../src/libstd/sys/unix/fs.rs:220             slice::from_raw_parts(self.entry.d_name.as_ptr() as *const u8,
                                             ^~~~~~~~~~~~~~~~~~~~~
../src/libstd/sys/unix/fs.rs:220:13: 220:34 help: run `rustc --explain E0433` to see a detailed explanation
../src/libstd/sys/unix/fs.rs:220:13: 220:34 error: unresolved name `slice::from_raw_parts` [E0425]
../src/libstd/sys/unix/fs.rs:220             slice::from_raw_parts(self.entry.d_name.as_ptr() as *const u8,
                                             ^~~~~~~~~~~~~~~~~~~~~
../src/libstd/sys/unix/fs.rs:220:13: 220:34 help: run `rustc --explain E0425` to see a detailed explanation
../src/libstd/sys/unix/os.rs:207:24: 207:44 error: unresolved name `libc::KERN_PROC_ARGS` [E0425]
../src/libstd/sys/unix/os.rs:207                        libc::KERN_PROC_ARGS,
                                                        ^~~~~~~~~~~~~~~~~~~~
../src/libstd/sys/unix/os.rs:207:24: 207:44 help: run `rustc --explain E0425` to see a detailed explanation
../src/libstd/sys/unix/os.rs:209:24: 209:44 error: unresolved name `libc::KERN_PROC_ARGV` [E0425]
../src/libstd/sys/unix/os.rs:209                        libc::KERN_PROC_ARGV];
                                                        ^~~~~~~~~~~~~~~~~~~~

I can't check deeper today, but I will take a look tomorrow.

@alexcrichton
Copy link
Member Author

Thanks @semarie! I've added the imports for slice and I'm adding the constants to libc in rust-lang/libc#91.

@brson
Copy link
Contributor

brson commented Dec 3, 2015

@alexcrichton I think I would like the source to explicitly separate the cases where the cast is needed there, via cfg with a FIXME - it seems like it must be a bug right?

@cuviper
Copy link
Member

cuviper commented Dec 3, 2015

It's a bad thing if these functions ever truncate values for DirEntry, MetadataExt, etc. On Android this comes up for quite a few fields, d_ino, d_off, st_ino, st_size, st_blocks, and more, where stat and dirent have 64-bit types despite their 32-bit ino_t, off_t, etc. On Linux it will also come up when I switch to LFS stat64 etc.

I think we should either fake the os::raw types to sufficient 64-bit sizes, which means they won't match the libc types of the same name, or we should change return values to reflect the actual types at hand, which means this will vary by platform. e.g. on Android DirEntry::ino() -> u64 to match d_ino, instead of -> raw::ino_t.

@alexcrichton
Copy link
Member Author

I've pushed a commit to remove the cast, and I just started updating the ino_t type in std::os::raw to reflect what's actually returned (e.g. the larger type).

That's a little controversial to me, however, as the types in std::os::raw are basically becoming lies. I'm gonna tag this with T-libs to discuss that a little more, especially with respect to the other values inside of std::os::raw and 64-bit stat support on Linux.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 3, 2015
@alexcrichton
Copy link
Member Author

And to make my concerns a little more concrete, here's what I'm thinking:

  • The return value of ino is a good example where some platforms store the field in the struct as the literal d_ino type, but other platforms (like android) store a different type (e.g. u64) and the actual d_ino type is different (e.g. u32). This means that if we can't make d_ino the "FFI compatible type" as well as the right-width type for the purpose at hand.
  • Some platforms like Linux can have the meaning of types like off_t change based on #define directives in play. For example with LFS (large file support) it will become 64-bits on 32-bit platforms, but otherwise it's 32-bits on 32-bit platforms. This also affects the stat type as well. Like with the previous bullet, it's impossible to provide "FFI compatible types" here as well as retaining the same name in all situations.

Overall I would propose a plan that looks like:

  1. Remove all symbolic names for types on all platforms. Concretely mark them all #[rustc_deprecated] and #[doc(hidden)] eventually in the future.
  2. Change basically everything to return u64 or i64 depending on signedness.
  3. Add a whole mess of extension traits to access all the lowered fields of stat on all platforms.

This will separate us from the C names as well as how they may change depending on how C code is compiled, and it means that we are no longer providing types that are "FFI compatible" as it doesn't always work out.

For now we can probably leave the basic integer definitions (e.g. c_void, c_int, and such) as not deprecated, but I'd also be tempted to just deprecate all of them as well...

@cuviper
Copy link
Member

cuviper commented Dec 3, 2015

👍 for hiding these and going all 64-bit.

Keeping fundamental C types (c_void, c_int, c_char, et al) sounds sane, then leave all the derived types to libc as much as possible

@alexcrichton
Copy link
Member Author

re-r? @brson

I'd like to move forward on this rather than wait for dealing with questions about other types in play here. Along those lines this patch now preserves the same behavior we have today.

@alexcrichton alexcrichton removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 16, 2015
@bors
Copy link
Contributor

bors commented Dec 17, 2015

☔ The latest upstream changes (presumably #30325) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 19, 2015

☔ The latest upstream changes (presumably #30381) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 20, 2015

☔ The latest upstream changes (presumably #30454) made this pull request unmergeable. Please resolve the merge conflicts.

@brson
Copy link
Contributor

brson commented Dec 22, 2015

r=me

@bors
Copy link
Contributor

bors commented Dec 22, 2015

⌛ Testing commit 606369f with merge 1e47505...

@bors
Copy link
Contributor

bors commented Dec 22, 2015

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member Author

@bors: r=brson b1e0f1a

@bors
Copy link
Contributor

bors commented Dec 22, 2015

⌛ Testing commit b1e0f1a with merge 727d7a0...

@bors
Copy link
Contributor

bors commented Dec 22, 2015

💔 Test failed - auto-linux-64-x-android-t

All these definitions can now be written in Rust, so do so!
@alexcrichton
Copy link
Member Author

@bors: r=brson 2f42ac4

bors added a commit that referenced this pull request Dec 22, 2015
All these definitions can now be written in Rust, so do so!
@bors
Copy link
Contributor

bors commented Dec 22, 2015

⌛ Testing commit 2f42ac4 with merge 5178449...

@bors bors merged commit 2f42ac4 into rust-lang:master Dec 22, 2015
@alexcrichton alexcrichton deleted the less-c-code branch December 22, 2015 17:02
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 22, 2015
The corresopnding C file was removed in rust-lang#30175 and looks like I forgot to remove
the header as well.
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Dec 23, 2015
The corresopnding C file was removed in rust-lang#30175 and looks like I forgot to remove
the header as well.
@dhuseby
Copy link

dhuseby commented Dec 30, 2015

When #30643 lands, i'll take a close look at the BSD builds and fix anything up that is needed.

@larsbergstrom
Copy link
Contributor

FYI, this did end up breaking Android. It broke rust-errno (https://github.com/lfairy/rust-errno), and getting a fix landed for that is currently blocking Servo's rustup.

Maybe we should just backport the symbol removed here (__errno_location) into Servo until we can reach @lfairy?

@alexcrichton
Copy link
Member Author

@larsbergstrom you can probably fix it temporarily by adding this to any crate:

#[cfg(target_os = "android")]
#[no_mangle]
pub unsafe extern fn __errno_location() -> *mut i32 {
    extern { fn __errno() -> *mut i32; }
    __errno()
}

@larsbergstrom
Copy link
Contributor

@alexcrichton Yeah, thanks! I was thinking of doing just that. Otherwise, we might have to fork errno, add a submodule for it, add a paths override to .cargo/config, etc. Ick!

@dhuseby
Copy link

dhuseby commented Jan 27, 2016

@alexcrichton @semarie when #31230 and rust-lang/libc#157 land, the BSD builds should be green.

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.

10 participants