Type validation mistreats layout errors #71353
Assignees
Labels
A-const-eval
Area: Constant evaluation (MIR interpretation)
C-cleanup
Category: PRs that clean code up or issues documenting cleanup.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
jonas-schievink
added
A-const-eval
|
Cc @rust-lang/wg-const-eval This is definitely not intended, that example you gave is entirely safe code and should compile just fine. The odd thing is, I am indeed responsible for the error but that was back in #66147 which has long arrived on stable -- so the error itself cannot be the source of the regression. Would be good to get the regression narrowed down a bit more. I also don't understand why The error message is probably (hopefully!) just wrong, I doubt the pointer is actually uninitialized. We just show that error when anything goes wrong in this line: rust/src/librustc_mir/interpret/validity.rs Lines 487 to 491 in 4ca5fd2 Likely ref_to_mplace really doesn't like that the pointer is NULL. The thing is, this code works fine, and that just makes no sense:
fn main() {
const NULL: *mut i32 = std::ptr::null_mut();
println!("{:?}", NULL);
} |
jonas-schievink
added
the
E-needs-bisection
|
@rustbot ping bisect |
rustbot
added
the
ICEBreaker-Cleanup-Crew
|
Hey Cleanup Crew ICE-breakers! This bug has been identified as a good cc @AminArria @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @jakevossen5 @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke |
|
It's not an ICE, but if ICE-breakers also help with other bisects then yes this would be awesome. :) |
|
If it helps pinpoint when the error was introduced, my |
|
Regression in nightly-2020-04-17. Looking currently for regression commit between 2020-04-17 and 2020-04-16. searched nightlies: from nightly-2020-04-07 to nightly-2020-04-19 Full Log: |
|
My guess would've been #70566 |
|
@senden9 awesome, thanks a lot. I am very sure it's not #71149, that would ICE complaining about bad types in a function call. I doubt it's #71141, that doesn't sound likely to me. My guess is that it's #70566 (Cc @jumbatm), which affects const-prop and what code const-eval gets run on. But still that doesn't explain why it would suddenly stop liking NULL pointers. In fact it's not just NULL, this also fails: impl<T> Nullable for *mut T {
const NULL: Self = 4usize as *mut _;
#[inline]
fn is_null(&self) -> bool {
*self == Self::NULL
}
}Hm... actually... that const-prop change will mean we run const-prop on some generic code. Maybe it tries to evaluate that |
|
Bingo, the backtrace where errors is created |
|
So it is probably this call that is failing: rust/src/librustc_mir/transform/const_prop.rs Line 534 in 33500a2 |
|
But then it gets weird. We end up here: rust/src/librustc_mir/interpret/operand.rs Line 489 in 33500a2 and then here And then somehow we end up evaluating a const (?!?) even though we just wanted to subst on a |
wesleywiser
added
P-high
|
I can see it evaluating |
|
We already have a very similar test for Nullable. It's curious that it didn't catch this. rust/src/test/ui/consts/const-eval/ice-generic-assoc-const.rs Lines 1 to 18 in 20fc02f Ah, changing the test case to |
|
Ah, good catch! Probably it was a mistake to not add The test was added in #51852. |
jumbatm
added a commit
to jumbatm/rust
that referenced
this issue
Apr 21, 2020
|
@rustbot claim |
|
Namely, the proposed solution is to:
|
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Apr 25, 2020
…=Mark-Simulacrum [beta] fix failing const validation This is the **beta branch fix** for rust-lang#71353, by reverting rust-lang#70566. r? @oli-obk Not sure if there is any extra process for the beta part. This is not a backport; we intend to "properly" fix this on master but for beta a revert is faster and less risky.
|
The beta PR landed, master PR by @pnkfelix is on the way. |
|
This bug is now fixed, so I will remove all the critical label. But let's keep this open to track the issue of validation swallowing |
RalfJung
added
C-cleanup
RalfJung
changed the title
rustbot
added
the
I-prioritize
|
Assigning |
spastorino
added
P-medium
|
@Mark-Simulacrum As this problem raised in the Crater run, should #71663 be nominated to be backported to beta? |
|
Oh right, I re-read #71353 (comment) |
|
Indeed, the beta fix was to revert the original incorrect PR. This PR that just landed now just fixed #69021, which doesn't need backporting. However, I am going to re-open this issue because validation still swallows layout errors in many cases -- just this one particular case got fixed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The original problem got fixed by avoiding the broken code paths; this issue now tracks fixing those code paths.
Original issue
In the
ffi_helperscrate we have aNullabletrait which gives you a generic way to do null pointer checks.A user recently reported that the crate no longer compiles on
nightly(Michael-F-Bryan/ffi_helpers#2) because type validation detects thatstd::ptr::null()andstd::ptr::null_mut()create uninitialized raw pointers.I can reproduce this on the playground with the latest nightly,
1.44.0-nightly (2020-04-19 dbf8b6bf116c7bece298).Is the use shown in that playground example (
*self == Self::NULLon a*mut T) actually UB?Also, I noticed that calling the
is_null()method defined on a raw pointer with<*const T>::is_null(*self)doesn't trigger this error, implying that the problem isn't with declaring a constant that contains a null pointer (const NULL: Self = std::ptr::null_mut()), but the fact that we're using it for a comparison. Was this intended, or is it just an oversight in the error detection code?Full example with output
CC: @RalfJung because it looks like they were the last person to touch that error message (9ee4d1a).
This issue has been assigned to @jumbatm via this comment.
The text was updated successfully, but these errors were encountered: