-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Miri api refactor #50967
Miri api refactor #50967
Conversation
ByRef(Pointer, Align), | ||
ByVal(PrimVal), | ||
ByValPair(PrimVal, PrimVal), | ||
ByRef(Scalar, Align), |
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.
Should this be Pointer
? Or is there a real usecase of ByRef
pointing to a non-allocation?
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.
yes, it's used to preserve zst identity
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.
But this is Value
, which means the address shouldn't be observable, only the value behind the pointer.
src/librustc/mir/interpret/value.rs
Outdated
/// matches Value's optimizations for easy conversions between these two types | ||
#[derive(Clone, Copy, Debug, Eq, PartialEq, RustcEncodable, RustcDecodable, Hash)] | ||
pub enum ConstValue<'tcx> { | ||
/// Used only for types with layout::abi::Scalar ABI and ZSTs which use PrimVal::Undef | ||
ByVal(PrimVal), | ||
/// Used only for types with layout::abi::Scalar ABI and ZSTs which use Scalar::Undef |
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.
Scalar::Undef
comment outdated?
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.
fixed
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.
Doesn't seem changed to me.
src/librustc/mir/interpret/value.rs
Outdated
} | ||
|
||
#[inline] | ||
pub fn to_primval(&self) -> Option<PrimVal> { | ||
pub fn to_primval(&self) -> Option<Scalar> { |
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.
Should primval
be replaced with scalar
?
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.
yes, done
src/librustc/mir/interpret/value.rs
Outdated
} | ||
|
||
#[derive(Clone, Copy, Debug, PartialEq)] | ||
pub enum PrimValKind { | ||
pub enum ScalarKind { | ||
I8, I16, I32, I64, I128, | ||
U8, U16, U32, U64, U128, |
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.
You could at least wrap layout::Integer
for signed and unsigned integers, if you want to keep ScalarKind
at all (as opposed to reusing layout::Primitive
).
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.
done
src/librustc/ty/sty.rs
Outdated
@@ -1755,6 +1757,25 @@ impl<'a, 'gcx, 'tcx> TyS<'tcx> { | |||
_ => bug!("cannot convert type `{:?}` to a closure kind", self), | |||
} | |||
} | |||
|
|||
/// If this type is a scalar, compute its size without | |||
/// going through `tcx.layout_of` |
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 not sure this is a good idea...
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.
and it's gone!
src/librustc/mir/mod.rs
Outdated
values.iter() | ||
.map(|&u| { | ||
let mut s = String::new(); | ||
print_miri_value( | ||
Value::ByVal(PrimVal::Bytes(u)), | ||
Value::Scalar(Scalar::Bits { bits: u, defined: size }), |
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.
Since this is for printing, could you pass in 128? There are certainly 128 defined bits, trivially.
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 this is used for sign extension o_O.
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 will still be passing this as a Scalar
, because the function is also called elsewhere with other variants, but I fixed the sign extension thing
Scalar::Bits { | ||
bits: s.len() as u128, | ||
defined: self.tcx.data_layout.pointer_size.bits() as u8, | ||
}, |
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.
Can't this use .to_value_with_len
?
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.
jup, done
☔ The latest upstream changes (presumably #50866) made this pull request unmergeable. Please resolve the merge conflicts. |
85638ee
to
ca34069
Compare
@@ -1149,11 +1149,17 @@ impl<'tcx> TerminatorKind<'tcx> { | |||
Return | Resume | Abort | Unreachable | GeneratorDrop => vec![], | |||
Goto { .. } => vec!["".into()], | |||
SwitchInt { ref values, switch_ty, .. } => { | |||
let size = ty::tls::with(|tcx| { |
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.
Please call this bits
, or, better, keep the name but do size.bits() as u8
at use sites.
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.
done
src/librustc/mir/mod.rs
Outdated
write!(f, "{}f32", Single::from_bits(bits)), | ||
(Value::ByVal(PrimVal::Bytes(bits)), &TyFloat(ast::FloatTy::F64)) => | ||
(Value::Scalar(Scalar::Bits { bits, defined: 64 }), &TyFloat(ast::FloatTy::F64)) => |
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.
Can you remove all the defined
pattern-matching in here?
src/librustc/mir/mod.rs
Outdated
}, | ||
(Value::Scalar(Scalar::Bits { bits, defined: 32 }), &TyChar) => | ||
write!(f, "{:?}", ::std::char::from_u32(bits as u32).unwrap()), | ||
(Value::Scalar(Scalar::Bits { defined: 0, .. }), &TyFnDef(did, _)) => |
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.
Also two more cases here.
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.
In fact, this specific one can ignore even whether the value is a Value::Scalar
.
src/librustc/ty/layout.rs
Outdated
@@ -1514,28 +1513,28 @@ impl<'a, 'tcx> LayoutOf for LayoutCx<'tcx, ty::maps::TyCtxtAt<'a, 'tcx, 'tcx>> { | |||
} | |||
|
|||
// Helper (inherent) `layout_of` methods to avoid pushing `LayoutCx` to users. | |||
impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> { | |||
impl<'a, 'tcx, 'empty> TyCtxt<'a, 'tcx, 'empty> { |
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.
Please use impl<'a, 'tcx> TyCtxt<'a, 'tcx, '_>
if you don't want to name the lifetime.
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 tried that, that doesn't compile
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.
Doesn't it need a feature-gate? in_band_lifetimes
I think?
src/librustc/ty/layout.rs
Outdated
param_env: param_env_and_ty.param_env | ||
}; | ||
cx.layout_of(param_env_and_ty.value) | ||
} | ||
} | ||
|
||
impl<'a, 'tcx> ty::maps::TyCtxtAt<'a, 'tcx, 'tcx> { | ||
impl<'a, 'tcx, 'empty> ty::maps::TyCtxtAt<'a, 'tcx, 'empty> { |
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.
Same here.
@@ -68,7 +70,7 @@ pub fn primval_to_llvm(cx: &CodegenCx, | |||
&C_usize(cx, ptr.offset.bytes()), | |||
1, | |||
) }; | |||
if scalar.value != layout::Pointer { | |||
if layout.value != layout::Pointer { | |||
unsafe { llvm::LLVMConstPtrToInt(llval, llty.to_ref()) } | |||
} else { | |||
consts::bitcast(llval, llty) |
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.
Looking at this, this sort of thing probably needs to always use bitcast
after LLVMConstPtrToInt
, in case the latter is needed at all. You should add a test for an union
used to convert a reference into a f32
/ f64
(depending on #[cfg(target_pointer_width)]
), which I highly suspect trips a LLVM assertion in the current state of the code.
src/librustc_mir/hair/cx/mod.rs
Outdated
let param_ty = self.param_env.and(gcx.lift(&ty).unwrap()); | ||
let bit_width = gcx.layout_of(param_ty).unwrap().size.bits(); | ||
trace!("clamp {} with size {} and amt {}", n, bit_width, 128 - bit_width); | ||
let amt = 128 - bit_width; | ||
let result = (n << amt) >> amt; |
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.
Can't this use some truncate
function from miri
?
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.
won't fix in this PR, there's an issue open for cleaning up this kind of code: #49937
src/librustc_mir/hair/cx/mod.rs
Outdated
Scalar::Bits { | ||
bits: s.len() as u128, | ||
defined: self.tcx.data_layout.pointer_size.bits() as u8, | ||
} |
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.
Can't this use with_len
or something?
src/librustc_mir/hair/cx/mod.rs
Outdated
LitKind::Int(n, _) if neg => { | ||
let n = n as i128; | ||
let n = n.overflowing_neg().0; | ||
let n = clamp(n as u128); | ||
ConstValue::ByVal(PrimVal::Bytes(n)) | ||
clamp(n as u128) |
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.
Is this a true clamping operation (i.e. saturating), or a truncation? I actually expect truncation.
src/librustc_mir/hair/pattern/mod.rs
Outdated
lo, | ||
hi, | ||
self.param_env.and(ty), | ||
).unwrap(); |
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.
Note that this unwrap
can fail - we should be matching on Option<Ordering>
below and explicitly list all the possible cases - currently the _
success at the end of the match
would catch None
which would be bad.
src/librustc_mir/hair/pattern/mod.rs
Outdated
)) | ||
) if size_a == size_b && defined_a == defined_b && | ||
ptr_a.offset.bytes() == 0 && ptr_b.offset.bytes() == 0 && | ||
tcx.data_layout.pointer_size.bits() == defined_a.into() => { |
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.
You're comparing defined
again. At most these should <=
relationships, not ==
.
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 is, each of defined_a
and defined_b
should be larger or equal to pointer_size.bits()
- why not use to_bits
with usize
instead?
src/librustc_mir/hair/pattern/mod.rs
Outdated
let map = tcx.alloc_map.lock(); | ||
let alloc_a = map.unwrap_memory(ptr_a.alloc_id); | ||
let alloc_b = map.unwrap_memory(ptr_b.alloc_id); | ||
if alloc_a.bytes.len() as u64 == size_a as u64 { |
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.
Would be better to cast alloc_a.bytes.len()
to u128
.
src/librustc_mir/interpret/memory.rs
Outdated
PrimVal::Bytes(bytes) => { | ||
let v = ((bytes as u128) % (1 << self.pointer_size().bytes())) as u64; | ||
Scalar::Bits { bits, defined } => { | ||
if defined <= self.pointer_size().bits() as u8 { |
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 wrong, shouldn't be <
?
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.
Also, please cast the small type to the large one for comparisons.
src/librustc_mir/interpret/memory.rs
Outdated
return err!(ReadUndefBytes); | ||
} | ||
// FIXME: what on earth does this line do? docs or fix needed! | ||
let v = ((bits as u128) % (1 << self.pointer_size().bytes())) as u64; |
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.
It's a truncation, shouldn't be necessary nowadays.
src/librustc_mir/interpret/memory.rs
Outdated
assert_eq!(size, self.pointer_size()); | ||
val.offset.bytes() as u128 | ||
} | ||
|
||
PrimVal::Bytes(bytes) => bytes, | ||
Scalar::Bits { bits, defined } if defined >= size.bits() as u8 && defined != 0 => bits, |
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'd write size.bits() != 0
here instead.
src/librustc_mir/interpret/memory.rs
Outdated
Value::ByVal(PrimVal::Undef) => err!(ReadUndefBytes), | ||
Value::ByVal(_) => bug!("expected ptr and length, got {:?}", value), | ||
Value::Scalar(Scalar::Bits { defined: 0, .. }) => err!(ReadUndefBytes), | ||
Value::Scalar(_) => bug!("expected ptr and length, got {:?}", value), |
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.
How is it possible to get a Value::Scalar(Scalar::undef())
for a type with an Abi::ScalarPair
?
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.
by reading a fat pointer from undefined bytes I'd presume
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 should produce a ScalarPair
with two undefs.
PartialOrd, Ord)] | ||
pub enum FloatTy { | ||
F32, | ||
F64, |
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.
For the record, I disagree with this, in general rustc
's typesystem should not include rustc_target::abi
types.
@bors r=eddyb |
📌 Commit 50d3783 has been approved by |
⌛ Testing commit 50d3783 with merge 53fc7286e4ea16cc5909e0ad96d16e714501132c... |
💔 Test failed - status-appveyor |
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 r=eddyb |
📌 Commit 5f599bb has been approved by |
☀️ Test successful - status-appveyor, status-travis |
@@ -780,7 +793,11 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M | |||
let ty = self.place_ty(place); | |||
let place = self.eval_place(place)?; | |||
let discr_val = self.read_discriminant_value(place, ty)?; | |||
self.write_primval(dest, PrimVal::Bytes(discr_val), dest_ty)?; | |||
let defined = self.layout_of(ty).unwrap().size.bits() as u8; |
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 believe this causes #51086. ty
is the enum of which the discriminant is taken. This should have been dest_ty
rustc_target: inline abi::FloatTy into abi::Primitive. This effectively undoes a small part of @oli-obk's rust-lang#50967, now that the rest of the compiler doesn't use the `FloatTy` definition from `rustc_target`, post-rust-lang#65884.
r? @eddyb
cc @Zoxc
based on #50916