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

Make `CStr` an extern type #64021

Open
wants to merge 9 commits into
base: master
from

Conversation

@oli-obk
Copy link
Contributor

commented Aug 30, 2019

This shrinks &CStr to one word size and is thus equivalent to *const c_char, making it possible to use &CStr types directly in FFI APIs instead of having to convert from and to at the FFI boundaries

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 30, 2019

r? @cramertj

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

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Aug 30, 2019

The job mingw-check of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-30T14:38:15.5776857Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-30T14:38:15.5959214Z ##[command]git config gc.auto 0
2019-08-30T14:38:16.2323660Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-30T14:38:16.2329074Z ##[command]git config --get-all http.proxy
2019-08-30T14:38:16.2334914Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64021/merge:refs/remotes/pull/64021/merge
---
2019-08-30T14:44:54.7112070Z    Compiling rand_pcg v0.1.1
2019-08-30T14:44:55.1228023Z    Compiling rand_chacha v0.1.0
2019-08-30T14:44:55.4896955Z    Compiling rand v0.6.1
2019-08-30T14:44:55.8863712Z    Compiling parking_lot_core v0.4.0
2019-08-30T14:44:57.5759660Z error[E0599]: no method named `to_bytes` found for type `&std::ffi::CStr` in the current scope
2019-08-30T14:44:57.5760102Z   --> /cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.99/src/ser/impls.rs:82:41
2019-08-30T14:44:57.5760379Z    |
2019-08-30T14:44:57.5760698Z 82 |         serializer.serialize_bytes(self.to_bytes())
2019-08-30T14:44:57.5764183Z 
2019-08-30T14:44:57.5809915Z error[E0599]: no method named `to_bytes` found for type `&std::ffi::CString` in the current scope
2019-08-30T14:44:57.5810314Z   --> /cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.99/src/ser/impls.rs:93:41
2019-08-30T14:44:57.5810582Z    |
2019-08-30T14:44:57.5810582Z    |
2019-08-30T14:44:57.5810884Z 93 |         serializer.serialize_bytes(self.to_bytes())
2019-08-30T14:44:57.5811263Z    |                                         ^^^^^^^^ help: there is a method with a similar name: `as_bytes`
2019-08-30T14:44:58.4457192Z     Checking num_cpus v1.8.0
2019-08-30T14:44:58.4857164Z error: aborting due to 2 previous errors
2019-08-30T14:44:58.4857552Z 
2019-08-30T14:44:58.4869762Z For more information about this error, try `rustc --explain E0599`.
---
2019-08-30T14:44:58.5626856Z == clock drift check ==
2019-08-30T14:44:58.5644547Z   local time: Fri Aug 30 14:44:58 UTC 2019
2019-08-30T14:44:59.3409654Z   network time: Fri, 30 Aug 2019 14:44:58 GMT
2019-08-30T14:44:59.3409757Z == end clock drift check ==
2019-08-30T14:45:01.6609049Z ##[error]Bash exited with code '1'.
2019-08-30T14:45:01.6640115Z ##[section]Starting: Checkout
2019-08-30T14:45:01.6641741Z ==============================================================================
2019-08-30T14:45:01.6641795Z Task         : Get sources
2019-08-30T14:45:01.6641857Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 @TimNN. (Feature Requests)

@Centril Centril added the I-nominated label Aug 30, 2019

@oli-obk oli-obk marked this pull request as ready for review Sep 2, 2019

