-
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
Ensure that we get a hard error on generic ZST constants if their bod… #67134
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the regression fix for the regression introduced in #66216 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not quite dead code. Just like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. I guess this is more of an unrelated "something @oli-obk / @spastorino could be doing later" thing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😂
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; | ||
}, | ||
} | ||
} | ||
} | ||
|
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(); | ||
} |
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 | ||
|
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 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 caseThere 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.
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?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, won't this lead to errors being emitted in the middle of a Miri (the tool) execution? That seems odd.^^
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 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.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.
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.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.
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.
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.
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.
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.
Wait so the hard error from your example does not come from codegen, but from const-prop? That is surprising.
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, 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.
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.
Ah, so both of my questions have the same answer? I love it when that happens. :D