Stop generating an intermediate deref when taking references to statics #76982
Conversation
rust-highfive
added
the
S-waiting-on-review
| ty::RawPtr(..) => "raw ", | ||
| _ => bug!("Rvalue::ThreadLocalRef types can only be pointer or reference"), | ||
| }; | ||
| write!(fmt, "&{}/*tls*/ {}{}", raw, muta, tcx.def_path_str(did)) |
There was a problem hiding this comment.
Why is TLS between "raw" and "mut"?
| let muta = ty.builtin_deref(true).unwrap().mutbl.prefix_str(); | ||
| let raw = match ty.kind() { | ||
| ty::Ref(..) => "", | ||
| ty::RawPtr(..) => "raw ", |
There was a problem hiding this comment.
If you are matching anyway, the same match could also extract the mutability... do you think taht would make the code cleaner?
| let res = cx.typeck_results().qpath_res(qpath, arg.hir_id); | ||
| if let Res::Def(DefKind::Static, id) = res { | ||
| let mut tm = cx.tcx.static_ptr_ty(id).builtin_deref(true).unwrap(); | ||
| tm.mutbl = mutability; |
There was a problem hiding this comment.
This looks like shoehorning the existing static_ptr_ty helper into something else. Is that helper still used elsewhere, given the new shape of things?
There was a problem hiding this comment.
yes, it's still used further up. I didn't want to repeat its code, but I could split out the part that obtains the static's type into a separate function
There was a problem hiding this comment.
In particular, that function contains some subtle logic if I recall correctly (for using raw ptrs vs references), and now we overlay our own logic on top of this here... given how critical this code is, I'd prefer that to be cleaner, even if that makes the code longer.
Also it would be good to have some comments explaining why all of this here crucially has to be exactly the way it is.
There was a problem hiding this comment.
yes, it's still used further up.
Ah, I just saw that. But that further up should now not be reachable any more for refs-to-static, right, just for when it is used as a value? If so, please add a comment explaining that. Also, doesn't that mean we can always use a shared reference, as we always just read? Well and I guess a raw pointer for unsafe-to-read things to make sure we do not sidestep the unsafety checker.
I feel like all the subtle logic that determines the type of the reference we use for reads and for address-of should be in one place.
| fn main() { | ||
| bar(unsafe { core::ptr::raw_const!(FOO) }); | ||
| bar(unsafe { &raw const FOO }); | ||
| } |
There was a problem hiding this comment.
Could you also somewhere (hopefully there is a good existing test file?) add a test for the thread-local case? After all this PR also changes thread-local static ptrs quite a bit, would be good to have that covered as well.
| @@ -0,0 +1,21 @@ | |||
| #![feature(never_type)] | |||
| #![feature(raw_ref_macros)] | |||
| #![feature(raw_ref_op)] | |||
There was a problem hiding this comment.
Maybe add deny(unreachable_code) (with a comment) to ensure even more that this test fails without the PR? The MIR output is auto-blessable so there is some risk that it'll just get blessed in the future if this ever regresses...
| def_id: id, | ||
| } | ||
| }; | ||
| let kind = convert_static_ref(cx, id, ty); |
There was a problem hiding this comment.
"kind" is a really confusing variable name here, this is an expression after all.
|
So... none of these changes actually affect the test to make it pass... The root problem is much earlier. Typeck decides that these expressions are unreachable because it sees rust/compiler/rustc_typeck/src/check/expr.rs Line 159 in 81e0270 rust/compiler/rustc_typeck/src/check/expr.rs Lines 204 to 207 in 81e0270 |
|
I don't really want to keep slapping more hacks on top of this function... Can we just add a future incompat error for extern statics that are of uninhabited type? I don't see any way to use them safely and they seem fairly useless if the only thing you can soundly do with them is to take a raw pointer to them. |
Somehow my understanding was that this is a regression introduced by the move away from statics-as-places towards statics-as-ptrs... but that does not even seem to be the case, these uninhabited extern statics never worked, did they? I agree it is not worth adding a typeck hack just to silence this warning.
Sounds fine to me. |
No
It's not just about the warning. Typeck causes this to become UB (the |
Oh, dang. (moving discussion back to the issue) |
r? @RalfJung
fixes #74840