treat &raw (const|mut) UNSAFE_STATIC implied deref as safe

[workingjubilee]

Fixes #125833

As reported in that and related issues, static mut STATIC_MUT: T is very often used in embedded code, and is in many ways equivalent to static STATIC_CELL: SyncUnsafeCell<T>. The Rust expression of &raw mut STATIC_MUT and SyncUnsafeCell::get(&STATIC_CELL) are approximately equal, and both evaluate to *mut T. The library function is safe because it has declared itself to be safe. However, the raw ref operator is unsafe because all uses of static mut are considered unsafe, even though the static's value is not used by this expression (unlike, for example, &STATIC_MUT).

We can fix this unnatural difference by simply adding the proper exclusion for the safety check inside the THIR unsafeck, so that we do not declare it unsafe if it is not.

While the primary concern here is static mut, this change is made for all instances of an "unsafe static", which includes a static declared inside extern "abi" {}. Hypothetically, we could go as far as generalizing this to all instances of &raw (const|mut) *ptr, but today we do not, as we have not actually considered the range of possible expressions that use a similar encoding. We do not even extend this to thread-local equivalents, because they have less clear semantics.

[rustbot]

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[workingjubilee]

ah, miri.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[workingjubilee]

@traviscross you tagged #125833 as T-lang, should I be r?ing lang on this or something

[rustbot]

Failed to set assignee to ing: cannot assign: response: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/assignees#add-assignees-to-an-issue"}

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[traviscross]

@workingjubilee: On re-review, I've removed it. On first impression, it seemed potentially relevant to our recent discussions on various static mut issues, but looking again, I agree this is just a bug.

[workingjubilee]

@traviscross Yeah, it doesn't seem irrelevant, in a sense, but it's not... like... Big Discussion Time for T-lang, just sort of an "FYI" footnote maybe.

[RalfJung]

I am a bit confused... Why are extern statics treated differently? That does bot seem right to me.

[workingjubilee]

via email

man @RalfJung do you just like subscribe to all github PRs ever?

I am a bit confused... Why are extern statics treated differently? That does bot seem right to me.

Hmm I think I had gotten a bit muddled after reading things a few times, and had over-indexed on this comment here:

rust/compiler/rustc_mir_build/src/thir/cx/expr.rs

Lines 942 to 943 in 20be84a

	// We encode uses of statics as a `*&STATIC` where the `&STATIC` part is
	// a constant reference (or constant raw pointer for `static mut`) in MIR

which is dated, it seems, because what I should have done is glance at the desugaring code (which I did read several times, but what is more important is...)

rust/compiler/rustc_mir_build/src/thir/cx/expr.rs

Lines 944 to 958 in 20be84a

	Res::Def(DefKind::Static { .. }, id) => {
	let ty = self.tcx.static_ptr_ty(id);
	let temp_lifetime = self
	.rvalue_scopes
	.temporary_scope(self.region_scope_tree, expr.hir_id.local_id);
	let kind = if self.tcx.is_thread_local_static(id) {
	ExprKind::ThreadLocalRef(id)
	} else {
	let alloc_id = self.tcx.reserve_and_set_static_alloc(id);
	ExprKind::StaticRef { alloc_id, ty, def_id: id }
	};
	ExprKind::Deref {
	arg: self.thir.exprs.push(Expr { ty, temp_lifetime, span: expr.span, kind }),
	}
	}

and then glance behind static_ptr_ty: (this is what I did not do, or if I did I misread what I saw)

rust/compiler/rustc_middle/src/ty/util.rs

Lines 728 to 745 in 20be84a

	/// Get the type of the pointer to the static that we use in MIR.
	pub fn static_ptr_ty(self, def_id: DefId) -> Ty<'tcx> {
	// Make sure that any constants in the static's type are evaluated.
	let static_ty = self.normalize_erasing_regions(
	ty::ParamEnv::empty(),
	self.type_of(def_id).instantiate_identity(),
	);
	// Make sure that accesses to unsafe statics end up using raw pointers.
	// For thread-locals, this needs to be kept in sync with `Rvalue::ty`.
	if self.is_mutable_static(def_id) {
	Ty::new_mut_ptr(self, static_ty)
	} else if self.is_foreign_item(def_id) {
	Ty::new_imm_ptr(self, static_ty)
	} else {
	Ty::new_imm_ref(self, self.lifetimes.re_erased, static_ty)
	}
	}

@RalfJung Happy to change it so that it functions the same way for both extern statics and static mut, now that I've reread everything.

[workingjubilee]

Oh wait, you got tagged because of the miri diff, I see.

[RalfJung]

Oh wait, you got tagged because of the miri diff, I see.

