-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
core: use the platform's memchr
#159090
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
base: main
Are you sure you want to change the base?
core: use the platform's memchr
#159090
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| // Some parts taken from rust-memchr. | ||
| // Copyright 2015 Andrew Gallant, bluss and Nicolas Koch | ||
|
|
||
| #![forbid(unsafe_op_in_unsafe_fn)] | ||
|
|
||
| use core::ffi::{c_int, c_void}; | ||
| use core::ptr; | ||
|
|
||
| unsafe fn memchr_naive( | ||
| mut current: *const u8, | ||
| needle: u8, | ||
| mut n: usize, | ||
| ) -> *mut c_void { | ||
| while n > 0 { | ||
| let byte = unsafe { current.read() }; | ||
| if byte == needle { | ||
| return current as *mut c_void; | ||
| } | ||
|
|
||
| current = unsafe { current.add(1) }; | ||
| n -= 1; | ||
| } | ||
|
|
||
| ptr::null_mut() | ||
| } | ||
|
|
||
| type Chunk = cfg_select! { | ||
| any( | ||
| all(target_arch = "x86_64", target_feature = "sse2"), | ||
| all(target_arch = "aarch64", target_feature = "neon"), | ||
| ) => core::simd::u8x16, | ||
| _ => [usize; 2], | ||
| }; | ||
|
|
||
| pub unsafe fn memchr( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation does not implement the contract specified for
This means we are not allowed to read any bytes beyond the first byte that is different, which means that the naive implementation is the only legal implementation (unless we use something like speculative loads which are still being worked on on the LLVM side: llvm/llvm-project#179642). We can of course have our own contract for our own function, but we should make sure this never gets used by anything that expects
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "as if" here is doing some heavy lifting. Otherwise glibc's implementation is also non-conformant according to your interpretation. (If I'm understanding what you're saying correctly.)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "as if" means "must not have more UB than". So yes I am saying that if the glibc implementation reads bytes beyond the first occurrence of the needle (and it doesn't use inline asm to do so) then it is non-conformant, at least if it ends up in the same translation unit as the C program using it and hence the compiler can see that more bytes than "until the first difference" are being accessed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm getting lost here. Can you rephrase what you're saying more concretely? What I'm hearing from you is that the only correct implementation of I think I'm not getting where this implementation specifically violates what the C standard is. I can't tell if your commentary is applying to all possible implementations of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe all such implementations in glibc are in fully separate
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But there is a difference between "reading bytes beyond where Like, is this implementation here committing UB somewhere? It might read past the first occurrence of the needle, but that doesn't seem like an issue to me. And that's where the "as if" does some heavier lifting I think. I would really like to see someone be concrete about why this implementation is problematic. :-) Mostly because if it is, then there is perhaps something problematic about the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am saying that this code is entirely well-defined: #include <string.h>
int main() {
char *haystack = "abcdef";
memchr(haystack, 'f', 25600); // note that the `n` parameter is way bigger than the buffer
return 0;
}And the same goes for the equivalent Rust code invoking Do we have consensus on those two claims? If yes, then let's talk about the consequences. OOB reads are usually UB. Whether this specific OOB read is UB depends on whether So this Rust-written implementation here is not a valid replacement for a libc's
I don't think there is a problem for the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Given that your crate makes no attempt to be identical to C's
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I get it now. I was not thinking about whacky values of Makes me wonder whether musl's implementation here is not correct? Since it's written in C and can read in word-sized chunks: https://github.com/kraj/musl/blob/a42e9dee266f398026a33d0793c66225c7997755/src/string/memchr.c#L15-L24
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't easily make sense of what that code does but based on your description -- yes that sounds like it'd be not correct, modulo the "separate translation unit" argument. |
||
| s: *const c_void, | ||
| c: c_int, | ||
| mut n: usize, | ||
| ) -> *mut c_void { | ||
| let mut current = s.cast::<u8>(); | ||
| let needle = c as u8; | ||
|
|
||
| if n < size_of::<Chunk>() { | ||
| return unsafe { memchr_naive(current, needle, n) }; | ||
| } | ||
|
|
||
| // Advance until the pointer is aligned to a chunk. | ||
| // Since the size of a chunk must be larger than its alignment and we | ||
| // only reach this point if `n` is larger or equal to the size of a chunk, | ||
| // this doesn't need bound checks. | ||
| while !current.cast::<Chunk>().is_aligned() { | ||
| let byte = unsafe { current.read() }; | ||
| if byte == needle { | ||
| return current as *mut c_void; | ||
| } | ||
|
|
||
| current = unsafe { current.add(1) }; | ||
| n -= 1; | ||
| } | ||
|
|
||
| cfg_select! { | ||
| // These targets have efficient SIMD acceleration. | ||
| any( | ||
| all(target_arch = "x86_64", target_feature = "sse2"), | ||
| all(target_arch = "aarch64", target_feature = "neon"), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you checked the codegen for neon here? It doesn't have
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the specific blocker to using the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We cannot use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. |
||
| ) => { | ||
| use core::simd::cmp::SimdPartialEq; | ||
|
|
||
| let mut current = current.cast::<Chunk>(); | ||
| let chunk_needle = Chunk::splat(needle); | ||
| while n >= size_of::<Chunk>() { | ||
| let chunk = unsafe { current.read() }; | ||
| if let Some(index) = chunk.simd_eq(chunk_needle).first_set() { | ||
| return unsafe { current.cast::<u8>().add(index) as *mut c_void } | ||
| } | ||
|
|
||
| current = unsafe { current.add(1) }; | ||
| n -= size_of::<Chunk>(); | ||
| } | ||
| } | ||
| // Unfortunately LLVM is not smart enough to use SWAR (SIMD-within-a-register) | ||
| // techniques if native SIMD is not available, so we need to do SWAR manually. | ||
| _ => { | ||
| const LOW_BITS: usize = usize::from_ne_bytes([0x01; _]); | ||
| const HIGH_BITS: usize = usize::from_ne_bytes([0x80; _]); | ||
|
|
||
| /// Returns `true` if `x` contains any zero byte. | ||
| /// | ||
| /// From *Matters Computational*, J. Arndt: | ||
| /// | ||
| /// "The idea is to subtract one from each of the bytes and then look for | ||
| /// bytes where the borrow propagated all the way to the most significant | ||
| /// bit." | ||
| #[inline] | ||
| const fn contains_zero_byte(x: usize) -> bool { | ||
| x.wrapping_sub(LOW_BITS) & !x & HIGH_BITS != 0 | ||
| } | ||
|
|
||
| let mut current = current.cast::<Chunk>(); | ||
| // Since `needle` is 8-bit wide, this multiplication will splat those | ||
| // 8 bits over all bytes of `usize`. | ||
| let chunk_needle = LOW_BITS * needle as usize; | ||
| while n >= size_of::<Chunk>() { | ||
| // Compare two words in one go. | ||
| let [lower, upper] = unsafe { current.read() }; | ||
| // If the byte matches, then the XOR will result in that byte | ||
| // being zero. | ||
| let a = contains_zero_byte(lower ^ chunk_needle); | ||
| let b = contains_zero_byte(upper ^ chunk_needle); | ||
| if a | b { | ||
| // Find the matching byte. The loop doesn't need to be bounded | ||
| // since we know there is a match. | ||
| let mut current = current.cast::<u8>(); | ||
| loop { | ||
| let byte = unsafe { current.read() }; | ||
| if byte == needle { | ||
| return current as *mut c_void; | ||
| } | ||
|
|
||
| current = unsafe { current.add(1) }; | ||
| } | ||
| } | ||
|
|
||
| current = unsafe { current.add(1) }; | ||
| n -= size_of::<Chunk>(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Search in the remaining bytes. | ||
| let current = current.cast::<u8>(); | ||
| unsafe { memchr_naive(current, needle, n) } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,11 @@ | ||
| // Original implementation taken from rust-memchr. | ||
| // Copyright 2015 Andrew Gallant, bluss and Nicolas Koch | ||
|
|
||
| use crate::ffi::{c_int, c_void}; | ||
| use crate::intrinsics::const_eval_select; | ||
|
|
||
| const LO_USIZE: usize = usize::repeat_u8(0x01); | ||
| const HI_USIZE: usize = usize::repeat_u8(0x80); | ||
| const USIZE_BYTES: usize = size_of::<usize>(); | ||
|
|
||
| /// Returns `true` if `x` contains any zero byte. | ||
| /// | ||
|
|
@@ -22,86 +22,47 @@ const fn contains_zero_byte(x: usize) -> bool { | |
| /// Returns the first index matching the byte `x` in `text`. | ||
| #[inline] | ||
| #[must_use] | ||
| #[rustc_allow_const_fn_unstable(const_eval_select)] // both impls have the exact same behaviour | ||
| pub const fn memchr(x: u8, text: &[u8]) -> Option<usize> { | ||
| // Fast path for small slices. | ||
| if text.len() < 2 * USIZE_BYTES { | ||
| return memchr_naive(x, text); | ||
| } | ||
|
|
||
| memchr_aligned(x, text) | ||
| } | ||
|
|
||
| #[inline] | ||
| const fn memchr_naive(x: u8, text: &[u8]) -> Option<usize> { | ||
| let mut i = 0; | ||
|
|
||
| // FIXME(const-hack): Replace with `text.iter().pos(|c| *c == x)`. | ||
| while i < text.len() { | ||
| if text[i] == x { | ||
| return Some(i); | ||
| } | ||
|
|
||
| i += 1; | ||
| } | ||
|
|
||
| None | ||
| } | ||
|
|
||
| #[rustc_allow_const_fn_unstable(const_eval_select)] // fallback impl has same behavior | ||
| const fn memchr_aligned(x: u8, text: &[u8]) -> Option<usize> { | ||
| // The runtime version behaves the same as the compiletime version, it's | ||
| // just more optimized. | ||
| const_eval_select!( | ||
| @capture { x: u8, text: &[u8] } -> Option<usize>: | ||
| @capture { x: u8 = x, text: &[u8] = text } -> Option<usize>: | ||
| if const { | ||
| memchr_naive(x, text) | ||
| } else { | ||
| // Scan for a single byte value by reading two `usize` words at a time. | ||
| // | ||
| // Split `text` in three parts | ||
| // - unaligned initial part, before the first word aligned address in text | ||
| // - body, scan by 2 words at a time | ||
| // - the last remaining part, < 2 word size | ||
| let mut i = 0; | ||
|
|
||
| // search up to an aligned boundary | ||
| let len = text.len(); | ||
| let ptr = text.as_ptr(); | ||
| let mut offset = ptr.align_offset(USIZE_BYTES); | ||
|
|
||
| if offset > 0 { | ||
| offset = offset.min(len); | ||
| let slice = &text[..offset]; | ||
| if let Some(index) = memchr_naive(x, slice) { | ||
| return Some(index); | ||
| // FIXME(const-hack): Replace with `text.iter().pos(|c| *c == x)`. | ||
| while i < text.len() { | ||
| if text[i] == x { | ||
| return Some(i); | ||
| } | ||
| } | ||
|
|
||
| // search the body of the text | ||
| let repeated_x = usize::repeat_u8(x); | ||
| while offset <= len - 2 * USIZE_BYTES { | ||
| // SAFETY: the while's predicate guarantees a distance of at least 2 * usize_bytes | ||
| // between the offset and the end of the slice. | ||
| unsafe { | ||
| let u = *(ptr.add(offset) as *const usize); | ||
| let v = *(ptr.add(offset + USIZE_BYTES) as *const usize); | ||
| i += 1; | ||
| } | ||
|
|
||
| // break if there is a matching byte | ||
| let zu = contains_zero_byte(u ^ repeated_x); | ||
| let zv = contains_zero_byte(v ^ repeated_x); | ||
| if zu || zv { | ||
| break; | ||
| } | ||
| } | ||
| offset += USIZE_BYTES * 2; | ||
| None | ||
| } else { | ||
| unsafe extern "C" { | ||
| // Provided in either libc or compiler-builtins. | ||
| fn memchr( | ||
| s: *const c_void, | ||
| c: c_int, | ||
| n: usize, | ||
| ) -> *mut c_void; | ||
| } | ||
|
|
||
| // Find the byte after the point the body loop stopped. | ||
| // FIXME(const-hack): Use `?` instead. | ||
| // FIXME(const-hack, fee1-dead): use range slicing | ||
| let slice = | ||
| // SAFETY: offset is within bounds | ||
| unsafe { super::from_raw_parts(text.as_ptr().add(offset), text.len() - offset) }; | ||
| if let Some(i) = memchr_naive(x, slice) { Some(offset + i) } else { None } | ||
| // SAFETY: | ||
| // The pointer and length come from a slice reference and thus | ||
| // describe a valid memory region. Since the reference is a `&[u8]`, | ||
| // every byte contained therein is interpretable as an initialized | ||
| // byte. | ||
| let res = unsafe { memchr(text.as_ptr().cast(), x as c_int, text.len()) }; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the slice is empty, it might legally be a no-provenance pointer. C requires the For some of the other operations, we assume that they work on arbitrary pointers if the length is 0, and there are recent proposals to the C standard to officially bless this. Do those proposals cover
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd have though the validity requirement was only for null pointers, which cannot occur here since the pointer comes from a slice. In any case, N3322 covers
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In C it is UB to even create a pointer that doesn't point to an allocation (except for null), so I see no way to argue that it would be allowed to pass such a pointer when even the much-more-well-defined null pointer is disallowed.
Okay, good. And we have to figure out what to do with Miri. Currently, if you directly invoke memcpy or any of the others in Miri, it still enforces the C23 rules. Only if you directly invoke Rust's intrinsics do we allow arbitrary dangling pointers for size 0. For memchr we thus have to either also introduce an intrinsic, or we have to make Miri implement the rules of https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3322.pdf in the hopes that the committee will include them in the next standard. Do you have any idea how far along that proposal is through the process? (Cc @nikic)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But one-past-the-end pointers are allowed to be created, and so I'd expect the following to work. char buf[16];
memchr(&buf + 16, 42, 0);
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's entirely fine. But this isn't: memchr((char*)16, 42, 0);
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But it means that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The int2ptr is UB. And because such an int2ptr is UB I don't think it is valid to pass such a ptr to C even if you can create it in Rust UB-free. It's basically a validity invariant for pointers in C that they must be associated with a live allocation.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that this is all too relevant here. I hope you agree that the following is the maximally-strict story code for fn memchr(mut s: *const u8, v: i32, n: usize) -> *mut u8 {
assert_unchecked(!s.is_null());
let needle = v as u8;
while n > 0 {
let byte = s.read();
if byte == needle {
return s.cast_mut();
}
s = s.add(1);
n -= 1;
}
ptr::null_mut()
}But if you hold that So this poses the fundamental question of whether the Rust AM can be lowered to an abstract machine that has undefined behaviour not expressible in Rust. While this would be a correct lowering, I don't think it should be allowed, as it makes it impossible to reason about FFI in Rust without reasoning about an arbitrary, cross-language AM.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you call code written in a different language, you have to follow its rules. Calling IOW, you have to make both Abstract Machines happy. And in this case, while the Rust AM is happy, the C AM is not. (Or at least, I haven't yet seen a good argument for why it should be happy.) |
||
| if res.is_null() { | ||
| None | ||
| } else { | ||
| // SAFETY: `res` is non-null, and thus is guaranteed to lie | ||
| // within the memory region passed to `memchr`. | ||
| let index = unsafe { res.offset_from_unsigned(ptr) }; | ||
| Some(index) | ||
| } | ||
| } | ||
| ) | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The c-b part should land as a PR to rust-lang/compiler-builtins first. We're not running all its checks and benchmarks in r-l/r
View changes since the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think this would be good to add regardless of whether or not we use it, if only for the purpose of helping
no-stdbuilds)