core: use the platform's memchr#159090
Conversation
|
cc @tgross35
cc @rust-lang/wg-const-eval |
rustbot
added
A-compiler-builtins
|
|
| // 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()) }; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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);There was a problem hiding this comment.
Yeah that's entirely fine.
But this isn't:
memchr((char*)16, 42, 0);There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
|
The job Click to see the possible cause of the failure (guessed by this bot)Important For more information how to resolve CI failures of this job, visit this link. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
(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)
rustbot
added
S-waiting-on-author
|
Nominating for @rust-lang/libs because there are some tradeoffs here. Notably in #t-libs > Using the platform's memchr, the possibility of using the The top post says:
We could also use the @rustbot label +I-libs-nominated |
rustbot
added
the
I-libs-nominated
|
IMO, the In contrast, the Here are some benchmarks comparing the And |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
core: use the platform's `memchr`
|
💔 Test for bb887d8 failed: CI. Failed job:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| _ => [usize; 2], | ||
| }; | ||
|
|
||
| pub unsafe fn memchr( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
"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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Mostly because if it is, then there is perhaps something problematic about the
memchrcrate 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| // These targets have efficient SIMD acceleration. | ||
| any( | ||
| all(target_arch = "x86_64", target_feature = "sse2"), | ||
| all(target_arch = "aarch64", target_feature = "neon"), |
There was a problem hiding this comment.
Have you checked the codegen for neon here? It doesn't have movemask. The memchr crate jumps through a lot of hoops to implement this efficiently: https://github.com/BurntSushi/memchr/blob/96c943f1ca5672abcb86d5910cd3df228773ff73/src/vector.rs#L292-L465
There was a problem hiding this comment.
What is the specific blocker to using the memchr crate here?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Have you checked the codegen for neon here?
Yes. first_set is implemented without movemask on all non-SSE-platforms.
9 participants
The custom, SWAR-based
memchrimplementation is in many cases slower than the platform's own implementation. This is especially the case on GNU/Linux asglibcwill use IFUNCs to select the most efficient implementation at link time. Thus, this PR makescoreunconditionally use the platformmemchr.Although one could also imagine including the
memchrcrate intocore, platforms are in my opinion in a much better place to optimise their implementation – even some don't – since they can e.g. update their dynamically-linkedlibcto take advantage of new target features and know better than us whether to optimise for binary-size or performance.Just like #94079 did, this PR adds a new symbol to the ones required by
core. Given thatmemchris in the C standard just likestrlenis, I expect every platform that already provides the other symbols will also providememchr. This PR also adds a reasonably optimisedmemchrimplementation tocompiler_builtinsso that all targets that use the memory routines contained therein will continue to work.r? @tgross35