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

Ensure that we get a hard error on generic ZST constants if their bod… #67134

Merged
merged 2 commits into from
Dec 11, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
9 changes: 8 additions & 1 deletion src/librustc_codegen_ssa/mir/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
constant: &mir::Constant<'tcx>,
) -> Result<OperandRef<'tcx, Bx::Value>, ErrorHandled> {
match constant.literal.val {
// Special case unevaluated statics, because statics have an identity and thus should
// use `get_static` to get at their id.
// FIXME(oli-obk): can we unify this somehow, maybe by making const eval of statics
// always produce `&STATIC`. This may also simplify how const eval works with statics.
ty::ConstKind::Unevaluated(def_id, substs)
if self.cx.tcx().is_static(def_id) => {
assert!(substs.is_empty(), "we don't support generic statics yet");
Expand Down Expand Up @@ -46,7 +50,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
instance,
promoted: None,
};
self.cx.tcx().const_eval(ty::ParamEnv::reveal_all().and(cid))
self.cx.tcx().const_eval(ty::ParamEnv::reveal_all().and(cid)).map_err(|err| {
self.cx.tcx().sess.span_err(constant.span, "erroneous constant encountered");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an additional fix for when users turn off all lints with allow. In that case they may get a compiling program that runs into an abort. We now report a hard error again, as we already do in the monomorphized case

Copy link
Member

Choose a reason for hiding this comment

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

Wait, I thought we deliberately don't make const_err a hard error? Because that would be trivial to do, but there are backcompat concerns or so. So why don't these concerns apply here?

Copy link
Member

Choose a reason for hiding this comment

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

Also, won't this lead to errors being emitted in the middle of a Miri (the tool) execution? That seems odd.^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is codegen_ssa, and we already have a hard error for using erroneous constants in runtime code, it just doesn't work for polymorphic code because it's implemented on MIR. All this PR does is make the hard error work for polymorphic code, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=afc895c5754a7963154193ad32702bb1

If you comment out the let x = FOO; you don't get a hard error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference is that nonpolymorphic functions do not get monomorphized (because they don't need to get monomorphized). So when we const prop a nonpolymorphic function, when we encounter a constant, we can just eval it. When we encounter a constant in a polymorhphic function, we may only be able to eval it at monomorphization time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we monomorphized all MIR before codegenning it, this would solve itself, but we'd require loads of CPU and memory to do that, so we do the monomorphization during codegen on the fly.

Copy link
Member

Choose a reason for hiding this comment

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

Wait so the hard error from your example does not come from codegen, but from const-prop? That is surprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because const prop can actually eliminate the constant entirely, so we need to report it there. Same problem as the DCE problem discussed in the other thread. We can unify the scheme in one go.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so both of my questions have the same answer? I love it when that happens. :D

err
})
},
_ => Ok(self.monomorphize(&constant.literal)),
}
Expand Down
13 changes: 9 additions & 4 deletions src/librustc_mir/transform/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@

use rustc_index::bit_set::BitSet;
use rustc_index::vec::{Idx, IndexVec};
use rustc::ty::TyCtxt;
use rustc::ty::{self, TyCtxt};
use rustc::mir::*;
use rustc::mir::visit::{MutVisitor, Visitor, PlaceContext, MutatingUseContext};
use std::borrow::Cow;
Expand Down Expand Up @@ -367,9 +367,14 @@ impl<'a, 'tcx> Visitor<'tcx> for DeclMarker<'a, 'tcx> {
if let StatementKind::Assign(
box (p, Rvalue::Use(Operand::Constant(c)))
) = &stmt.kind {
if !p.is_indirect() {
trace!("skipping store of const value {:?} to {:?}", c, p);
return;
match c.literal.val {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the regression fix for the regression introduced in #66216

Copy link
Member

Choose a reason for hiding this comment

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

Looks like a rather unprincipled hack to me -- why only constants and why specifically unevaluated constants? Cc @eddyb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because all other constants can't hide errors in their body. This bug is solely about constants never getting evaluated, thus we are not getting any error

Copy link
Member

Choose a reason for hiding this comment

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

That seems quite annoying that we cannot DCE uses of constants -- and quite fragile, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not quite dead code. Just like let _: () = panic!(); still panics, we should still error on let _: () = ERRONEOUS_CONSTANT;

Copy link
Member

Choose a reason for hiding this comment

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

I think that is disputable... this makes consts more side-effecting than one might think.

Not being able to replace, in MIR optimizations, the computation of any ZST by just a ZST scalar is a big foot-gun, and I expect this to be quite detrimental to MIR-level optimizations. I feel like we should look into ways to obtain the "compilation failure" effect without constraining MIR transformations. For example, a MIR body could come with a list of consts that need to be error-free for this function to be compilable; that would avoid the need to keep these consts around in the MIR itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yea that would be a much cleaner way. Do you want me to rewrite the PR with that or can we merge it in the hope of getting it into beta before it becomes stable?

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 want to block anything here, certainly not a soundness fix for a release happening next week. ;)
So, sure, go ahead. Maybe add a link to this discussion in the code for people that stumble upon this and wonder what is going on and why.

Copy link
Member

Choose a reason for hiding this comment

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

For example, a MIR body could come with a list of consts that need to be error-free for this function to be compilable; that would avoid the need to keep these consts around in the MIR itself.

This is tempting me to use indices into such a list to refer to consts from the MIR itself.
Even if ty::Const is just a pointer, this would allow lowering that to u32, so maybe there's some place where we could take advantage of that?

I guess this is more of an unrelated "something @oli-obk / @spastorino could be doing later" thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😂

This is tempting me to use indices into such a list to refer to consts from the MIR itself.
Even if ty::Const is just a pointer, this would allow lowering that to u32, so maybe there's some place where we could take advantage of that?

omg yes

// Keep assignments from unevaluated constants around, since the evaluation
// may report errors, even if the use of the constant is dead code.
ty::ConstKind::Unevaluated(..) => {}
_ => if !p.is_indirect() {
trace!("skipping store of const value {:?} to {:?}", c, p);
return;
},
}
}
}
Expand Down
19 changes: 19 additions & 0 deletions src/test/ui/consts/assoc_const_generic_impl.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#![allow(const_err)]
oli-obk marked this conversation as resolved.
Show resolved Hide resolved

trait ZeroSized: Sized {
const I_AM_ZERO_SIZED: ();
fn requires_zero_size(self);
}

impl<T: Sized> ZeroSized for T {
const I_AM_ZERO_SIZED: () = [()][std::mem::size_of::<Self>()];
fn requires_zero_size(self) {
let () = Self::I_AM_ZERO_SIZED; //~ ERROR erroneous constant encountered
println!("requires_zero_size called");
}
}

fn main() {
().requires_zero_size();
42_u32.requires_zero_size();
}
8 changes: 8 additions & 0 deletions src/test/ui/consts/assoc_const_generic_impl.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: erroneous constant encountered
--> $DIR/assoc_const_generic_impl.rs:11:18
|
LL | let () = Self::I_AM_ZERO_SIZED;
| ^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error