Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly #114382

scottmcm · 2023-08-02T19:49:17Z

As discussed in #113435, this lets the backends be the place that can have the "don't call the function if n == 0" logic, if it's needed for the target. (I didn't actually add those checks, though, since as I understood it we didn't actually need them on known targets?)

Doing this also let me make it const (unstable), which I don't think extern "C" fn memcmp can be.

cc @RalfJung @Amanieu

rustbot · 2023-08-02T19:49:23Z

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-08-02T19:49:26Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

rustbot · 2023-08-02T21:57:02Z

The Miri subtree was changed

cc @rust-lang/miri

oli-obk · 2023-08-03T07:50:59Z

library/core/src/intrinsics.rs

+ ///
+ /// # Safety
+ ///
+ /// * If `bytes` is zero, `left` and `right` must be non-null.


Suggested change

/// * If `bytes` is zero, `left` and `right` must be non-null.

/// * Even if `bytes` is zero, `left` and `right` must be non-null.

otherwise this reads surprisingly if you don't already know about this

RalfJung · 2023-08-03T10:17:56Z

compiler/rustc_const_eval/src/interpret/intrinsics.rs

+ let left = self.read_pointer(left)?;
+ let right = self.read_pointer(right)?;
+ let n = Size::from_bytes(self.read_target_usize(byte_count)?);
+ let result = if n == Size::ZERO {


This shouldn't be needed. Please copy the current memcmp from Miri.

https://github.com/rust-lang/miri/blob/0556996399b8315625df0be4fcf6a7fc0e30a2b0/src/shims/foreign_items.rs#L686-L705

Ah, I didn't know that it would already do the right null checks without also requiring "real" alloc ids. I'll remove the extra checks.

(And yes, that's where I got read_bytes_ptr_strip_provenance 🙂)

@rustbot author

Done with https://github.com/rust-lang/rust/compare/d29af6737206d64f834a73a185356bf29958ab78..1f4b525312600f65b7c4d93e459bf2a5953ea5d4

Looks good. :)

The checks Miri's functions do by default should exactly match what the docs call "valid for reads of size N", that's why you don't need any extra checks. This also ensures we apply the exact same checks everywhere (direct memory accesses, memcpy, memset, memcmp, ...).

Nice! I like the synergy between that and your notes about how to document it.

(More updates coming for the other comments.)

RalfJung · 2023-08-03T10:19:02Z

library/core/src/intrinsics.rs

+ /// # Safety
+ ///
+ /// * If `bytes` is zero, `left` and `right` must be non-null.
+ ///
+ /// * If `bytes` is non-zero,
+ ///
+ /// * `left` must be [valid] for reads of `bytes` bytes,
+ /// all of which must be initialized.
+ ///
+ /// * `right` must be [valid] for reads of `bytes` bytes,
+ /// all of which must be initialized.


I think this should follow the usual documentation of all our intrinsics: left and right must be valid for reads of size bytes. That's all we need to say.

We already say that as far as Rust is concerned, 4 as *const u8 is valid for reads of size 0. We shouldn't special-case "if bytes is zero" here in the docs nor in the Miri implementation.

In particular, if we ever declare that even the null pointer is valid for zero-sized reads and writes (which we may, rust-lang/opsem-team#12), then even passing null to these intrinsics should be allowed.

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs

compiler/rustc_codegen_llvm/src/intrinsic.rs

RalfJung · 2023-08-03T10:23:02Z

tests/ui/consts/const-compare-bytes-ub.stderr

+ --> $DIR/const-compare-bytes-ub.rs:34:9
+ |
+LL | compare_bytes([1].as_ptr(), MaybeUninit::uninit().as_ptr(), 1)
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reading memory at alloc25[0x0..0x1], but memory is uninitialized at [0x0..0x1], and this operation requires initialized memory


Can you add a test that involves provenance? As in, pass in a buffer that contains pointers.

Something like compare_bytes([&0].as_ptr(), [&0].as_ptr(), size_of_val(&0)) should probably do it

scottmcm · 2023-08-03T18:15:17Z

tests/ui/proc-macro/meta-macro-hygiene.stdout

@@ -18,7 18,7 @@ Respanned: TokenStream [Ident { ident: "$crate", span: $DIR/auxiliary/make-macro
 use core /* 0#1 */::prelude /* 0#1 */::rust_2018 /* 0#1 */::*;
 #[macro_use /* 0#1 */]
 extern crate core /* 0#1 */;
-extern crate compiler_builtins /* 443 */ as _ /* 0#1 */;
+extern crate compiler_builtins /* 444 */ as _ /* 0#1 */;


I have no idea how this is impacted, but 🤷

klensy · 2023-08-06T10:10:11Z

Comments here too?

rust/library/core/src/slice/cmp.rs

Line 77 in 11467b1

// Use memcmp for bytewise equality when the types allow

rust/library/core/src/slice/cmp.rs

Line 186 in 11467b1

// memcmp compares a sequence of unsigned bytes lexicographically.

rust/library/core/src/lib.rs

Line 23 in 11467b1

 //! * `memcpy`, `memcmp`, `memset`, `strlen` - These are core memory routines which are 

rust/compiler/rustc_codegen_llvm/src/context.rs

Lines 893 to 898 in 11467b1

 // This isn't an "LLVM intrinsic", but LLVM's optimization passes 

 // recognize it like one and we assume it exists in `core::slice::cmp` 

 match self.sess().target.arch.as_ref() { 

 "avr" | "msp430" => ifn!("memcmp", fn(ptr, ptr, t_isize) -> t_i16), 

 _ => ifn!("memcmp", fn(ptr, ptr, t_isize) -> t_i32), 

 }

cjgillot · 2023-08-06T11:31:25Z

@bors r-

RalfJung · 2023-08-06T11:38:23Z

This one

rust/library/core/src/lib.rs

Line 23 in 11467b1

 //! * `memcpy`, `memcmp`, `memset`, `strlen` - These are core memory routines which are 

has an ongoing PR clarifying our documentation: #114412.

Co-authored-by: Ralf Jung <[email protected]>

scottmcm · 2023-08-06T23:54:28Z

Updated those comments in https://github.com/rust-lang/rust/compare/1ac2710780489b50a5feb5e838f8c81a66720dbc..febffe212880dfef083bb860de45abc5aa075a3a, and rebased to pick up another proc-macro test change.

@bors r=cjgillot

bors · 2023-08-06T23:54:30Z

📌 Commit 75277a6 has been approved by cjgillot

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#98935 (Implement `Option::take_if`) - rust-lang#114093 (Add regression test for `echo 'mod unknown;' | rustc -`) - rust-lang#114229 (Nest tests/codegen/sanitizer*.rs tests in sanitizer dir) - rust-lang#114230 (Nest other codegen test topics) - rust-lang#114362 (string.rs: remove "Basic usage" text) - rust-lang#114365 (str.rs: remove "Basic usage" text) - rust-lang#114382 (Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly) - rust-lang#114549 (Style fix and refactor on resolve diagnostics) r? `@ghost` `@rustbot` modify labels: rollup

…r=cjgillot Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly As discussed in rust-lang#113435, this lets the backends be the place that can have the "don't call the function if n == 0" logic, if it's needed for the target. (I didn't actually *add* those checks, though, since as I understood it we didn't actually need them on known targets?) Doing this also let me make it `const` (unstable), which I don't think `extern "C" fn memcmp` can be. cc `@RalfJung` `@Amanieu`

document our assumptions about symbols provided by the libc LLVM makes assumptions about `memcmp`, `memmove`, and `memset` that go beyond what the C standard guarantees -- see https://reviews.llvm.org/D86993. Since we use LLVM, we are inheriting these assumptions. With rust-lang#114382 we are also making a similar assumption about `memcmp`, so I added that to the list. Fixes rust-lang/unsafe-code-guidelines#426.

document our assumptions about symbols provided by the libc LLVM makes assumptions about `memcmp`, `memmove`, and `memset` that go beyond what the C standard guarantees -- see https://reviews.llvm.org/D86993. Since we use LLVM, we are inheriting these assumptions. With rust-lang/rust#114382 we are also making a similar assumption about `memcmp`, so I added that to the list. Fixes rust-lang/unsafe-code-guidelines#426.

…r=cjgillot Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly As discussed in rust-lang#113435, this lets the backends be the place that can have the "don't call the function if n == 0" logic, if it's needed for the target. (I didn't actually *add* those checks, though, since as I understood it we didn't actually need them on known targets?) Doing this also let me make it `const` (unstable), which I don't think `extern "C" fn memcmp` can be. cc `@RalfJung` `@Amanieu`

rustbot assigned cjgillot Aug 2, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 2, 2023

scottmcm mentioned this pull request Aug 2, 2023

slice: avoid calling memcmp with size 0 #113435

Closed

This comment has been minimized.

Sign in to view

scottmcm force-pushed the compare-bytes-intrinsic branch from 51c1830 to e5b23a6 Compare August 2, 2023 20:38

This comment has been minimized.

Sign in to view

scottmcm force-pushed the compare-bytes-intrinsic branch from e5b23a6 to 029d660 Compare August 2, 2023 21:10

This comment has been minimized.

Sign in to view

scottmcm force-pushed the compare-bytes-intrinsic branch from 029d660 to 4bccdab Compare August 2, 2023 21:28

This comment has been minimized.

Sign in to view

scottmcm force-pushed the compare-bytes-intrinsic branch from 4bccdab to f732b5a Compare August 2, 2023 21:57

This comment has been minimized.

Sign in to view

scottmcm force-pushed the compare-bytes-intrinsic branch from f732b5a to d29af67 Compare August 2, 2023 22:28

oli-obk reviewed Aug 3, 2023

View reviewed changes

RalfJung reviewed Aug 3, 2023

View reviewed changes

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs Show resolved Hide resolved

RalfJung reviewed Aug 3, 2023

View reviewed changes

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs Show resolved Hide resolved

RalfJung reviewed Aug 3, 2023

View reviewed changes

compiler/rustc_codegen_llvm/src/intrinsic.rs Show resolved Hide resolved

RalfJung reviewed Aug 3, 2023

View reviewed changes

RalfJung mentioned this pull request Aug 3, 2023

document our assumptions about symbols provided by the libc #114412

Merged

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2023

scottmcm commented Aug 3, 2023

View reviewed changes

scottmcm force-pushed the compare-bytes-intrinsic branch from d29af67 to 1f4b525 Compare August 4, 2023 06:01

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 6, 2023

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 6, 2023

scottmcm force-pushed the compare-bytes-intrinsic branch from 1ac2710 to febffe2 Compare August 6, 2023 22:04

This comment has been minimized.

Sign in to view

scottmcm and others added 2 commits August 6, 2023 15:47

Add a new compare_bytes intrinsic instead of calling memcmp directly

502af03

Apply suggestions from code review

75277a6

Co-authored-by: Ralf Jung <[email protected]>

scottmcm force-pushed the compare-bytes-intrinsic branch from febffe2 to 75277a6 Compare August 6, 2023 22:47

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 6, 2023

matthiaskrgr mentioned this pull request Aug 7, 2023

Rollup of 8 pull requests #114569

Merged

bors merged commit cbe2522 into rust-lang:master Aug 7, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Aug 7, 2023

RalfJung mentioned this pull request Aug 15, 2023

C mem function shims: consistently treat "invalid" pointers as UB rust-lang/miri#2972

Merged

scottmcm deleted the compare-bytes-intrinsic branch August 15, 2023 20:01

zhassan-aws mentioned this pull request Aug 16, 2023

Upgrade Rust toolchain to 2023-08-14 model-checking/kani#2688

Merged

4 tasks

krtab mentioned this pull request Oct 17, 2023

Non memcmp slice comparison optimization #113654

Closed

scottmcm mentioned this pull request Jun 20, 2024

Tracking Issue for const_cstr_from_ptr #113219

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly #114382

Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly #114382

scottmcm commented Aug 2, 2023

rustbot commented Aug 2, 2023

rustbot commented Aug 2, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Aug 2, 2023

This comment has been minimized.

oli-obk Aug 3, 2023

RalfJung Aug 3, 2023

scottmcm Aug 3, 2023

scottmcm Aug 4, 2023

RalfJung Aug 4, 2023

scottmcm Aug 4, 2023

RalfJung Aug 3, 2023 •

edited

Loading

RalfJung Aug 3, 2023

RalfJung Aug 3, 2023

RalfJung Aug 4, 2023

scottmcm Aug 3, 2023

klensy commented Aug 6, 2023

cjgillot commented Aug 6, 2023

RalfJung commented Aug 6, 2023 •

edited

Loading

This comment has been minimized.

scottmcm commented Aug 6, 2023

bors commented Aug 6, 2023

	/// * If `bytes` is zero, `left` and `right` must be non-null.
	/// * Even if `bytes` is zero, `left` and `right` must be non-null.

Add a new compare_bytes intrinsic instead of calling memcmp directly #114382

Add a new compare_bytes intrinsic instead of calling memcmp directly #114382

Conversation

scottmcm commented Aug 2, 2023

rustbot commented Aug 2, 2023

rustbot commented Aug 2, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Aug 2, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klensy commented Aug 6, 2023

cjgillot commented Aug 6, 2023

RalfJung commented Aug 6, 2023 • edited Loading

This comment has been minimized.

scottmcm commented Aug 6, 2023

bors commented Aug 6, 2023

Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly #114382

Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly #114382

RalfJung Aug 3, 2023 •

edited

Loading

RalfJung commented Aug 6, 2023 •

edited

Loading