@oli-obk oli-obk changed the title WIP: Make `CStr` an extern type Make `CStr` an extern type Sep 2, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 2, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-09-02T14:33:48.5481380Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-02T14:33:48.5702346Z ##[command]git config gc.auto 0
2019-09-02T14:33:48.5791646Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-02T14:33:48.5845263Z ##[command]git config --get-all http.proxy
2019-09-02T14:33:48.5977988Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64021/merge:refs/remotes/pull/64021/merge
---
2019-09-02T15:36:47.8374539Z .................................................................................................... 1500/8986
2019-09-02T15:36:53.5310326Z .................................................................................................... 1600/8986
2019-09-02T15:37:06.3599697Z .................................................i...............i.................................. 1700/8986
2019-09-02T15:37:14.7918512Z .................................................................................................... 1800/8986
2019-09-02T15:37:29.2609117Z ........................................iiiii....................................................... 1900/8986
2019-09-02T15:37:40.1956534Z .................................................................................................... 2100/8986
2019-09-02T15:37:42.8645477Z .................................................................................................... 2200/8986
2019-09-02T15:37:46.9950979Z .................................................................................................... 2300/8986
2019-09-02T15:37:54.6791965Z .................................................................................................... 2400/8986
---
2019-09-02T15:40:56.2463984Z ...........................i...............i........................................................ 4700/8986
2019-09-02T15:41:08.3028634Z .................................................................................................... 4800/8986
2019-09-02T15:41:14.5224154Z .................................................................................................... 4900/8986
2019-09-02T15:41:25.5847165Z .................................................................................................... 5000/8986
2019-09-02T15:41:31.4415051Z ........ii.ii....................................................................................... 5100/8986
2019-09-02T15:41:44.6212454Z .................................................................................................... 5300/8986
2019-09-02T15:41:53.1070655Z .......................................................................i............................ 5400/8986
2019-09-02T15:42:00.4088850Z .................................................................................................... 5500/8986
2019-09-02T15:42:07.3832407Z .................................................................................................... 5600/8986
2019-09-02T15:42:07.3832407Z .................................................................................................... 5600/8986
2019-09-02T15:42:18.0208847Z .................................................................ii...i..ii............i............ 5700/8986
2019-09-02T15:42:44.0220434Z .................................................................................................... 5900/8986
2019-09-02T15:42:53.8803036Z .................................................................................................... 6000/8986
2019-09-02T15:42:53.8803036Z .................................................................................................... 6000/8986
2019-09-02T15:43:02.2125431Z ...................................................................i..ii............................ 6100/8986
2019-09-02T15:43:31.8950413Z .................................................................................................... 6300/8986
2019-09-02T15:43:33.9925028Z ......................i............................................................................. 6400/8986
2019-09-02T15:43:36.1622451Z ..............................................................................................i..... 6500/8986
2019-09-02T15:43:38.8466739Z .................................................................................................... 6600/8986
---
2019-09-02T15:48:29.5181084Z  finished in 19.761
2019-09-02T15:48:29.5376650Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-02T15:48:29.7398416Z 
2019-09-02T15:48:29.7398836Z running 149 tests
2019-09-02T15:48:33.1948110Z i....iii......iii..iiii....i.............................i..i..................i....i.........ii.i.i 100/149
2019-09-02T15:48:35.2792237Z ..iiii..............i.........iii.i......ii......
2019-09-02T15:48:35.2793166Z 
2019-09-02T15:48:35.2796335Z  finished in 5.741
2019-09-02T15:48:35.2985967Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-02T15:48:35.4660974Z 
---
2019-09-02T15:48:37.6442799Z  finished in 2.345
2019-09-02T15:48:37.6655133Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-02T15:48:37.8342515Z 
2019-09-02T15:48:37.8342761Z running 9 tests
2019-09-02T15:48:37.8343533Z iiiiiiiii
2019-09-02T15:48:37.8343959Z 
2019-09-02T15:48:37.8344005Z  finished in 0.168
2019-09-02T15:48:37.8541903Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-02T15:48:38.0543583Z 
---
2019-09-02T15:48:56.7082473Z  finished in 18.854
2019-09-02T15:48:56.7329326Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-02T15:48:56.9485242Z 
2019-09-02T15:48:56.9486532Z running 123 tests
2019-09-02T15:49:21.4681778Z .iiiii...i.....i..i...i..i.i.i..i.ii..i.i.....i..i....ii..........iiii..........i...ii...i.......ii. 100/123
2019-09-02T15:49:26.2467377Z i.i.i......iii.i.....ii
2019-09-02T15:49:26.2470976Z 
2019-09-02T15:49:26.2471039Z  finished in 29.513
2019-09-02T15:49:26.2478450Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-02T15:49:26.2479459Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2019-09-02T16:03:51.1486619Z 
2019-09-02T16:03:51.1487640Z    Doc-tests core
2019-09-02T16:03:56.4857058Z 
2019-09-02T16:03:56.4857955Z running 2379 tests
2019-09-02T16:04:08.0583457Z ......iiiii......................................................................................... 100/2379
2019-09-02T16:04:19.5572779Z .........................................................................ii......................... 200/2379
2019-09-02T16:04:45.9643485Z .................................................................................................... 400/2379
2019-09-02T16:04:45.9643485Z .................................................................................................... 400/2379
2019-09-02T16:04:56.2496392Z ..............................i..i.................iiii............................................. 500/2379
2019-09-02T16:05:17.4452390Z .................................................................................................... 700/2379
2019-09-02T16:05:28.0354568Z .................................................................................................... 800/2379
2019-09-02T16:05:38.7793344Z .................................................................................................... 900/2379
2019-09-02T16:05:49.3972879Z .................................................................................................... 1000/2379
---
2019-09-02T16:11:00.7107674Z     ffi::c_str::tests::equal_hash
2019-09-02T16:11:00.7107705Z 
2019-09-02T16:11:00.7107754Z test result: FAILED. 762 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
2019-09-02T16:11:00.7107788Z 
2019-09-02T16:11:00.7112894Z error: test failed, to rerun pass '-p std --lib'
2019-09-02T16:11:00.7115863Z 
2019-09-02T16:11:00.7115863Z 
2019-09-02T16:11:00.7116809Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/src/libtest/Cargo.toml" "-p" "std" "--" "--quiet"
2019-09-02T16:11:00.7117466Z 
2019-09-02T16:11:00.7117593Z 
2019-09-02T16:11:00.7124176Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-09-02T16:11:00.7124412Z Build completed unsuccessfully in 1:30:01
2019-09-02T16:11:00.7124412Z Build completed unsuccessfully in 1:30:01
2019-09-02T16:11:00.7182102Z == clock drift check ==
2019-09-02T16:11:00.7198825Z   local time: Mon Sep  2 16:11:00 UTC 2019
2019-09-02T16:11:00.8686164Z   network time: Mon, 02 Sep 2019 16:11:00 GMT
2019-09-02T16:11:00.8690389Z == end clock drift check ==
2019-09-02T16:11:01.5171703Z ##[error]Bash exited with code '1'.
2019-09-02T16:11:01.5212612Z ##[section]Starting: Checkout
2019-09-02T16:11:01.5214487Z ==============================================================================
2019-09-02T16:11:01.5214554Z Task         : Get sources
2019-09-02T16:11:01.5214593Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 @TimNN. (Feature Requests)

