Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/compiler-builtins/compiler-builtins/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#![feature(linkage)]
#![feature(repr_simd)]
#![feature(macro_metavar_expr_concat)]
#![feature(portable_simd)]
#![feature(rustc_attrs)]
#![cfg_attr(f16_enabled, feature(f16))]
#![cfg_attr(f128_enabled, feature(f128))]
Expand Down
133 changes: 133 additions & 0 deletions library/compiler-builtins/compiler-builtins/src/mem/memchr_impl.rs

@tgross35 tgross35 Jul 10, 2026

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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-std builds)

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(

@RalfJung RalfJung Jul 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation does not implement the contract specified for memchr by the C standard. That standard says:

This function behaves as if it reads the bytes sequentially and stops as soon as a matching bytes is found: if the array pointed to by ptr is smaller than count, but the match is found within the array, the behavior is well-defined.

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 libc::memchr.

View changes since the review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.)

@RalfJung RalfJung Jul 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 memchr is the naive implementation, but this seems like a whacky state of affairs right? And if glibc's implementation is also non-conformant, but is used as a gold standard implementation of memchr (IMO it is at least), then I think it should be fine for us to use the same implementation tricks?

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 memchr beyond the naive one, or if it's something specific about the code in front of us here. So I think being more concrete here would be edifying.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe all such implementations in glibc are in fully separate .S files, so their actual behavior is not visible to the C compiler. At that level, it only needs to care about more direct hardware constraints, like making sure any possible over-reads are still in the same page as the matching byte, so it doesn't cause a rogue segfault. e.g. A fully aligned SIMD read will also be page-aligned, so that's fine.

@BurntSushi BurntSushi Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there is a difference between "reading bytes beyond where s + n ends" and "reading bytes beyond the first occurence of c in s up to s + n." It sounds like @RalfJung is talking about the latter, in which case, I don't see why it's relevant whether Assembly is used or not. (I absolutely acknowledge that writing the impl in Assembly is relevant to the former case, and @cuviper is absolutely correct there. I just don't think we're talking about that case here.)

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 memchr crate too. And perhaps all non-naive implementations of memchr. To be doubly clear, I find this statement to be controversial:

which means that the naive implementation is the only legal implementation

@RalfJung RalfJung Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rephrase what you're saying more concretely?

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 libc::memchr.
However, if memchr is implemented by doing 8-byte reads (such as the implementation in this PR), it would read beyond the bounds of haystack.

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 memchr itself is subject to the rules of the Abstract Machine. If it is written in Rust or C, then I would say that it is, though in the absence of LTO one can make complicated arguments about separate translation units. If it is written in assembly, and if the implementation is careful not to cause any segfaults by accessing any pages beyond the one that has the first hit, then that assembly code has a story that justifies its correctness.

So this Rust-written implementation here is not a valid replacement for a libc's memchr, unless we want to make complicated arguments about separate translation units and rely on no-LTO. I don't know if we want it to be a valid replacement for libc's memchr, but if not, then that should be clearly documented and we should be sure it doesn't accidentally get used as such.

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 memchr crate too.

I don't think there is a problem for the memchr crate unless it is being used as a replacement for libc::memchr. For a pure Rust function it is entirely reasonable to demand that the entire haystack of size n must be dereferenceable memory. But the C function happens to be specified in a way that says that haystack only has to be dereferenceable up until the first occurrence of needle -- a spec we'd never use in Rust, but we're not in charge of defining the spec of the C function.

@joboet joboet Jul 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly because if it is, then there is perhaps something problematic about the memchr crate too.

Given that your crate makes no attempt to be identical to C's memchr, no. This whole thing is not a problem for functions that take a slice, as then the entire memory region has to be valid anyway.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 n here. Thank you for the explanation @RalfJung!

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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"),

@BurntSushi BurntSushi Jul 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the specific blocker to using the memchr crate here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot use memchr in a normal fashion as it depends on core, which would create a circular dependency. It is possible to solve that, just like how backtrace is used in std even though it depends on std, but doing so is very complicated (backtrace is included as a git subtree and then #[path]-imported as a module from std. This requires a lot of cooperation on the part of backtrace, as it has to both compile as a crate and as a module in a bigger crate.) Hence I wanted to avoid that whole mess...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked the codegen for neon here?

Yes. first_set is implemented without movemask on all non-SSE-platforms.

) => {
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) }
}
10 changes: 10 additions & 0 deletions library/compiler-builtins/compiler-builtins/src/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// memcpy/memmove/memset have optimized implementations on some architectures
#[cfg_attr(all(feature = "arch", target_arch = "x86_64"), path = "x86_64.rs")]
mod impls;
mod memchr_impl;

intrinsics! {
#[mem_builtin]
Expand Down Expand Up @@ -67,4 +68,13 @@ intrinsics! {
pub unsafe extern "C" fn strlen(s: *const core::ffi::c_char) -> usize {
impls::c_string_length(s)
}

#[mem_builtin]
pub unsafe extern "C" fn memchr(
s: *const core::ffi::c_void,
c: core::ffi::c_int,
n: usize
) -> *mut core::ffi::c_void {
memchr_impl::memchr(s, c, n)
}
}
4 changes: 2 additions & 2 deletions library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
// FIXME: Fill me in with more detail when the interface settles
//! This library is built on the assumption of a few existing symbols:
//!
//! * `memcpy`, `memmove`, `memset`, `memcmp`, `bcmp`, `strlen` - These are core memory routines
//! * `memcpy`, `memmove`, `memset`, `memcmp`, `bcmp`, `strlen`, `memchr` - These are core memory routines
//! which are generated by Rust codegen backends. Additionally, this library can make explicit
//! calls to `strlen`. Their signatures are the same as found in C, but there are extra
//! calls to `strlen` and `memchr`. Their signatures are the same as found in C, but there are extra
//! assumptions about their semantics: For `memcpy`, `memmove`, `memset`, `memcmp`, and `bcmp`, if
//! the `n` parameter is 0, the function is assumed to not be UB, even if the pointers are NULL or
//! dangling. (Note that making extra assumptions about these functions is common among compilers:
Expand Down
105 changes: 33 additions & 72 deletions library/core/src/slice/memchr.rs
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.
///
Expand All @@ -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()) };

@RalfJung RalfJung Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 memchr pointer to always be valid, even if the length is 0. In that case, this call would cause UB (that Miri would flag).

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 memchr as well?

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 memchr.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In any case, N3322 covers memchr.

Okay, good.
Still, please add this to the libcore crate-level docs as an assumption we make, since it's not yet in a published standard.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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);

@RalfJung RalfJung Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it means that memchr cannot make any reads if n is zero – it cannot assume anything about the memory before the pointer, and it must not read from the pointer. So I'd say that your example is sound, too (modulo the int2ptr).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 memchr:

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 s has to be associated with a live allocation, then it isn't. Because Rust's AM does not have a way to test for live allocations, there wouldn't be any story code that matches the behaviour of memchr.

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.

@RalfJung RalfJung Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 memchr with (char*)16 is entirely unproblematic on the Rust side (so writing story code in Rust misses the point), but the value you are passing to the C Abstract Machine simply does not make sense there, and hence you have UB.

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)
}
}
)
}
Expand Down
Loading