Reintroduce Undef and properly check constant value sizes
#52712
Conversation
This file contains hidden or 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
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
oli-obk
force-pushed
the
const_eval_cleanups
branch
from
d830891 to
bb94129
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
RalfJung
reviewed
Jul 27, 2018
| } | ||
| } | ||
|
|
||
| pub fn to_value_with_len<C: HasDataLayout>(self, len: u64, cx: C) -> Value { | ||
| ScalarMaybeUndef::Scalar(self).to_value_with_len(len, cx) | ||
| } |
There was a problem hiding this comment.
Why do we have this, but not the vtable version?
There was a problem hiding this comment.
the vtable one wasn't used anywhere
There was a problem hiding this comment.
I think in terms of API consistency it still makes sense to have them both.
RalfJung
reviewed
Jul 27, 2018
src/librustc/mir/interpret/value.rs
Outdated
| ScalarMaybeUndef::Scalar(scalar) => { | ||
| scalar.ptr_signed_offset(i, cx).map(ScalarMaybeUndef::Scalar) | ||
| }, | ||
| ScalarMaybeUndef::Undef => Ok(ScalarMaybeUndef::Undef) |
There was a problem hiding this comment.
Uh, no error here? Don't we otherwise insta-error on any operation in undef?
RalfJung
reviewed
Jul 27, 2018
src/librustc/mir/interpret/value.rs
Outdated
| } | ||
|
|
||
| impl ScalarMaybeUndef { | ||
| pub fn read(self) -> EvalResult<'static, Scalar> { |
There was a problem hiding this comment.
read sounds a bit like it might access memory. I don't have a great idea for a better name either, though.
There was a problem hiding this comment.
it's "accessing" the "memory" inside the Scalar. I changed it to unwrap_or_err
RalfJung
reviewed
Jul 27, 2018
| _ => bug!("invalid `{}` argument: {:?}", name, bits), | ||
| }; | ||
| let extra = 128 - defined as u128; | ||
| let extra = 128 - size.bits() as u128; | ||
| let bits_out = match name { | ||
| "ctpop" => bits.count_ones() as u128, | ||
| "ctlz" => bits.leading_zeros() as u128 - extra, | ||
| "cttz" => (bits << extra).trailing_zeros() as u128 - extra, | ||
| "bswap" => (bits << extra).swap_bytes(), |
There was a problem hiding this comment.
Hm, so we have these duplicated between miri and CTFE? :/
There was a problem hiding this comment.
miri should just forward to ctfe for these
RalfJung
reviewed
Jul 27, 2018
| @@ -1233,7 +1248,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M | |||
| } | |||
| } | |||
|
|
|||
| pub fn read_value(&self, ptr: Scalar, align: Align, ty: Ty<'tcx>) -> EvalResult<'tcx, Value> { | |||
| pub fn read_value(&mut self, ptr: Scalar, align: Align, ty: Ty<'tcx>) -> EvalResult<'tcx, Value> { | |||
There was a problem hiding this comment.
because the sanity check branch hadn't been merged yet and discriminant reading still allocated here
There was a problem hiding this comment.
So this can go back to just & now?
RalfJung
reviewed
Jul 27, 2018
| ScalarMaybeUndef::Undef, | ||
| ScalarMaybeUndef::Undef, | ||
| ), | ||
| _ => Value::ByRef(self.alloc_ptr(ty)?.into(), layout.align), |
There was a problem hiding this comment.
I'll try make it lazy again
RalfJung
reviewed
Jul 27, 2018
| self.mark_definedness(ptr, size, false)?; | ||
| Scalar::Bits { size: 0, .. } => { | ||
| // nothing to do for ZSTs | ||
| assert_eq!(type_size.bytes(), 0); | ||
| return Ok(()); |
There was a problem hiding this comment.
Early return is dangerous. Did we check everything we have to check, i.e., pointer alignment?
RalfJung
reviewed
Jul 27, 2018
| // compile-pass | ||
|
|
||
| const PARSE_BOOL: Option<&'static str> = None; | ||
| static FOO: (Option<&str>, u32) = (PARSE_BOOL, 42); |
There was a problem hiding this comment.
Where is there an undef here? The enum gets layout optimized so writing None initializes the entire thing, does it not?
There was a problem hiding this comment.
I had a regression when building stage 2 that didn't occur in the test suite, so I added a test. The issue was that some function didn't read the discriminant and figure out the variant layout before reading fields, so it was reading fields by enum indices (where 0 is the discriminant).
There was a problem hiding this comment.
I agree with the test, I am just confused by the filename, which says that undef is relevant here.
You mentioned you found some real bugs. Could they be reached by actual code, i.e., should we have testcases (probably in miri to avoid CTFE restrictions)? Are these places still subtle in the code and could warrant extra comments? The patch looks good to me (except for the comments I added), but I also did not notice any actual behavioral changes. |
I have not been able to do that.
Not anymore. We'll run into assertions now and generally the requirement for an
That's the idea. Silent unreachable conversions become assertions. |
oli-obk
force-pushed
the
const_eval_cleanups
branch
from
bb94129 to
2c836a7
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
This is ready @RalfJung |
RalfJung
reviewed
Aug 3, 2018
src/librustc_mir/interpret/memory.rs
Outdated
| let endianness = self.endianness(); | ||
|
|
||
| let val = match val { | ||
| ScalarMaybeUndef::Scalar(scalar) => scalar, | ||
| ScalarMaybeUndef::Undef => return self.mark_definedness(ptr, type_size, false), |
There was a problem hiding this comment.
This still early returns (also below). I had a comment about that in the first round already. Unfortunately GitHub is really bad at tracking review comments...
Previously this function always called self.check_align, now it does not seem to do that at all any more?
There was a problem hiding this comment.
all the code below makes no sense without the scalar from the other match arm.
I reintroduced the check_align in the very beginning of the function. I'm not sure why we only checked alignment for non-zsts...
RalfJung
reviewed
Aug 3, 2018
| @@ -1554,7 +1545,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M | |||
| let b_val = self.memory.read_scalar(b_ptr, ptr_align, b_size)?; | |||
| Ok(Some(Value::ScalarPair(a_val, b_val))) | |||
| } | |||
| _ => Ok(None), | |||
| _ => Ok(None), | |||
RalfJung
reviewed
Aug 3, 2018
| @@ -60,12 +57,12 @@ impl<'tcx> ConstValue<'tcx> { | |||
|
|
|||
| #[inline] | |||
| pub fn to_bits(&self, size: Size) -> Option<u128> { | |||
| self.to_scalar()?.to_bits(size).ok() | |||
| self.try_to_scalar()?.to_bits(size).ok() | |||
|
r=me with |
|
@bors r=RalfJung |
|
📌 Commit 828aebf has been approved by |
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit 828aebf with merge ca7b6345191e8da156fe6989f29b6a03c32800ba... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors retry lolwat? |
bors
added
S-waiting-on-bors
Another instance of travis-ci/travis-ci#4924 |
|
☀️ Test successful - status-appveyor, status-travis |
|
📣 Toolstate changed by #52712! Tested on commit 59fa6bd. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). |
rust-highfive
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
Aug 3, 2018
Tested on commit rust-lang/rust@59fa6bd. Direct link to PR: <rust-lang/rust#52712> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
|
RLS and clippy: Miri: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
r? @RalfJung
cc @eddyb
basically all kinds of silent failures that never occurred are assertions now