Make `CStr` an extern type #64021

oli-obk · 2019-08-30T14:37:39Z

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 · 2019-08-30T14:37:42Z

r? @cramertj

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

rust-highfive · 2019-09-02T16:11:21Z

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)

abonander · 2019-09-04T03:53:43Z

src/libstd/ffi/c_str.rs

@@ -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`.


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.

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

src/libstd/ffi/c_str.rs

Centril · 2019-09-05T03:37:52Z

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 · 2019-09-05T19:34:04Z

@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.

cramertj · 2019-09-05T21:47:57Z

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 · 2019-09-06T06:33:48Z

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)

…ect usage

src/libstd/ffi/c_str.rs

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

oli-obk · 2019-09-08T16:40:24Z

Rebased and addressed all review comments

src/libstd/ffi/c_str.rs

joshtriplett · 2019-09-09T19:35:21Z

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

src/libstd/ffi/c_str.rs

kennytm · 2019-09-12T11:53:13Z

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

oli-obk · 2019-09-20T15:35:52Z

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

Oh... I did not think about this.

Some data:

Currently returns the number of bytes including the trailing null byte
This can only be re-achieved by counting the number of bytes
The ?DynSized PR has been postponed without a clear way forward
extern { type Foo; } is unstable

Proposed solution:

extern {
    type Foo {
        fn size_of_val(&self) -> usize {
            unsafe {
				let this = self as *const Foo as *const CStr;
				this.len()
			};
        }
        fn align_of_val(&self) -> usize {
            1
        }
    }
}

We can require every extern type to supply these functions. It's all unstable without a clear way forward anyway. There's no need to see this as us ever intending to stabilize this API, but it would allow the standard library to use it.

Since for some types this isn't possible, these types can explicity opt to panic inside the size_of_val and align_of_val methods as specified by the RFC.

JohnCSimon · 2019-10-05T19:42:33Z

Ping from triage
@oli-obk @cramertj This has sat idle for the last 15 days, what else needs to happen for this PR?
Thanks.

oli-obk · 2019-10-17T14:15:39Z

I don't think this can happen without further language level work

joshtriplett · 2019-10-19T08:59:41Z

@oli-obk What language-level work would be required to make this work?

It sounded like you saw a path forward, given that the standard library can use unstable features.

oli-obk · 2019-10-19T10:42:11Z

Well... I'd still need to implement that, and I'm currently swamped in reviews and my own PRs that I need to rebase and a bunch of issues that should really be resolved. Making CStr an extern type is just a nice to have, so not very high on my priority list.

rust-highfive assigned cramertj Aug 30, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2019

This comment has been minimized.

Sign in to view

cramertj added A-lang-item Area: Language items T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed A-lang-item Area: Language items labels Aug 30, 2019

Centril added the I-nominated label Aug 30, 2019

oli-obk marked this pull request as ready for review September 2, 2019 15:26

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

abonander reviewed Sep 4, 2019

View reviewed changes

bluss reviewed Sep 4, 2019

View reviewed changes

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

bluss reviewed Sep 4, 2019

View reviewed changes

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

joshtriplett added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 5, 2019

Centril added this to the 1.39 milestone Sep 5, 2019

oli-obk force-pushed the unsized_cstr branch from fe24d5d to 6340dfb Compare September 6, 2019 11:52

oli-obk added 5 commits September 6, 2019 13:52

Make CStr an extern type

2d6c6b7

Work around extern type assoc function bug

b039dc9

Update docs to reflect the performance changes of certain methods

6d3ee10

Rewrite the unsafe rules in a manner that guides the user to the corr…

7eabd86

…ect usage

Unify hashing between CString and CStr

2a7aa53

oli-obk force-pushed the unsized_cstr branch from 6340dfb to 2a7aa53 Compare September 6, 2019 11:53

Centril reviewed Sep 6, 2019

View reviewed changes

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 removed the I-nominated label Sep 7, 2019

Apply suggestions from code review

76d733e

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

Explain one more unsafe

298f8ee

joshtriplett reviewed Sep 9, 2019

View reviewed changes

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

joshtriplett reviewed Sep 11, 2019

View reviewed changes

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

Null pointers are optional

719c734

oli-obk force-pushed the unsized_cstr branch from 4c4a7b4 to 719c734 Compare September 11, 2019 10:45

phrohdoh reviewed Sep 12, 2019

View reviewed changes

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

Typo

e3d928c

Centril modified the milestones: 1.39, 1.40 Sep 26, 2019

oli-obk closed this Oct 17, 2019

Centril removed this from the 1.40 milestone Nov 7, 2019

oli-obk deleted the unsized_cstr branch March 16, 2021 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `CStr` an extern type #64021

Make `CStr` an extern type #64021

oli-obk commented Aug 30, 2019 •

edited

Loading

rust-highfive commented Aug 30, 2019

This comment has been minimized.

rust-highfive commented Sep 2, 2019

abonander Sep 4, 2019 •

edited

Loading

oli-obk Sep 4, 2019

Centril commented Sep 5, 2019

joshtriplett commented Sep 5, 2019

cramertj commented Sep 5, 2019

oli-obk commented Sep 6, 2019

oli-obk commented Sep 8, 2019

joshtriplett commented Sep 9, 2019

kennytm commented Sep 12, 2019

oli-obk commented Sep 20, 2019

JohnCSimon commented Oct 5, 2019

oli-obk commented Oct 17, 2019

joshtriplett commented Oct 19, 2019

oli-obk commented Oct 19, 2019

Make CStr an extern type #64021

Make CStr an extern type #64021

Conversation

oli-obk commented Aug 30, 2019 • edited Loading

rust-highfive commented Aug 30, 2019

This comment has been minimized.

rust-highfive commented Sep 2, 2019

abonander Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

oli-obk Sep 4, 2019

Choose a reason for hiding this comment

Centril commented Sep 5, 2019

joshtriplett commented Sep 5, 2019

cramertj commented Sep 5, 2019

oli-obk commented Sep 6, 2019

oli-obk commented Sep 8, 2019

joshtriplett commented Sep 9, 2019

kennytm commented Sep 12, 2019

oli-obk commented Sep 20, 2019

JohnCSimon commented Oct 5, 2019

oli-obk commented Oct 17, 2019

joshtriplett commented Oct 19, 2019

oli-obk commented Oct 19, 2019

Make `CStr` an extern type #64021

Make `CStr` an extern type #64021

oli-obk commented Aug 30, 2019 •

edited

Loading

abonander Sep 4, 2019 •

edited

Loading