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