Yes, exactly. ;) Touching Miri is enough to summon me.

Hmm I think I had gotten a bit muddled after reading things a few times, and had over-indexed on this comment here:

Ah, I see. Certainly it'd be good to update the misleading comment.

Now, whether this should be safe or not... I am not entirely sure. Raw pointers seem fine, but then there is the point that an extern static must be immutable -- should we have unsafe at every access to indicate that proof obligation? But the proof obligation exists even when the static is never used so that probably does not make sense?
Cc @rust-lang/opsem

[workingjubilee]

Raw pointers seem fine, but then there is the point that an extern static must be immutable -- should we have unsafe at every access to indicate that proof obligation?

Hmm, note this doesn't commit to anything except that acquiring the address itself is safe: you can't "actually access" it (defining "actually access" as to read or write or call methods on it or whatever), as far as I know, because that would require another deref. It does imply that we have to be able to, at this point, answer some potentially-convoluted questions like "so uhh if we land weak linkage how does that... work... with this..."

[workingjubilee]

It feels clear, to me, that static mut should allow ptr::addr_of!(STATIC_MUT) without unsafe. But yes, I am not as sure about extern static. I must admit I am holding on to that originally-drafted PR's commit via reverting it instead of squashing it away because I'm curious to see if T-opsem thinks that there should be a difference.

[digama0]

I don't see any reason addr_of{_mut}!(STATIC_MUT) should be unsafe.

[RalfJung]

Note that that this is in particular about extern statics.

[workingjubilee]

Yes, the specific question for T-opsem here is "Should

extern "C" {
    static IMM_EXTERN: Type;
}

have the same safety conditions for the &raw {const,mut} NAME operation as

static mut STATIC_MUT: Type = Type::new();

does, or should they be different?"

Part of the problem is that (as I understand it):

• static mut
  • mutable
  • guaranteed to be initialized
  • safety conditions are about data races
• extern static
  • no guarantee it was initialized by the extern language
  • may overlap addresses of other extern statics
  • nonetheless expected to be operationally immutable
  • safety conditions are because we expect to be able to optimize reads of the global

[digama0]

As long as the computation of the address of the static is possible without UB (and I don't see how it wouldn't be, since it's a runtime constant to be determined by the linker), it should be safe to use &raw [const|mut] {EXTERN_}STATIC. There is possibly some edge case where the definition of the extern static itself makes some promises to the linker, similar to the issue with extern functions that motivated the unsafe extern { ... } RFC, but even in this case it seems the unsafety is limited to the declaration site, not the use site of the static.

[RalfJung]

Okay, yeah that makes sense.

@workingjubilee mentioned weak linkage, which I know very little about -- is there something we need to be concerned about there?

Also Cc @bjorn3 for more linker know-how.

[workingjubilee]

A weakly linked symbol that has not been resolved has the null address.

A weakly linked symbol that has resolved has a non-null address.

My understanding is that this can change at runtime considering e.g. dlopening things, so such a static may "move" in the address space. As I understand it, it is still safe to take the address, but it is a detail that seems worth mentioning.

[RalfJung]

Is it the place itself that changes or its contents? The weak linkage statics I have seen in std have ptr type and their contents are whatever the symbol resolves to. So if that can change at runtime that would mean they have to be static mut, but otherwise nothing is special?

[bjorn3]

For all static with the #[linkage] attribute we codegen two global variables. One which is just a declaration, takes the linkage name from the static and has the specified linkage. And one which has a definition of being a pointer to the other global variable and a mangled name. All uses of the static are redirected to this second global variable.

So if that can change at runtime that would mean they have to be static mut, but otherwise nothing is special?

I recently made a PR to deny static mut with #[linkage]. The mut would only apply to the second global variable. The first one is always implicitly mut if the target is.

[workingjubilee]

@bjorn3 Ah, that is interesting! Thank you for explaining, I didn't realize we were that tricksy about it. ( Part of why I was wondering exactly what was going on there... )

[RalfJung]

@bjorn3 thanks for explaining!
However, if what @workingjubilee said about dlopen above is correct, then the address of a weak-linkage static can change. So when I have

            extern "C" {
                #[linkage = "extern_weak"]
                static WEAK: Option<unsafe extern "C" fn($($t),*) -> $ret>;
            }

then that would mean that the value of WEAK changes from None to Some when a library with this symbol gets loaded -- or did I misunderstand?

[bjorn3]

Symbol resolution only happens once while loading the current DSO. If a symbol could be resolved by a future dlopen, this will only affect symbol resolution from that point onwards. The dynamic linker will not update existing weak references that have already been resolved to NULL.