-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add needs_substs check to check_binary_op
.
#71386
Closed
jumbatm
wants to merge
5
commits into
rust-lang:master
from
jumbatm:fix-nullable-type-validation-error
Closed
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
32ff9a0
Also catch #71353
jumbatm c61c17c
Skip check_binary_op if rhs needs substs.
jumbatm cf3b744
Also add substs check before evaluating for lhs.
jumbatm 2d25b2b
Also add substs check to check_unary_op.
jumbatm 2e6fab9
Explain check_{unary,binary}_op needs-substs behaviour.
jumbatm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems entirely ad-hoc, and also not necessarily complete. What about
left
? What about the other functions that your previous PR exposed to subst-carrying data which were previously guarded by thatif
you moved around?And what is even happening that we see uninitialized data, and that no assertion anywhere fires to detect that we run the engine on code it cannot run? This is the most concerning part to me. Cc @oli-obk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, you're right. I've left left out.
The only reason I felt comfortable with doing this is
check_{unary, binary}_op
is only called from theconst_prop
method, so onlycheck_{unary, binary}_op
(and their callees) are newly exposed to subst-carrying data (speaking of which, I seecheck_unary_op
also needs to be guarded because it evals its operand).And what is even happening that we see uninitialized data, and that no assertion anywhere fires to detect that we run the engine on code it cannot run?
Per #71353 (comment), isn't it just the error message raised during validation that's suspicious? I'm not sure that there's really uninitialised data here. I'll chuck a debugger on and see where the Err is being raised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first theory. But in #71353 (comment) it doesn't look like that any more.
Also, raw ptr consts which are NULL otherwise work fine. So,
ref_to_mplace
probably actually accepts NULL ptrs like it should.If you guard
left
andcheck_unary_op
, you fully un-did your previous PR and the bitshift warning will regress again. ;)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see where you are going... we can do the check for
left
after the bitshift check. Then the warning will still work fine and we fix the bug. Good idea.However, we still should figure out what is happening that we see uninitialized data coming back here. That is not a good failure mode. Instead we should have gotten an ICE somewhere saying "there are still substs here, this const cannot be evaluated".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm entirely confused by this issue. I don't like the fix in this PR, it doesn't address the issue, it just makes sure we don't run into it.
It should be perfectly possible to evaluate
std::ptr::null()
for unknown (butSized
!)T
. I don't see why it's failing validation of all things. I'd understand it if we'd get aTooGeneric
error due to layout computation bailing out or sth. I think this is a bug in validation.After checking what causes "uninitialized raw ptr", I believe the fault is at
rust/src/librustc_mir/interpret/place.rs
Line 307 in c6b55ee
well, not really, this is where I'd expect the
TooGeneric
error to come from, buttry_validation!
turns every error into a validation error:rust/src/librustc_mir/interpret/validity.rs
Line 54 in 2dc5b60
We should probably not do that for any errors of the
InvalidProgramError
category: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/interpret/enum.InterpError.html#variant.InvalidProgram (and we can probably ICE for a few other categories)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nono, that would be very wrong. We often invoke const eval on too generic code with
Reveal::UserFacing
, and we need to keep doing that. But forReveal::All
we can indeed report an errorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we should just forward it for now, then.
So, action items:
try_validation!
with a pattern of errors to catch, the rest is forwarded. (Figuring out all the right patterns will take a few runs of the test suite here.)InvalidProgram
errors out of validation instead of ICEing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jumbatm do you want to work on this alternative solution? (It's okay if you don't, in that case we'll look for someone else to do it. :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to keep going and work on the alternative solution. It definitely seems like the more robust way to fix the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks. :)
Probably best to make that a new PR, as most comments here will not apply any more.