@@ -1143,7 +1140,13 @@ impl CStr {
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn to_bytes_with_nul(&self) -> &[u8] {
unsafe { &*(&self.inner as *const [c_char] as *const [u8]) }
// safety: safe references to `CStr` can only exist if they point to a memory location
// that is terminated by a `\0`.

This comment has been minimized.

Copy link
@abonander

abonander Sep 4, 2019

Contributor

Is there a reason the original code in from_ptr didn't assert that the pointer was not null? I'm guessing it either caused miscompilation or pessmizations.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Sep 4, 2019

Author Contributor

I don't know. I'm guessing an oversight?

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Seems extern type is not used anywhere in the standard library. We'll need to consider whether we are OK with relying on this unstable language feature in the standard library.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@Centril We should also add a clippy lint that notices if you use a pointer to c_char in FFI and immediately convert between that and CStr.

@Centril Centril added this to the 1.39 milestone Sep 5, 2019

@cramertj

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

We discussed this in the lang team meeting today and we were all roughly onboard, with idea that the only additional thing we were promising was that &CStr was FFI-safe and compatible with *const [c_char]. If there are additional APIs or guarantees being promised here, we should review them separately.

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

No, there are no other changes (except the implicit one being that &CStr is now a narrow pointer). &CStr and *const char are now transmutable between each other (assuming the validity constraints on *const char that the from_ptr method always had)

@oli-obk oli-obk force-pushed the oli-obk:unsized_cstr branch from fe24d5d to 6340dfb Sep 6, 2019

@oli-obk oli-obk force-pushed the oli-obk:unsized_cstr branch from 6340dfb to 2a7aa53 Sep 6, 2019

src/libstd/ffi/c_str.rs Outdated Show resolved Hide resolved
src/libstd/ffi/c_str.rs Outdated Show resolved Hide resolved
src/libstd/ffi/c_str.rs Outdated Show resolved Hide resolved
src/libstd/ffi/c_str.rs Outdated Show resolved Hide resolved
src/libstd/ffi/c_str.rs Show resolved Hide resolved

@cramertj cramertj removed the I-nominated label Sep 7, 2019

Apply suggestions from code review
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

Rebased and addressed all review comments

src/libstd/ffi/c_str.rs Outdated Show resolved Hide resolved
@joshtriplett

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

I made one comment about adding a comment for Option<&CStr>; otherwise, this looks good to me.

src/libstd/ffi/c_str.rs Outdated Show resolved Hide resolved

@oli-obk oli-obk force-pushed the oli-obk:unsized_cstr branch from 4c4a7b4 to 719c734 Sep 11, 2019

/// placed in the signatures of FFI functions. Instead, safe wrappers of FFI
/// functions may leverage the unsafe [`from_ptr`] constructor to provide a safe
/// interface to other consumers.
/// Note that this structure is `repr(C)` and is can be

This comment has been minimized.

Copy link
@Phrohdoh

Phrohdoh Sep 12, 2019

Drop the "is" before "can" here?

@kennytm

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Is the question of size_of_val(cstr) (https://internals.rust-lang.org/t/pre-rfc-make-cstr-a-thin-pointer/6258/18) settled?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.