Skip to content
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

don't allow ZST in ScalarInt #98957

Merged
merged 4 commits into from
Jul 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_codegen_cranelift/src/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ pub(crate) fn codegen_const_value<'tcx>(
}

match const_val {
ConstValue::ZeroSized => unreachable!(), // we already handles ZST above
ConstValue::Scalar(x) => match x {
Scalar::Int(int) => {
if fx.clif_type(layout.ty).is_some() {
Expand Down
9 changes: 4 additions & 5 deletions compiler/rustc_codegen_gcc/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use rustc_codegen_ssa::traits::{
StaticMethods,
};
use rustc_middle::mir::Mutability;
use rustc_middle::ty::ScalarInt;
use rustc_middle::ty::layout::{TyAndLayout, LayoutOf};
use rustc_middle::mir::interpret::{ConstAllocation, GlobalAlloc, Scalar};
use rustc_target::abi::{self, HasDataLayout, Pointer, Size};
Expand Down Expand Up @@ -159,13 +158,13 @@ impl<'gcc, 'tcx> ConstMethods<'tcx> for CodegenCx<'gcc, 'tcx> {
None
}

fn zst_to_backend(&self, _ty: Type<'gcc>) -> RValue<'gcc> {
self.const_undef(self.type_ix(0))
}

fn scalar_to_backend(&self, cv: Scalar, layout: abi::Scalar, ty: Type<'gcc>) -> RValue<'gcc> {
let bitsize = if layout.is_bool() { 1 } else { layout.size(self).bits() };
match cv {
Scalar::Int(ScalarInt::ZST) => {
assert_eq!(0, layout.size(self).bytes());
self.const_undef(self.type_ix(0))
}
Scalar::Int(int) => {
let data = int.assert_bits(layout.size(self));

Expand Down
9 changes: 4 additions & 5 deletions compiler/rustc_codegen_llvm/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use rustc_codegen_ssa::traits::*;
use rustc_middle::bug;
use rustc_middle::mir::interpret::{ConstAllocation, GlobalAlloc, Scalar};
use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
use rustc_middle::ty::ScalarInt;
use rustc_target::abi::{self, AddressSpace, HasDataLayout, Pointer, Size};

use libc::{c_char, c_uint};
Expand Down Expand Up @@ -223,13 +222,13 @@ impl<'ll, 'tcx> ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> {
})
}

fn zst_to_backend(&self, _llty: &'ll Type) -> &'ll Value {
self.const_undef(self.type_ix(0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think i0 is a legal LLVM type? At least not according to https://llvm.org/docs/LangRef.html#integer-type

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this from elsewhere... 🤷

}

fn scalar_to_backend(&self, cv: Scalar, layout: abi::Scalar, llty: &'ll Type) -> &'ll Value {
let bitsize = if layout.is_bool() { 1 } else { layout.size(self).bits() };
match cv {
Scalar::Int(ScalarInt::ZST) => {
assert_eq!(0, layout.size(self).bytes());
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm looking at this (in the context of Rust-GPU updating the nightly version of Rust we use).

How could this ever pass? abi::Scalar just can't encode sizes other than 1, 2, 4, 8, 16:

pub enum Integer {
I8,
I16,
I32,
I64,
I128,
}

pub enum Primitive {
/// The `bool` is the signedness of the `Integer` type.
///
/// One would think we would not care about such details this low down,
/// but some ABIs are described in terms of C types and ISAs where the
/// integer arithmetic is done on {sign,zero}-extended registers, e.g.
/// a negative integer passed by zero-extension will appear positive in
/// the callee, and most operations on it will produce the wrong values.
Int(Integer, bool),
F32,
F64,
Pointer,
}

pub enum Scalar {
Initialized {
value: Primitive,
// FIXME(eddyb) always use the shortest range, e.g., by finding
// the largest space between two consecutive valid values and
// taking everything else as the (shortest) valid range.
valid_range: WrappingRange,
},
Union {
/// Even for unions, we need to use the correct registers for the kind of
/// values inside the union, so we keep the `Primitive` type around. We
/// also use it to compute the size of the scalar.
/// However, unions never have niches and even allow undef,
/// so there is no `valid_range`.
value: Primitive,
},
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the const eval Scalar, not abi Scalar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

layout is abi::Scalar though.

If you scroll up through this PR there is a comment of mine somewhere saying that this must have been dead code.

self.const_undef(self.type_ix(0))
}
Scalar::Int(int) => {
let data = int.assert_bits(layout.size(self));
let llval = self.const_uint_big(self.type_ix(bitsize), data);
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_codegen_ssa/src/mir/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
let llval = bx.scalar_to_backend(x, scalar, bx.immediate_backend_type(layout));
OperandValue::Immediate(llval)
}
ConstValue::ZeroSized => {
let llval = bx.zst_to_backend(bx.immediate_backend_type(layout));
OperandValue::Immediate(llval)
}
Comment on lines +87 to +90
Copy link
Member

@eddyb eddyb Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you expand the diff there's a if layout.is_zst() { above, that early-returns, so this should've been unreachable!() without the illegal zst_to_backend implementations existing, looks like.
(and this is the only callsite of zst_to_backend AFAICT)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I think I see how this happened, scalar_to_backend had a nonsense Scalar::Int(ScalarInt::ZST) => { case from many years ago (at least 4? I wasn't able to easily go far back enough in git blame).

And this PR just refactored it into a more obvious spot.

ConstValue::Slice { data, start, end } => {
let Abi::ScalarPair(a_scalar, _) = layout.abi else {
bug!("from_const: invalid ScalarPair layout: {:#?}", layout);
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/src/traits/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub trait ConstMethods<'tcx>: BackendTypes {
fn const_data_from_alloc(&self, alloc: ConstAllocation<'tcx>) -> Self::Value;

fn scalar_to_backend(&self, cv: Scalar, layout: abi::Scalar, llty: Self::Type) -> Self::Value;
fn zst_to_backend(&self, llty: Self::Type) -> Self::Value;
fn from_const_alloc(
&self,
layout: TyAndLayout<'tcx>,
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use super::{CompileTimeEvalContext, CompileTimeInterpreter, ConstEvalErr};
use crate::interpret::eval_nullary_intrinsic;
use crate::interpret::{
intern_const_alloc_recursive, Allocation, ConstAlloc, ConstValue, CtfeValidationMode, GlobalId,
Immediate, InternKind, InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking, Scalar,
Immediate, InternKind, InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking,
ScalarMaybeUninit, StackPopCleanup,
};

Expand Down Expand Up @@ -157,7 +157,7 @@ pub(super) fn op_to_const<'tcx>(
"this MPlaceTy must come from a validated constant, thus we can assume the \
alignment is correct",
);
ConstValue::Scalar(Scalar::ZST)
ConstValue::ZeroSized
}
}
};
Expand Down
8 changes: 2 additions & 6 deletions compiler/rustc_const_eval/src/const_eval/valtrees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ pub fn valtree_to_const_value<'tcx>(
match ty.kind() {
ty::FnDef(..) => {
assert!(valtree.unwrap_branch().is_empty());
ConstValue::Scalar(Scalar::ZST)
ConstValue::ZeroSized
}
ty::Bool | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Char => match valtree {
ty::ValTree::Leaf(scalar_int) => ConstValue::Scalar(Scalar::Int(scalar_int)),
Expand Down Expand Up @@ -344,11 +344,7 @@ fn valtree_into_mplace<'tcx>(

match ty.kind() {
ty::FnDef(_, _) => {
ecx.write_immediate(
Immediate::Scalar(ScalarMaybeUninit::Scalar(Scalar::ZST)),
&place.into(),
)
.unwrap();
ecx.write_immediate(Immediate::Uninit, &place.into()).unwrap();
}
ty::Bool | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Char => {
let scalar_int = valtree.unwrap_leaf();
Expand Down
13 changes: 7 additions & 6 deletions compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

let Some(alloc) = self.get_place_alloc(mplace)? else {
return Ok(Some(ImmTy {
// zero-sized type
imm: Scalar::ZST.into(),
// zero-sized type can be left uninit
imm: Immediate::Uninit,
layout: mplace.layout,
}));
};
Expand Down Expand Up @@ -441,8 +441,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// This makes several assumptions about what layouts we will encounter; we match what
// codegen does as good as we can (see `extract_field` in `rustc_codegen_ssa/src/mir/operand.rs`).
let field_val: Immediate<_> = match (*base, base.layout.abi) {
// the field contains no information
_ if field_layout.is_zst() => Scalar::ZST.into(),
// the field contains no information, can be left uninit
_ if field_layout.is_zst() => Immediate::Uninit,
// the field covers the entire type
_ if field_layout.size == base.layout.size => {
assert!(match (base.layout.abi, field_layout.abi) {
Expand Down Expand Up @@ -553,8 +553,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
let layout = self.layout_of_local(frame, local, layout)?;
let op = if layout.is_zst() {
// Do not read from ZST, they might not be initialized
Operand::Immediate(Scalar::ZST.into())
// Bypass `access_local` (helps in ConstProp)
Operand::Immediate(Immediate::Uninit)
} else {
*M::access_local(frame, local)?
};
Expand Down Expand Up @@ -709,6 +709,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Operand::Indirect(MemPlace::from_ptr(ptr.into()))
}
ConstValue::Scalar(x) => Operand::Immediate(tag_scalar(x)?.into()),
ConstValue::ZeroSized => Operand::Immediate(Immediate::Uninit),
ConstValue::Slice { data, start, end } => {
// We rely on mutability being set correctly in `data` to prevent writes
// where none should happen.
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
#![feature(drain_filter)]
#![feature(intra_doc_pointers)]
#![feature(yeet_expr)]
#![feature(const_option)]
#![recursion_limit = "512"]
#![allow(rustc::potential_query_instability)]

Expand Down
14 changes: 6 additions & 8 deletions compiler/rustc_middle/src/mir/interpret/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,14 @@ pub struct ConstAlloc<'tcx> {
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, TyEncodable, TyDecodable, Hash)]
#[derive(HashStable)]
pub enum ConstValue<'tcx> {
/// Used only for types with `layout::abi::Scalar` ABI and ZSTs.
/// Used only for types with `layout::abi::Scalar` ABI.
///
/// Not using the enum `Value` to encode that this must not be `Uninit`.
Scalar(Scalar),

/// Only used for ZSTs.
ZeroSized,

/// Used only for `&[u8]` and `&str`
Slice { data: ConstAllocation<'tcx>, start: usize, end: usize },

Expand All @@ -55,6 +58,7 @@ impl<'a, 'tcx> Lift<'tcx> for ConstValue<'a> {
fn lift_to_tcx(self, tcx: TyCtxt<'tcx>) -> Option<ConstValue<'tcx>> {
Some(match self {
ConstValue::Scalar(s) => ConstValue::Scalar(s),
ConstValue::ZeroSized => ConstValue::ZeroSized,
ConstValue::Slice { data, start, end } => {
ConstValue::Slice { data: tcx.lift(data)?, start, end }
}
Expand All @@ -69,7 +73,7 @@ impl<'tcx> ConstValue<'tcx> {
#[inline]
pub fn try_to_scalar(&self) -> Option<Scalar<AllocId>> {
match *self {
ConstValue::ByRef { .. } | ConstValue::Slice { .. } => None,
ConstValue::ByRef { .. } | ConstValue::Slice { .. } | ConstValue::ZeroSized => None,
ConstValue::Scalar(val) => Some(val),
}
}
Expand Down Expand Up @@ -111,10 +115,6 @@ impl<'tcx> ConstValue<'tcx> {
pub fn from_machine_usize(i: u64, cx: &impl HasDataLayout) -> Self {
ConstValue::Scalar(Scalar::from_machine_usize(i, cx))
}

pub fn zst() -> Self {
Self::Scalar(Scalar::ZST)
}
}

/// A `Scalar` represents an immediate, primitive value existing outside of a
Expand Down Expand Up @@ -194,8 +194,6 @@ impl<Tag> From<ScalarInt> for Scalar<Tag> {
}

impl<Tag> Scalar<Tag> {
pub const ZST: Self = Scalar::Int(ScalarInt::ZST);

#[inline(always)]
pub fn from_pointer(ptr: Pointer<Tag>, cx: &impl HasDataLayout) -> Self {
Scalar::Ptr(ptr, u8::try_from(cx.pointer_size().bytes()).unwrap())
Expand Down
11 changes: 9 additions & 2 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1711,7 +1711,7 @@ impl<'tcx> Operand<'tcx> {
Operand::Constant(Box::new(Constant {
span,
user_ty: None,
literal: ConstantKind::Val(ConstValue::zst(), ty),
literal: ConstantKind::Val(ConstValue::ZeroSized, ty),
}))
}

Expand Down Expand Up @@ -2196,7 +2196,7 @@ impl<'tcx> ConstantKind<'tcx> {

#[inline]
pub fn zero_sized(ty: Ty<'tcx>) -> Self {
let cv = ConstValue::Scalar(Scalar::ZST);
let cv = ConstValue::ZeroSized;
Self::Val(cv, ty)
}

Expand Down Expand Up @@ -2772,6 +2772,13 @@ fn pretty_print_const_value<'tcx>(
fmt.write_str(&cx.into_buffer())?;
return Ok(());
}
(ConstValue::ZeroSized, ty::FnDef(d, s)) => {
let mut cx = FmtPrinter::new(tcx, Namespace::ValueNS);
cx.print_alloc_ids = true;
let cx = cx.print_value_path(*d, s)?;
fmt.write_str(&cx.into_buffer())?;
return Ok(());
}
// FIXME(oli-obk): also pretty print arrays and other aggregate constants by reading
// their fields instead of just dumping the memory.
_ => {}
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,9 @@ impl<'tcx> Visitor<'tcx> for ExtraComments<'tcx> {
self.push(&format!("+ user_ty: {:?}", user_ty));
}

// FIXME: this is a poor version of `pretty_print_const_value`.
let fmt_val = |val: &ConstValue<'tcx>| match val {
ConstValue::ZeroSized => format!("<ZST>"),
ConstValue::Scalar(s) => format!("Scalar({:?})", s),
ConstValue::Slice { .. } => format!("Slice(..)"),
ConstValue::ByRef { .. } => format!("ByRef(..)"),
Expand Down Expand Up @@ -679,6 +681,7 @@ pub fn write_allocations<'tcx>(
ConstValue::Scalar(interpret::Scalar::Int { .. }) => {
Either::Left(Either::Right(std::iter::empty()))
}
ConstValue::ZeroSized => Either::Left(Either::Right(std::iter::empty())),
ConstValue::ByRef { alloc, .. } | ConstValue::Slice { data: alloc, .. } => {
Either::Right(alloc_ids_from_alloc(alloc))
}
Expand Down
10 changes: 4 additions & 6 deletions compiler/rustc_middle/src/thir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,10 @@ pub enum ExprKind<'tcx> {
lit: ty::ScalarInt,
user_ty: Option<Canonical<'tcx, UserType<'tcx>>>,
},
/// A literal of a ZST type.
ZstLiteral {
user_ty: Option<Canonical<'tcx, UserType<'tcx>>>,
},
/// Associated constants and named constants
NamedConst {
def_id: DefId,
Expand Down Expand Up @@ -454,12 +458,6 @@ pub enum ExprKind<'tcx> {
},
}

impl<'tcx> ExprKind<'tcx> {
pub fn zero_sized_literal(user_ty: Option<Canonical<'tcx, UserType<'tcx>>>) -> Self {
ExprKind::NonHirLiteral { lit: ty::ScalarInt::ZST, user_ty }
}
}

/// Represents the association of a field identifier and an expression.
///
/// This is used in struct constructors.
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/thir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ pub fn walk_expr<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor: &mut V, expr: &Exp
Closure { closure_id: _, substs: _, upvars: _, movability: _, fake_reads: _ } => {}
Literal { lit: _, neg: _ } => {}
NonHirLiteral { lit: _, user_ty: _ } => {}
ZstLiteral { user_ty: _ } => {}
NamedConst { def_id: _, substs: _, user_ty: _ } => {}
ConstParam { param: _, def_id: _ } => {}
StaticRef { alloc_id: _, ty: _, def_id: _ } => {}
Expand Down
Loading