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

Conversation

@oli-obk
Copy link
Contributor

oli-obk commented Dec 8, 2019

…y causes an error during evaluation

cc #67083 (does not fix because we still need the beta backport)

r? @wesleywiser

cc @RalfJung

…y causes an error during evaluation
if !p.is_indirect() {
trace!("skipping store of const value {:?} to {:?}", c, p);
return;
match c.literal.val {

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 8, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 9, 2019

Member

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

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 9, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 9, 2019

Member

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

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 9, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 9, 2019

Member

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.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 10, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 10, 2019

Member

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.

This comment has been minimized.

Copy link
@eddyb

eddyb Dec 10, 2019

Member

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?

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 10, 2019

Author Contributor

😂

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

@@ -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");

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 8, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 9, 2019

Member

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?

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 9, 2019

Member

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

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 9, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 9, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 9, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 9, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 9, 2019

Member

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

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 10, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 10, 2019

Member

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

Copy link
Member

wesleywiser left a comment

One question but these changes look good to me. Feel free to r=me after addressing.

src/test/ui/consts/assoc_const_generic_impl.rs Outdated Show resolved Hide resolved
@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Dec 10, 2019

@bors r=wesleywiser

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 10, 2019

📌 Commit c143471 has been approved by wesleywiser

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 10, 2019
Ensure that we get a hard error on generic ZST constants if their bod…

…y causes an error during evaluation

cc rust-lang#67083 (does not fix because we still need the beta backport)

r? @wesleywiser

cc @RalfJung
bors added a commit that referenced this pull request Dec 10, 2019
Rollup of 9 pull requests

Successful merges:

 - #66377 (Update RELEASES.md for 1.40.0)
 - #67134 (Ensure that we get a hard error on generic ZST constants if their bod…)
 - #67152 (Sort auto trait and blanket implementations display)
 - #67154 (Fix typos in src/libcore/alloc.rs docs)
 - #67168 (corrected comment in E0478)
 - #67178 (Move non clean impls items)
 - #67180 (doc: Use .copied() instead of .cloned() in Vec example)
 - #67181 (Update hashmap doc)
 - #67193 (In which we start tracking polonius in `-Z self-profile`)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Dec 11, 2019
Rollup of 9 pull requests

Successful merges:

 - #66377 (Update RELEASES.md for 1.40.0)
 - #67134 (Ensure that we get a hard error on generic ZST constants if their bod…)
 - #67152 (Sort auto trait and blanket implementations display)
 - #67154 (Fix typos in src/libcore/alloc.rs docs)
 - #67168 (corrected comment in E0478)
 - #67178 (Move non clean impls items)
 - #67180 (doc: Use .copied() instead of .cloned() in Vec example)
 - #67181 (Update hashmap doc)
 - #67193 (In which we start tracking polonius in `-Z self-profile`)

Failed merges:

r? @ghost
@bors bors merged commit c143471 into rust-lang:master Dec 11, 2019
4 checks passed
4 checks passed
pr #20191209.17 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Centril added a commit to Centril/rust that referenced this pull request Dec 11, 2019
…=oli-obk

Ensure that panicking in constants eventually errors

based on rust-lang#67134

closes rust-lang#66975

r? @oli-obk
@oli-obk oli-obk deleted the oli-obk:const_prop_zst branch Dec 13, 2019
@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Dec 13, 2019

@rust-lang/compiler we forgot this beta nomination yesterday. It fixes a regression from stable to beta, so if we want the next stable (next week) not to have this but, we need to backport now

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 13, 2019

Accepted for backport in today's design meeting

@oli-obk oli-obk mentioned this pull request Dec 14, 2019
bors added a commit that referenced this pull request Dec 14, 2019
[beta] Beta backports

Backporting the following pull requests:

 * resolve: Always resolve visibilities on impl items #67236
 * resolve: Resolve visibilities on fields with non-builtin attributes #67106
 * E0023: handle expected != tuple pattern type #67044
 * Fix `unused_parens` triggers on macro by example code #66983
 * Fix some issues with attributes on unnamed fields #66669
 * Ensure that we get a hard error on generic ZST constants if their bodies #67134 (via #67297)

Some of these conflicted on merge, I resolved where possible, sometimes by cherry-picking a commit or two more from the relevant PRs. Since those changes are necessary though for backport to proceed (otherwise not even std/core compile), seems fine -- they're fairly minor cleanups anyway.
bors added a commit that referenced this pull request Jan 14, 2020
[Beta] Backports

I did not include #67134 and #67289 since they seem to be on beta already.

* Fix up Command Debug output when arg0 is specified. #67219
* Do not ICE on unnamed future #67289
* Don't suppress move errors for union fields #67314
* Reenable static linking of libstdc++ on windows-gnu #67410
* Use the correct type for static qualifs #67621
* Treat extern statics just like statics in the "const pointer to static" representation #67630
* Do not ICE on lifetime error involving closures #67687
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.