Open beepster4096 opened 2 years ago
@rustbot label +A-codegen +A-LLVM
Note that if we follow my suggested answer to https://github.com/rust-lang/unsafe-code-guidelines/issues/77, then &NonNull
may actually point to 0, so the suggested optimization would be incorrect. Under my proposal, only data that you actually use by-value is required to comply with the validity invariant.
So, basically, under my proposal the following are not equivalent:
pub fn use_nonnull1(p: &NonNull) {
std::hint::black_box(p.pointer);
}
pub fn use_nonnull2(p: &NonNull) {
let p = *p;
std::hint::black_box(p.pointer);
}
Some variations on the theme for reference: (Godbolt)
; std::hint::black_box(p.pointer);
define void @use_nonnull_a(i8** align 8 %p) unnamed_addr #1 {
%0 = bitcast i8** %p to {}**
%_3 = load {}*, {}** %0, align 8
%_2 = call {}* @core_hint_black_box({}* %_3)
br label %bb1
bb1: ; preds = %start
ret void
}
; std::hint::black_box({*p}.pointer);
define void @use_nonnull_b(i8** align 8 %p) unnamed_addr #1 {
%p1 = load i8*, i8** %p, align 8, !nonnull !3, !noundef !3
%_4 = bitcast i8* %p1 to {}*
%_3 = call {}* @core_hint_black_box({}* %_4)
br label %bb1
bb1: ; preds = %start
ret void
}
It looks like {*p}.pointer
(note the {}
to induce a typed copy) does get the !nonnull
metadata.
That said, even with reference validity ignoring memory contents, I'd still expect the deref/retag to assert memory validity and justify the !nonnull
metadata here. But even if it doesn't, I'd still expect the rustc attribute to impact the copy from the field; instead of a typed copy *const () -> *const ()
, a "typed copy" #[rustc_layout_scalar_valid_range_start(1)] *const () -> *const ()
. The compiler literally knows that more is going on here than a simple typed copy of a *const ()
field, it's just forgetting to write that down.
IOW, this feels like a bug in how #[rustc_layout_scalar_valid_range_start(1)]
works independent of how the memory model works; the goal of the attribute is that loading the field should be known in the valid range despite coming out as an unbounded type.
I'd still expect the deref/retag to assert memory validity
I don't think they should do anything like that (and in Miri, they don't).
This code does not have UB according to how I think Rust should be specified (and accordingly, it passes in Miri):
#![feature(rustc_attrs, bench_black_box)]
#[rustc_layout_scalar_valid_range_start(1)]
pub struct NonNull {
pointer: *const (),
}
#[no_mangle]
pub fn use_nonnull(p: &NonNull) {
std::hint::black_box(p.pointer);
}
fn main() {
let ptr = 0usize as *const ();
use_nonnull(unsafe { std::mem::transmute(&ptr) });
}
Here is a version of the code that does have UB:
#![feature(rustc_attrs, bench_black_box)]
#[rustc_layout_scalar_valid_range_start(1)]
#[derive(Copy, Clone)]
pub struct NonNull {
pointer: *const (),
}
#[no_mangle]
pub fn use_nonnull(p: &NonNull) {
std::hint::black_box(*p);
}
fn main() {
let ptr = 0usize as *const ();
use_nonnull(unsafe { std::mem::transmute(&ptr) });
}
You just have to actually tell the compiler that you care about the NonNull
, not just its field.
I have no objections to making sure that that code optimizes well. :)
The following code generates the following LLVM IR. (Godbolt)
The load here does not have
!nonnull
metadata on it, even though rustc knows the field will never be null. This is not currently a problem in practice sincestd::ptr::NonNull
's field is only accessed through a method that takes theNonNull
by value. This copies theNonNull
instead of the underlying pointer, which does properly emit!nonnull
. However with #95576, dereferencing a box will now go though the underlying pointer field, which can trigger this issue.