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

Tracking Issue for const_cstr_from_ptr #113219

Closed
4 tasks done
tgross35 opened this issue Jul 1, 2023 · 18 comments
Closed
4 tasks done

Tracking Issue for const_cstr_from_ptr #113219

tgross35 opened this issue Jul 1, 2023 · 18 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tgross35
Copy link
Contributor

tgross35 commented Jul 1, 2023

Feature gate: #![feature(const_cstr_from_ptr)]

This is a tracking issue for using CStr::from_ptr() in a const context.

This method was previously gated under const_cstr_methods, but was split off after discussion in #107624 (review).

Public API

// core/ffi/c_str.rs

impl CStr {
    pub const unsafe fn from_ptr<'a>(ptr: *const c_char) -> &'a CStr;
    pub const fn count_bytes(&self) -> usize;
}

Steps / History

Unresolved Questions

Making this const currently depends on const_eval_select, which unstable. If const_eval_select becomes internally usable, we will be able to stabilize this.

The other currently available option is to always use a const strlen, but this would come with a performance hit.

Alternatively, whenever CStr becomes a thin pointer this can be const stabilized since it will have no internal operations.

@tgross35 tgross35 added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 1, 2023
tgross35 added a commit to tgross35/rust that referenced this issue Jul 1, 2023
Tracking issue rust-lang#101719 was for `const_cstr_methods`, rust-lang#113219 is a new
issue specific for `const_cstr_from_ptr`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 22, 2023
…ing-issue, r=ChrisDenton

Update the tracking issue for `const_cstr_from_ptr`

Tracking issue rust-lang#101719 was for `const_cstr_methods`, rust-lang#113219 is a new issue specific for `const_cstr_from_ptr`.

