Add debug assertions to validate NUL terminator in c strings #93979
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
SUPERCILEX
changed the title
library/std/src/ffi/c_str.rs
Outdated
| @@ -449,12 +449,18 @@ impl CString { | |||
| /// let c_string = CString::from_vec_unchecked(raw); | |||
| /// } | |||
| /// ``` | |||
| #[inline(always)] | |||
There was a problem hiding this comment.
What's the justification for this attribute? inline(always) is very rarely appropriate in libstd.
There was a problem hiding this comment.
My thinking was that the public method now simply forwards to the internal method, so it should be removed to match the performance of not having that extra layer of indirection.
There was a problem hiding this comment.
the public method now simply forwards to the internal method
This is not accurate if _from_vec_unchecked already got inlined into from_vec_unchecked. In that situation inline(always) would be forcing a potentially much larger chunk of code than intended to be duplicated into every call site.
We tend to be skeptical of inline(always) without compelling benchmarks that demonstrate it's the only way to achieve certain commonly-necessary optimizations downstream.
There was a problem hiding this comment.
So inlining is bottom up rather than top down? I was thinking that from_vec_unchecked would get inlined first (so the call site now uses _from_vec_unchecked) and only then would the inliner consider _from_vec_unchecked. You're making it sound like _from_vec_unchecked could be inlined into from_vec_unchecked and then all of that gets inlined into the call site.
There was a problem hiding this comment.
Yeah it's bottom up: https://llvm.org/docs/Passes.html#inline-function-integration-inlining
Bottom-up inlining of functions into callees.
There was a problem hiding this comment.
Dang, bummer. Fixed, except for _from_bytes_with_nul_unchecked which I changed to always since it's just a cast.
SUPERCILEX
force-pushed
the
debug_check
branch
2 times, most recently
from
f243b98 to
3dbb679
Compare
There was a problem hiding this comment.
Could you please remove those inlines too, unless you have some evidence that they aren't already being inlined automatically and that inlining there is necessary for achieving good enough performance? Inlining more than necessary just slows down compile time and adds maintenance burden and noise to the standard library implementation. In libstd inline(always) is very rarely appropriate.
dtolnay
added
S-waiting-on-author
Signed-off-by: Alex Saveau <[email protected]>
SUPERCILEX
force-pushed
the
debug_check
branch
from
3dbb679 to
897c8d0
Compare
|
@dtolnay remove them completely? I just downgraded the always to a normal inline so it matches what was already there, but I can remove it completely if that's preferred. |
|
@bors r+ |
|
📌 Commit 897c8d0 has been approved by |
bors
added
S-waiting-on-bors
…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#92902 (Improve the documentation of drain members) - rust-lang#93658 (Stabilize `#[cfg(panic = "...")]`) - rust-lang#93954 (rustdoc-json: buffer output) - rust-lang#93979 (Add debug assertions to validate NUL terminator in c strings) - rust-lang#93990 (pre rust-lang#89862 cleanup) - rust-lang#94006 (Use a `Field` in `ConstraintCategory::ClosureUpvar`) - rust-lang#94086 (Fix ScalarInt to char conversion) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The
uncheckedvariants from the stdlib usually perform the check anyway if debug assertions are on (for example,unwrap_unchecked). This PR does the same thing forCStrandCString, validating the correctness for the NUL byte in debug mode.