ptr::read unable to assume slice is always not null

[AnsonYeung]

Expects the following function to always return true since it's undefined behaviour otherwise.

use std::ptr;

pub unsafe fn test(s: &Vec<&[i32]>, idx: usize) -> bool {
    unsafe { Some(ptr::read(s.as_ptr().add(idx))).is_some() }
}

Actual result:

example::test:
        mov     rax, qword ptr [rdi + 8]
        shl     rsi, 4
        cmp     qword ptr [rax + rsi], 0
        setne   al
        ret

https://rust.godbolt.org/z/xzcTr15Yj
Causes #106368

[AnsonYeung]

This is related to the function ptr::read, replacing it with deref produces the expected result.

use std::ptr;

pub unsafe fn test_ptr_read(s: *const &[i32]) -> bool {
    let x: &[i32] = unsafe { ptr::read(s) };
    Some(x).is_some()
}

pub unsafe fn test_deref(s: *const &[i32]) -> bool {
    let x: &[i32] = unsafe { *s };
    Some(x).is_some()
}

pub fn test_expected(s: &[i32]) -> bool {
    Some(s).is_some()
}

example::test_ptr_read:
        cmp     qword ptr [rdi], 0
        setne   al
        ret

example::test_deref:
        mov     al, 1
        ret

example::test_expected:
        mov     al, 1
        ret

https://rust.godbolt.org/z/z3bKfac6s

[saethlin]

This looks a lot like #93011 to me. ptr::read does a copy_nonoverlapping into a MaybeUninit, perhaps that wrapping also causes LLVM to lose the nonnull.

[Noratrieb]

This can be minimized even further to just this

#[no_mangle]
pub unsafe fn bad(ptr: *const &i32) -> bool {
    unsafe { Some(ptr::read(ptr)).is_some() }
}

which on master emits (rustc read.rs --crate-type=lib -Copt-level=3 --emit=asm)

bad:
	.cfi_startproc
	cmpq	$0, (%rdi)
	setne	%al
	retq

This actually gets fixed by applying

diff --git a/compiler/rustc_codegen_llvm/src/abi.rs b/compiler/rustc_codegen_llvm/src/abi.rs
index a6fd2a7de6b..30006ac5e7b 100644
--- a/compiler/rustc_codegen_llvm/src/abi.rs
+++ b/compiler/rustc_codegen_llvm/src/abi.rs
@@ -81,7 +81,6 @@ fn should_use_mutable_noalias(cx: &CodegenCx<'_, '_>) -> bool {
             } else {
                 attrs.push(llvm::CreateDereferenceableOrNullAttr(cx.llcx, deref));
             }
-            regular -= ArgAttribute::NonNull;
         }
         for (attr, llattr) in OPTIMIZATION_ATTRIBUTES {
             if regular.contains(attr) {

which is weird, because this shouldn't change anything as dereferenceable implies nonnull (at least as far as I know and opts seem to rely on this).

With this applied, I now get

bad:
	.cfi_startproc
	movb	$1, %al
	retq

which is good.

But, this still doesn't help with the original example in the issue, which is the same with and without the patch.

[clubby789]

If we add a !nonnull when the slice pointer is loaded then the codegen (-Copt-level=3 -Cdebuginfo=0 -Cno-prepopulate-passes ) for this case looks better (godbolt). The IR before the generated assume isn't removed, but it codegens to just a mov al, 1; ret

pub unsafe fn test_ptr_read(s: *const &[()]) -> bool {
    let x: &[()] = unsafe { std::ptr::read(s) };
    Some(x).is_some()
}

[clubby789]

It looks like the problem isn't the CopyNonOverlapping - rather, when building the load, the layout of MaybeUninit (used in ptr::read) is used, (even though we're loading from an assume_init), so we don't know of any niches. godbolt

[AnsonYeung]

After some more digging it seems the issue is reported back in 2020 in #73258 . Seems like if it is imported internally to the crate, it can realize the optimization for reference but not slice

pub unsafe fn indirect_read<T>(src: *const T) -> T {
    std::ptr::read(src)
}

pub unsafe fn test_ptr_read(s: *const &[i32]) -> bool {
    let x: &[i32] = unsafe { indirect_read(s) };
    Some(x).is_some()
}

https://godbolt.org/z/MTWEbsvvc

[scottmcm]

Test: https://github.com/rust-lang/rust/blob/master/tests/codegen/issues/issue-106369.rs