Repository navigation
Regression in consteval: error[E0080]: could not evaluate static initializer (unable to turn pointer into raw bytes) #99923
Description
Activity
You should've never needed to transmute the pointer side to
usize, this works fine:pub const unsafe fn from_bytes_unchecked<T>(bytes: &[u8]) -> &[T] { let (data, mut metadata): (*const u8, usize) = core::mem::transmute(bytes); metadata /= core::mem::size_of::<T>(); core::mem::transmute((data, metadata)) }
Yeah, I managed to make it work with this instead:
// &[u8] and &[T::ULE] are the same slice with different length metadata. let data = bytes.as_ptr(); let mut metadata = bytes.len(); metadata /= core::mem::size_of::<T::ULE>(); Self::Borrowed(core::mem::transmute((data, metadata)))- That PR improved our ability to detect UB in const code. Transmuting a ptr (that was not created via int2ptr cast) to a usize in const is UB. So I am not sure this is a regression. Code with UB is not subject to stability guarantees.
I understand, but I will point out that currently what is and isn't UB is in flux (and almost completely undocumented), and also a lot of the APIs to do this safely are not yet available (const
slice::from_raw_partsjust stabilized1).I don't think "this is UB" is sufficient to ignore breaking changes when there is no documentation on what Rust considers UB, and Rust is still pinning down what it considers UB.
unsafecode writers are currently in a tricky position; where everything is in flux; and the Right Way to do things has not necessarily been figured out yet, so we basically are forced to try something and hope it sticks. I don't think it's particularly fair to putunsafewriters in this position2.This is not to say that we shouldn't make changes like this, but I think it would be good for things like this to go through a period of warning with better diagnostics, where possible. I had to bisect the compiler to understand what went wrong here, which is fine in my case since I'm used to it, but I don't really think it's acceptable for us to decide that this is an okay user experience for others.
(To be clear, my reaction here is to the statement that this isn't a regression, not to the breakage happening in the first place. Breakages happen, it's hard to predict it all, but I would prefer breakages like this to be handled more gentler once discovered)
Footnotes
-
Yes, I know that the broken bit of code was the piece of code before that with the transmute; however the lack of
slice::from_raw_partsis what ended up in me writing the code this way, since it led to a clearer symmetry in the code. ↩ -
Especially with the pointer provenance experiment stuff seems to be changing rapidly and that does not give us enough time to catch up to the exciting new forms of UB that are getting introduced. As far as I can tell, this particular case is an instance of that; I was somewhat aware that
usizecasts were going to become UB but haven't considered it a problem yet because the "safe" APIs for doing such things haven't been made available in Rust yet, and I figured Rust wouldn't cause problems on UB it has introduced that cannot be worked around yet. I would very much like to continue operating under that assumption, anything else basically means we cannot write unsafe code, period, until the UCG has figured stuff out. ↩
Reacted by Jubilee, Diana, Aaron Hill, Esteban Kuber, Suraj, jcrevier, Anders Musikka and Mads Marquart-
At minimum we need to start emitting a lot better diagnostics about how
const+unsafeis extra-"fun" territory. If something happens inside aconst unsafecontext that is busted in that context and we detect it, but it would work in a non-constunsafecontext, we should start signaling appropriately. If it would be UB in both contexts, all the more reason to emit a very clear error message! As is, This is a... cryptic... notice which lacks much context. People need extra help with the transmuting rules inconst unsafebecause it does not necessarily allow all the transmutes that non-constunsafedoes.And if people start hearing about how
const unsafeis a Bad Time, then they at least can know when to sigh in despair about how they have encountered the Bad Time. Much like occasionally someone decides to write a function with five different lifetimes and then questions theirfn life_choices<'up, 'to, 'the, 'current, 'moment'>(...)when they see the resulting error message featuring Esteban in crab cosplay trying to explain how busted their code is. Then they either soldier on with fixing their code or take a different tack. Ideally we should be there with a useful suggestion at that point.We do have more latitude to make something stop compiling if it's UB, of course. Rust programmers will probably be unsurprised if they hear "well, that
unsafeblock was actually never defined behavior, whoops" and get shuffled around to writing different code. But we need to have some directionality in mind when we actively break some extant code.Reacted by Manish Goregaokar, jade, Diana, Esteban Kuber, Ted Mielczarek, Crystal Durham, Mikayla Maki, León Orell Valerian Liehr, Acshi Haggenmiller, Hocuri and 3 moreKeep in mind the provenance changes pretty much only affect runtime execution.
CTFE has its own much stricter rules that AFAIK are unaffected by provenance variants, as it's by necessity/design at least as restrictive as the most restrictive provenance model for runtime, with symbolic pointers into allocations that behave more like disjoint address spaces.The irony here is that where before miri was (wrongly?) doing nothing (resulting in an
usizevariable holding a non-integer value), it now has to do something in order to make the ptr2inttransmutenot UB, and that is enough to unlock its ability to detect UB it was previously not seeing (because it used to just let a value pass through unmodified).
Now, why did CTFE miri not complain about ptr2int transmutes before? Is it only UB now?
There's an easy way to be sure - get it to run its validity checks godbolt:union Transmute<T: Copy, U: Copy> {from:T,to:U} pub static PTR_TO_USIZE: usize = unsafe { Transmute{from:&0}.to };
(using an
unionto allow testing on versions old enough to not haveconst fntransmute)Since 1.33, and still the case now, in 1.62 (wow, half the lifetime of Rust stable), it errors with:
error[E0080]: it is undefined behavior to use this value --> <source>:3:1 | 3 | pub static PTR_TO_USIZE: usize = unsafe { Transmute{from:&0}.to }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain (non-pointer) bytes(the error has gotten more detailed since, but this is what it used to look like)
Okay, but why does the "more local" one not error? Is there somehow "less UB" deeper in CTFE? Nah:
rust/compiler/rustc_const_eval/src/interpret/machine.rs
Lines 452 to 455 in bd84c73
It's just... turned off. It has been turned off ever since the functionality was added in:
It's only enabled by
miri(the tool):But for
rustcwe don't even have a-Zflag to turn it on so I can't even check what it would've done for old versions.Presumably it would be very expensive for large arrays, but it seems like an increasing hazard (as more people are building all sorts of abstractions using
const fn) to just let it sit disabled.
Perf-wise we might just need some fast paths for allocations that contain zero relocations and I guess arrays of types with few validity invariants? It would be interesting to see how far we could get without making it easy for unsoundness to creep in.@RalfJung did we ever do perf/crater runs with
enforce_validityreturningtruefor CTFE miri?
Even if we can't land it right away, we should probably be tracking its viability?I do agree with the concern that our UB rules can be hard to follow. However, note that the UB involved here is much more clearly documented than most other kinds of UB that we have. Quoting from the
transmutedocs:Transmuting pointers to integers in a const context is undefined behavior. Any attempt to use the resulting value for integer operations will abort const-evaluation.
So, if this is not enough, then I don't know what is. We also have an RFC saying that the compiler is not required to detect all UB. (I was arguing for mandating some amount UB detection, but the consensus was we shouldn't do that.) Put together, I am not quite sure what we could have done differently here. It is very easy for interpreter changes to "accidentally" improve or regress UB detection, and we need the ability to continue evolving that interpreter without going through a month-or-year-long deprecation process each time.
Do you have concrete suggestions for something that should have been done differently when landing #97684? Or is your concern that we need more docs elsewhere (where, and which kinds of docs?), and perhaps some way for crate authors to opt-in to stricter UB checks for CTFE? The irony is that for runtime UB we have Miri, but there is no way to have compile-time UB checks that are as strict as what Miri is doing...
Transmutation is a hornet's nest, and it is super hard to even talk about it precisely without first giving an entire lecture on Rust operational semantics (the details of which do not have consensus yet). This is true for runtime transmute and even more true for CTFE transmute.
Keep in mind the provenance changes pretty much only affect runtime execution.
CTFE has its own much stricter rules that AFAIK are unaffected by provenance variants, as it's by necessity/design at least as restrictive as the most restrictive provenance model for runtime, with symbolic pointers into allocations that behave more like disjoint address spaces.Yeah, this is only tangentially related to the strict provenance discussion. (The relationship is that as an outcome of that discussion I was able to make the handling of pointers in Miri a lot more coherent, and one side-effect of that refactor is improved UB detection even for CTFE.) The rules for what you are allowed to do with pointers during CTFE have never changed, we are now just able to detect rule violations a bit better.
did we ever do perf/crater runs with enforce_validity returning true for CTFE miri?
There recently was an experiment to selectively enable it for some types/situations, and even that was too expensive: #95377.
Reacted by Acshi Haggenmiller and Mads MarquartReacted by Mads MarquartI do agree with the concern that our UB rules can be hard to follow. However, note that the UB involved here is much more clearly documented than most other kinds of UB that we have. Quoting from the
transmutedocs:So, if this is not enough, then I don't know what is.
Yeah, I'm aware (since opening this issue and digging further) that this is documented. Thing is, it's on a function that has existed for ages; users aren't going to recheck that documentation each time they use it once they're familiar with it.
If Rust had a complete story for UB it would be totally worth it to go and read through the UCG docs, the reference, the nomicon, and the stdlib docs to learn what is and isn't UB for once and for all. While stuff is in flux and largely undocumented it is not practical to do such a literature survey each time, for the slim chance you actually find an answer for your specific kind of UB.
I don't know if it's possible for it to ever be "enough" until the work of the UCG group is finished. I absolutely would appreciate more docs like this, redundantly placed on the stdlib and UCG docs, but the problem here is a more general one. And it's a completely understandable problem — of course UCG can't wave a magic wand and be done in a week — I'd just like more understanding extended towards lib authors dealing with it.
I think of this Douglas Adams quote when thinking about how we document UB today:
“But the plans were on display…”
“On display? I eventually had to go down to the cellar to find them.”
“That’s the display department.”
“With a flashlight.”
“Ah, well, the lights had probably gone.”
“So had the stairs.”
“But look, you found the notice, didn’t you?”
“Yes,” said Arthur, “yes I did. It was on display in the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying ‘Beware of the Leopard.”In a situation where most UB is not documented, users do not expect UB to be documented, and there are four places to go look for UB documentation (hell, the transmute docs link to two of them), a documentation change on a function that has existed for ages and hasn't really had changes to UB-ness is not actually that prominent IMO.
We also have an RFC saying that the compiler is not required to detect all UB.
I'm aware. My point is based on what we ought to do, not what we can do.
Do you have concrete suggestions for something that should have been done differently when landing #97684? Or is your concern that we need more docs elsewhere (where, and which kinds of docs?), and perhaps some way for crate authors to opt-in to stricter UB checks for CTFE? The irony is that for runtime UB we have Miri, but there is no way to have compile-time UB checks that are as strict as what Miri is doing...
As I said earlier my pushback is more about the reaction here rather than #97684 itself, since mistakes happen, but here you go:
If it had been known at the time of #97684 that it would break code, I would like to have seen some discussion into whether or not a future incompat warning could have been introduced, with better diagnostics. I get that this is not always possible, but it should have been discussed.
Furthermore as @workingjubilee said, given that const is spicy when it comes to UB we should probably be adding better diagnostics in general, but especially around any kind of error.
More docs are always good. However, while UCG is still figuring stuff out, I personally have almost zero expectation of seeing more docs on everything y'all figure out. I'm happy to let y'all figure stuff out first and doc it later, my pushback is against UB being exploited or being errory till we're in that final stage.
It would also be nice if there were generally more communication on "UB in const is spicy". Stuff helping unsafe code writers learn that UB in const is different and how to reason about UB in const would be great (perhaps a blog post?).
Which I don't necessarily expect, I know y'all have a lot on your plate I just think it would be nice to have, and I would kinda consider that having been adequately communicated as a prerequisite to const code breaking.
But basically, I would like to see some thought given to the user experience of unsafe writers around stuff like this. I'm more plugged in than most to this stuff work; I've been paying some attention to the UCG discussions and I've had chats with various friends who are involved or knowledgeable. If this bit me, this kind of thing will quite likely happen again with more unsuspecting users, and I'd rather they had ample warning and a good user experience for things like this.
To give an idea of why I'm pushing back here; the status quo of writing unsafe Rust is to try and avoid UB as much as possible based on what you know is definitely UB, what you have seen documented, running
miria bunch, and for some people paying attention to what UCG is discussing. The flipside of this is that all of us are basically assuming that we have a fair amount of UB somewhere since it's just not possible to know for sure, and that we will have ample warning to fix that.Let me repeat that: we are all assuming we have UB, and that assumption is probably correct. Which means when you say "UB fixes are not regressions" with no caveats, in our case, just means "you don't get stability at all". It doesn't mean "be careful and you can avoid breakages". It doesn't mean "be careful there may be breakages around weird niche stuff". It just means "no stability for you".
Of course we cannot expect long term stability on this, rustc should not be prevented from eventually being stricter, none of us expect that it will. However the "ample warning" is something we can expect. It's the only way to make a situation like this work at all.
In general we have been operating under the assumption that UB , especially kinds of UB not found in C, will start breaking once UCG has actually reached a point where there's documentation that covers most UB that we can basically read a couple times, internalize, and apply. We have not been expecting these things to break until then; we have been expecting that one day we will sit down with the Complete Docs and go through our unsafe code and fix it alll, and later the old code will start breaking. And until then, we pay some attention and if Exciting New kinds of UB are announced, we go fix those; we can expect some time pass before that UB starts breaking.
Again, as far as I can tell, this is the only practical way to write unsafe code right now. So a position that ample warning is not necessary because it's a soundness change and thus allowed to be breaking is very scary, since I have a hard time reading that as anything but "if you write unsafe code, all bets are off on the stability of your software and it might break tomorrow". That's a bit of a rug pull for a language where the fact that you can write
unsafecode is a major reason it works for many kinds of software at all.While the assumption of ample warning is the only practical way to make this situation work, a lot of this assumption also comes from the fact that for Exciting New kinds of Rust UB, there's basically no chance of LLVM suddenly exploiting it, and it's extra work for Rust folks to write optimizations that depend on that UB, so it won't happen until after there's a full story.
constis an interesting wrinkle to that, one which I hadn't thought about before but @workingjubilee and @eddyb got me to connect the dots: UB inconstisn't an LLVM thing, it's amirithing, and whileconsttypically has no need to perform optimizations, the strong soundness requirements on something that plugs into the type system, and just general design concerns ofmiri, mean that it is far more likely that Exciting New kinds of Rust UB will trip upconsteven though the regular compiler will take years to exploit them.I do think it's worth spending some time communicating the way UB in const is different (and worth making the diagnostics for it better).
Pairing that with an understanding that UB-breakages are more likely in const (since it's a compile time error) than in actual compiled code could work. I would prefer ample warning for const code as well; and given that it's compile time the "warning" can actually come in the form of a future incompat warning, but "if you're writing unsafe in const, expect breakages" is actually workable (unlike "if you're writing unsafe, expect breakages", which is extremely not workable). I'd expect it to be clearly communicated before the breakages start, but it's a workable position to have.
Ultimately, a compile time error in const with no warning is far less scary than subtle "miscompiles" suddenly happening due to UB, so I'm kinda glad we're having this discussion here instead of later 😄
Reacted by Thom Chiovoloni, nora, Diana, Ralf Jung, Jubilee, Aaron Hill, Esteban Kuber, Crystal Durham, Nicholas, Dmitry Ivanov and 1 moreIt is very easy for interpreter changes to "accidentally" improve or regress UB detection, and we need the ability to continue evolving that interpreter without going through a month-or-year-long deprecation process each time.
Oh, also, like I said before, I totally understand stuff like this slipping; there are many times when it's too subtle to realize and this case might be one of them. But, in general, future incompat cycles aren't that long and it would be nice to at least consider them on a case by case basis.
There recently was an experiment to selectively enable it for some types/situations, and even that was too expensive: #95377.
The largest regression in the entire rustc-perf benchmark suite was 1.94% on
ctfe-stress, 0.8% on any real crates. The only way to invent a large regression was with a completely contrived program. Perhaps it is worth asking, @Manishearth would you have accepted a 0.8% compile time regression to know about this sooner, or to have more thorough UB detection inconstin general?@saethlin Probably, yeah, I'd even be happy to run a build under some rustflag to opt in if I knew about it (but stuff here changes so often it's hard for me to know about these things)
However in this case it's unclear to me why there would have to be a perf regression for this. We currently already detect this UB, what I would have preferred is if we threw a future incompat warning for a stable release cycle and then moved on. It's understandable if that's not compatible with the architecture of miri, but so far I haven't seen any discussion of this.
what I would have preferred is if we threw a future incompat warning for a stable release cycle and then moved on.
What @saethlin did was try out how expensive adding any checking would be without changing the miri logic. What Ralf did was a cleanup that accidentally opted into some of that checking by just not having a code path for the UB case anymore.
So yes, for this specific case we could have written a future incompat lint had we known about it. Had we known about it we'd have cratered it and discussed it, too. The change just seemed innocent to me as it didn't actually make things simpler, but just avoided some relaxed logic that I though only miri ever used. Turns out CTFE also went through that path in the presence of UB.
I guess it all boils down to the fact that we don't even have full branch coverage via out test suite.
I guess the only answer I have here is that i'll be more careful in future reviews. Not sure what else to do with the current capacity we have for CTFE work.
51 remaining items
From the crater run, regression on
transmute_copywas found, which I believe is the same kind of issue as this.Also, the crater run didn't bring up any other cases of this in particular, and
icu4xdoes not have dependents, so I would feel comfortable not backing out of this change (but announcing it very loudly in the blog just in case).Reacted by Manish GoregaokarIt seemed to me to be the same category of issue, where we break backwards compatibility in const unsafe code that currently compiles. Whatever decision is made about how to deal with such cases applies for that case. Am I wrong in my reading?
#100545 is more about runtime than const UB I think? Certainly it also applies at runtime, which this one here does not.
And anyway the question in this issue currently isn't whether we'd back out #97684, we already pretty much decided against that. The question is what else we can do to help people. And we have a plan for that.
Reacted by Esteban Kuber- added a commit that references this issue
on Sep 4, 2022 - added a commit that references this issue
on Jun 3, 2023
Unfortunately I'm not able to come up with a minimal working example for this, but running
cargo check -p icu_datetime --exampleson https://github.com/unicode-org/icu4x (commit unicode-org/icu4x@021a92e should be fine)ends up throwing a ton of errors that look like this:
(full errors; they're long since the "baked" stuff is a bunch of internationalization data that has been "serialized" to Rust code using https://docs.rs/databake)
We have a lot of consteval using unsafe code, which is what's tripping up here. The code that's breaking is code that's currently manually constructing a DST of
&[T::ULE]from&[u8], and in aconstenvironment that requires some pointer manipulation:Bisecting the regression points to #97684 (cc @oli-obk, @RalfJung), which seems to be about integer provenance. The code here isn't ideal; there are slightly better ways to do this but nothing is
const, so this is what we've got.Either way, this is a regression.
Bisection:
searched nightlies: from nightly-2022-06-01 to nightly-2022-06-30
regressed nightly: nightly-2022-06-07
searched commit range: fee3a45...50b0025
regressed commit: 9d20fd1
bisected with cargo-bisect-rustc v0.6.3
Host triple: x86_64-unknown-linux-gnu
Reproduce with: