-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Add validation to const fn CStr creation #99977
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
Conversation
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
This comment has been minimized.
This comment has been minimized.
Taking this review by request from @BlackHoleFox (since kennytm's backlog is big and seems to be busy). r? @thomcc |
Going to do a perf run since this changes O(1) to O(n) in consteval. I suspect const CStrs are overwhelmingly short, but lets just double-check. @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 0e54d71 with merge 421f956964ea447066077b77384c2a256f300834... |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
☀️ Try build successful - checks-actions |
Queued 421f956964ea447066077b77384c2a256f300834 with parent 34805f3, future comparison URL. |
Finished benchmarking commit (421f956964ea447066077b77384c2a256f300834): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesThis benchmark run did not return any relevant results for this metric. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Footnotes |
This has been a safety invariant since the function has been introduced, and only checking in const seems unlikely to hit surprise ecosystem breakage (it seems like you'd notice if you were calling this on a bytestring literal with extra embedded nulls, and that code that does it is broken at runtime in other ways regardless). The main concern I'd have is if this causes perf issues if someone uses it on a huge string, but we've done due diligence here and it's a nice UB check (especially if it catches a missing nul-terminator!). Re: your question about a UI test, I don't think this needs one -- the fact that it uses panic (and not intrinsics::abort) is already nicer than most of our other const_eval_select-based UB detection code 😅. @bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (792bc5a): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
Improves upon the existing validation that only worked when building the stdlib from source.
CStr::from_bytes_with_nul_unchecked
now utilizesconst_eval_select
to validate the safety requirements of the function when used asconst FOO: &CStr = CStr::from_bytes_with_nul_unchecked(b"Foobar\0");
.This can help catch erroneous code written by accident and, assuming a new enough
rustc
in use, remove the need for boilerplate safety comments for this function inconst
contexts.I think this might need a UI test, but I don't know where to put it. If this is a worth change, a perf run would be nice to make sure theO(n)
validation isn't too bad. I didn't notice a difference building the stdlib tests locally.