(I believe rust-lang#101719 could also be closed)

`@rustbot` label +T-libs-api +A-docs
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 22, 2023
…ing-issue, r=ChrisDenton

Update the tracking issue for `const_cstr_from_ptr`

Tracking issue rust-lang#101719 was for `const_cstr_methods`, rust-lang#113219 is a new issue specific for `const_cstr_from_ptr`.

(I believe rust-lang#101719 could also be closed)

``@rustbot`` label +T-libs-api +A-docs
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 22, 2023
…ing-issue, r=ChrisDenton

Update the tracking issue for `const_cstr_from_ptr`

Tracking issue rust-lang#101719 was for `const_cstr_methods`, rust-lang#113219 is a new issue specific for `const_cstr_from_ptr`.

(I believe rust-lang#101719 could also be closed)

```@rustbot``` label +T-libs-api +A-docs
@tgross35
Copy link
Contributor Author

tgross35 commented Aug 3, 2023

I gated CStr::count_bytes()'s constness under this same flag since it depends on the same decision. See #114443

tgross35 added a commit to tgross35/rust that referenced this issue Aug 29, 2023
This is feature gated under `cstr_count_bytes` and provides a more
straightforward way to access the length of a `CStr`

Link: rust-lang#113219
@dtolnay

This comment was marked as resolved.

@dtolnay
Copy link
Member

dtolnay commented Sep 19, 2023

There's an ongoing RFC for stabilizing that intrinsic (without exposing it as public API), so let's just let that RFC handle it

I infer that this is rust-lang/rfcs#3352. According to #107624 (comment), T-libs-api should not look into stabilizing the APIs tracked by this tracking issue until some clarity has been reached by T-lang on that RFC.

I have added a link to that RFC to the checklist in this issue.

@tgross35
Copy link
Contributor Author

That all sounds correct, thanks for the updates

@RalfJung
Copy link
Member

Why is rust-lang/rfcs#3352 listed as a blocker here? Using const_eval_select in stable const fn is fine, and in fact we already do that, e.g. from_bytes_with_nul_unchecked. It's only a problem if the two code paths do not behave the same way.

@RalfJung
Copy link
Member

Oh I see, this probably goes back to this discussion. So far we're only using const_eval_select to control assertions, this needs it to provide a const-time implementation for strlen.

But still, I don't think that requires the RFC -- as Oli said, all that requires is to commit to is that we'll have some way of calling strlen in the runtime path of a const fn. There's a lot of ways to do that.

@dtolnay dtolnay added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Mar 24, 2024
@dtolnay
Copy link
Member

dtolnay commented Mar 24, 2024

@rust-lang/libs-api, @rust-lang/lang:
@rfcbot fcp merge

I propose stabilizing const on this already existing stable function. All that requires us to commit to is that we'll have some way of calling strlen in the runtime path of a const fn. There's a lot of ways to do that.

impl CStr {
    pub const unsafe fn from_ptr<'a>(ptr: *const c_char) -> &'a CStr;
}

Relatedly, I propose moving the const stability of the currently unstable count_bytes method over to #114441 as a libs-api-only issue — we'll stabilize count_bytes separately without taking another T-lang approval to make it const.

impl CStr {
    #[unstable]
    pub const fn count_bytes(&self) -> usize;
}

The const for this pair of functions has been tracked here together because one of them will be trivial and one of them will be interesting under both the current and planned implementation of CStr. If &CStr is a fat pointer then from_ptr is interesting and count_bytes is trivial. If &CStr is a thin pointer then from_ptr is trivial and count_bytes is interesting. The part that is interesting basically looks the same either way, and that is:

const unsafe fn const_strlen(ptr: *const c_char) -> usize {
const fn strlen_ct(s: *const c_char) -> usize {
let mut len = 0;
// SAFETY: Outer caller has provided a pointer to a valid C string.
while unsafe { *s.add(len) } != 0 {
len += 1;
}
len
}
#[inline]
fn strlen_rt(s: *const c_char) -> usize {
extern "C" {
/// Provided by libc or compiler_builtins.
fn strlen(s: *const c_char) -> usize;
}
// SAFETY: Outer caller has provided a pointer to a valid C string.
unsafe { strlen(s) }
}
intrinsics::const_eval_select((ptr,), strlen_ct, strlen_rt)
}

For previous discussion of whether this is okay to stabilize as const, please see #107624 (comment) and #107624 (comment). I am including T-lang in this FCP to vet this as well. Ralf's comments in this issue suggest we should not have to wait for progress on rust-lang/rfcs#3352 as a prerequisite for stabilizing const fn from_ptr because regardless of what happens with that RFC, we will always be able to come up with some way for const_strlen to delegate to an optimized strlen at runtime and a straightforward const strlen in const, having identical observable behavior other than performance.

Note that this feature would be the first time we stabilize a usage of const_eval_select that is not just changing assertions. The previous use of const_eval_select in a stable const fn is from_bytes_with_nul_unchecked as Ralf pointed out.

#[stable(feature = "cstr_from_bytes", since = "1.10.0")]
#[rustc_const_stable(feature = "const_cstr_unchecked", since = "1.59.0")]
#[rustc_allow_const_fn_unstable(const_eval_select)]
pub const unsafe fn from_bytes_with_nul_unchecked(bytes: &[u8]) -> &CStr {
#[inline]
fn rt_impl(bytes: &[u8]) -> &CStr {
// Chance at catching some UB at runtime with debug builds.
debug_assert!(!bytes.is_empty() && bytes[bytes.len() - 1] == 0);
// SAFETY: Casting to CStr is safe because its internal representation
// is a [u8] too (safe only inside std).
// Dereferencing the obtained pointer is safe because it comes from a
// reference. Making a reference is then safe because its lifetime
// is bound by the lifetime of the given `bytes`.
unsafe { &*(bytes as *const [u8] as *const CStr) }
}
const fn const_impl(bytes: &[u8]) -> &CStr {
// Saturating so that an empty slice panics in the assert with a good
// message, not here due to underflow.
let mut i = bytes.len().saturating_sub(1);
assert!(!bytes.is_empty() && bytes[i] == 0, "input was not nul-terminated");
// Ending nul byte exists, skip to the rest.
while i != 0 {
i -= 1;
let byte = bytes[i];
assert!(byte != 0, "input contained interior nul");
}
// SAFETY: See `rt_impl` cast.
unsafe { &*(bytes as *const [u8] as *const CStr) }
}
intrinsics::const_eval_select((bytes,), const_impl, rt_impl)
}

@rfcbot
Copy link

rfcbot commented Mar 24, 2024

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 24, 2024
@scottmcm
Copy link
Member

Given that

const unsafe fn count_bytes(p: *const u8) -> usize {
    let mut i = 0;
    loop {
        if *p.add(i) == 0 { return i }
        i += 1;
    }
}

compiles on stable, I don't see any lang concerns here.

Totally happy leaving this up to libs-api.

@rfcbot reviewed

fmease added a commit to fmease/rust that referenced this issue Apr 10, 2024
…s, r=dtolnay

Stabilize `cstr_count_bytes`

Newly stable API:

```rust
impl CStr {
    pub fn count_bytes(&self) -> usize;
}
```

Const stabilization has not yet been decided, so that will continue to be gated under <rust-lang#113219>.

FCP finished at rust-lang#114441 (comment).

Fixes: <rust-lang#114441>
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 11, 2024
Rollup merge of rust-lang#123661 - tgross35:stabilize-cstr_count_bytes, r=dtolnay

Stabilize `cstr_count_bytes`

Newly stable API:

```rust
impl CStr {
    pub fn count_bytes(&self) -> usize;
}
```

Const stabilization has not yet been decided, so that will continue to be gated under <rust-lang#113219>.

FCP finished at rust-lang#114441 (comment).

Fixes: <rust-lang#114441>
@RalfJung RalfJung added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 20, 2024
@RalfJung
Copy link
Member

@joshtriplett @m-ou-se @nikomatsakis @pnkfelix @tmandry reminder that there's a checkbox here waiting for you. :)

@tmandry
Copy link
Member

tmandry commented Jun 20, 2024

Agreed with @scottmcm, and I'm personally comfortable with saying we'll always have a way to make this performant.

@rfcbot reviewed

@scottmcm
Copy link
Member

(Agreed, @tmandry, I'm also personally comfortable with it -- we could also always make it an intrinsic if needed, like memcmp #114382, if for some reason it needed to stop using C_E_S.)

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jun 26, 2024
@rfcbot
Copy link

rfcbot commented Jun 26, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this in the lang call today. Everyone felt comfortable with it, and it's now in FCP.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 26, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 6, 2024
@rfcbot
Copy link

rfcbot commented Jul 6, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jul 6, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 11, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 12, 2024
Stabilize const_cstr_from_ptr (CStr::from_ptr, CStr::count_bytes)

Completed the pair of FCPs rust-lang#113219 (comment) + rust-lang#114441 (comment).

`CStr::from_ptr` is covered by just the first FCP on its own. `CStr::count_bytes` requires the approval of both FCPs. The second paragraph of the first link and the last paragraph of the second link explain the relationship between the two FCPs. As both have been approved, we can proceed with stabilizing `const` on both of these already-stable functions.
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Jul 12, 2024
Stabilize const_cstr_from_ptr (CStr::from_ptr, CStr::count_bytes)

Completed the pair of FCPs rust-lang#113219 (comment) + rust-lang#114441 (comment).

`CStr::from_ptr` is covered by just the first FCP on its own. `CStr::count_bytes` requires the approval of both FCPs. The second paragraph of the first link and the last paragraph of the second link explain the relationship between the two FCPs. As both have been approved, we can proceed with stabilizing `const` on both of these already-stable functions.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 12, 2024
Stabilize const_cstr_from_ptr (CStr::from_ptr, CStr::count_bytes)

Completed the pair of FCPs rust-lang#113219 (comment) + rust-lang#114441 (comment).

`CStr::from_ptr` is covered by just the first FCP on its own. `CStr::count_bytes` requires the approval of both FCPs. The second paragraph of the first link and the last paragraph of the second link explain the relationship between the two FCPs. As both have been approved, we can proceed with stabilizing `const` on both of these already-stable functions.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 12, 2024
Stabilize const_cstr_from_ptr (CStr::from_ptr, CStr::count_bytes)

Completed the pair of FCPs rust-lang#113219 (comment) + rust-lang#114441 (comment).

`CStr::from_ptr` is covered by just the first FCP on its own. `CStr::count_bytes` requires the approval of both FCPs. The second paragraph of the first link and the last paragraph of the second link explain the relationship between the two FCPs. As both have been approved, we can proceed with stabilizing `const` on both of these already-stable functions.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 12, 2024
Stabilize const_cstr_from_ptr (CStr::from_ptr, CStr::count_bytes)

Completed the pair of FCPs rust-lang#113219 (comment) + rust-lang#114441 (comment).

`CStr::from_ptr` is covered by just the first FCP on its own. `CStr::count_bytes` requires the approval of both FCPs. The second paragraph of the first link and the last paragraph of the second link explain the relationship between the two FCPs. As both have been approved, we can proceed with stabilizing `const` on both of these already-stable functions.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 12, 2024
Rollup merge of rust-lang#127433 - dtolnay:conststrlen, r=workingjubilee

Stabilize const_cstr_from_ptr (CStr::from_ptr, CStr::count_bytes)

Completed the pair of FCPs rust-lang#113219 (comment) + rust-lang#114441 (comment).

`CStr::from_ptr` is covered by just the first FCP on its own. `CStr::count_bytes` requires the approval of both FCPs. The second paragraph of the first link and the last paragraph of the second link explain the relationship between the two FCPs. As both have been approved, we can proceed with stabilizing `const` on both of these already-stable functions.
@dtolnay dtolnay closed this as completed Jul 12, 2024
tgross35 added a commit to tgross35/rust that referenced this issue Jul 12, 2024
Since the libs and lang teams completed an FCP to allow for const
`strlen` ([1]), currently implemented with `const_eval_select`, there is
no longer any reason to avoid this specific function or use it only in
const.

Rename it to reflect this status change.

[1]: rust-lang#113219 (comment)
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Jul 12, 2024
…olnay

Rename the internal `const_strlen` to just `strlen`

Since the libs and lang teams completed an FCP to allow for const `strlen` ([1]), currently implemented with `const_eval_select`, there is no longer any reason to avoid this specific function or use it only in const.

Rename it to reflect this status change.

[1]: rust-lang#113219 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 13, 2024
Rollup merge of rust-lang#127660 - tgross35:const_strlen-rename, r=dtolnay

Rename the internal `const_strlen` to just `strlen`

Since the libs and lang teams completed an FCP to allow for const `strlen` ([1]), currently implemented with `const_eval_select`, there is no longer any reason to avoid this specific function or use it only in const.

Rename it to reflect this status change.

[1]: rust-lang#113219 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants