Stream: t-compiler

Topic: Replace our fragile safety scheme around erroneous constant


Santiago Pastorino (Apr 24 2020 at 19:44, on Zulip):

@oli @RalfJ is not entirely clear what's left to do here https://github.com/rust-lang/rust/issues/67191#issuecomment-619064331

Santiago Pastorino (Apr 24 2020 at 19:45, on Zulip):

we were checking with @Wesley Wiser and some code was changed meanwhile you've commented there and the PR was merged

Santiago Pastorino (Apr 24 2020 at 19:46, on Zulip):

please, let me know and I will follow up

oli (Apr 25 2020 at 08:57, on Zulip):

I think we can now remove the code changes in https://github.com/rust-lang/rust/pull/67134/files#diff-ac4e8531e3b428256ca33fa2bb472440

oli (Apr 25 2020 at 08:57, on Zulip):

(or just make it delay_span_bug)

RalfJ (Apr 25 2020 at 09:48, on Zulip):

@oli you also mentioned const-prop has to be careful here, which part of const-prop refers that to?

RalfJ (Apr 25 2020 at 09:49, on Zulip):

(I am referring to this comment)

RalfJ (Apr 25 2020 at 09:51, on Zulip):

In particular you said this code errors in const-prop -- which seems odd, it should error in codegen when the constant is actually "used".

RalfJ (Apr 25 2020 at 09:52, on Zulip):

and indeed I would expect this loop to also cover that constant

oli (Apr 25 2020 at 10:46, on Zulip):

well, it does error in const prop because const prop evaluates all Rvalue::Use in order to know the value for the local that is being assigned to, so when the local is referred to again later, we can propagate the constant into the use site of the local

oli (Apr 25 2020 at 10:46, on Zulip):

const prop doesn't have to be careful anymore, it can now freely eliminate even unevaluated constants as it pleases

oli (Apr 25 2020 at 10:46, on Zulip):

same goes for other optimizations

Santiago Pastorino (Apr 25 2020 at 11:18, on Zulip):

oli said:

I think we can now remove the code changes in https://github.com/rust-lang/rust/pull/67134/files#diff-ac4e8531e3b428256ca33fa2bb472440

that code doesn't exist anymore https://github.com/rust-lang/rust/blob/0b958790b336738540d027d645718713849638d7/src/librustc_codegen_ssa/mir/constant.rs#L14-L38

Santiago Pastorino (Apr 25 2020 at 11:20, on Zulip):

oli said:

const prop doesn't have to be careful anymore, it can now freely eliminate even unevaluated constants as it pleases

I don't understand this either, what happens then when you run cargo check? and also is not clear what we should remove from const prop

RalfJ (Apr 26 2020 at 20:35, on Zulip):

Santiago Pastorino said:

oli said:

I think we can now remove the code changes in https://github.com/rust-lang/rust/pull/67134/files#diff-ac4e8531e3b428256ca33fa2bb472440

that code doesn't exist anymore https://github.com/rust-lang/rust/blob/0b958790b336738540d027d645718713849638d7/src/librustc_codegen_ssa/mir/constant.rs#L14-L38

it does: https://github.com/rust-lang/rust/blob/0b958790b336738540d027d645718713849638d7/src/librustc_codegen_ssa/mir/constant.rs#L54

RalfJ (Apr 26 2020 at 20:36, on Zulip):

also there is still a FIXME there, but I am not sure if that is related

RalfJ (Apr 26 2020 at 20:37, on Zulip):

Santiago Pastorino said:

oli said:

const prop doesn't have to be careful anymore, it can now freely eliminate even unevaluated constants as it pleases

I don't understand this either, what happens then when you run cargo check? and also is not clear what we should remove from const prop

hm... maybe this can be removed?
https://github.com/rust-lang/rust/blob/7f3b3df9e2f2efe3434b4f6fc76462d2c8ad332f/src/librustc_mir/transform/const_prop.rs#L470

Santiago Pastorino (Apr 27 2020 at 16:31, on Zulip):

@RalfJ @oli to be honest I still have no idea what are we talking about exactly

Santiago Pastorino (Apr 27 2020 at 16:32, on Zulip):

RalfJ said:

Santiago Pastorino said:

oli said:

I think we can now remove the code changes in https://github.com/rust-lang/rust/pull/67134/files#diff-ac4e8531e3b428256ca33fa2bb472440

that code doesn't exist anymore https://github.com/rust-lang/rust/blob/0b958790b336738540d027d645718713849638d7/src/librustc_codegen_ssa/mir/constant.rs#L14-L38

it does: https://github.com/rust-lang/rust/blob/0b958790b336738540d027d645718713849638d7/src/librustc_codegen_ssa/mir/constant.rs#L54

yeah that code exist but is what causes the error we seem to want, if I remove that line there are not const errors anymore

Santiago Pastorino (Apr 27 2020 at 16:32, on Zulip):

I guess you're talking about something different but I can't understand what you're trying to say

RalfJ (Apr 28 2020 at 21:30, on Zulip):

yeah that code exist but is what causes the error we seem to want, if I remove that line there are not const errors anymore

that's odd, shouldn't wherever we iterate over required_const still report errors?

RalfJ (Apr 28 2020 at 21:31, on Zulip):

also when you wrote "that code doesn't exist anymore", I dont understand -- all the code that got added in #67134 in that file seems to still be there

RalfJ (Apr 28 2020 at 21:33, on Zulip):

RalfJ said:

yeah that code exist but is what causes the error we seem to want, if I remove that line there are not const errors anymore

that's odd, shouldn't wherever we iterate over required_const still report errors?

ah, that loop actually calls eval_mir_constant

RalfJ (Apr 28 2020 at 21:35, on Zulip):

but then, even before https://github.com/rust-lang/rust/pull/67134 we got some const-errors. and that is where this code in eval_mir_constant got added. so that place cannot be the only place where errors are emitted. that means there are at least 2 places, and that should be unnecessary now.

RalfJ (Apr 28 2020 at 21:35, on Zulip):

@Santiago Pastorino ^

RalfJ (Apr 28 2020 at 21:36, on Zulip):

and then there's a FIXME there with @oli's name in it that got added in #67134, not sure if that was resolved now or not

Santiago Pastorino (Apr 28 2020 at 21:39, on Zulip):

RalfJ said:

yeah that code exist but is what causes the error we seem to want, if I remove that line there are not const errors anymore

that's odd, shouldn't wherever we iterate over required_const still report errors?

yes, that happens here https://github.com/rust-lang/rust/pull/70820/files#diff-32c57af5c8e23eb048f55d1e955e5cd5R195

Santiago Pastorino (Apr 28 2020 at 21:40, on Zulip):

but we are calling exactly the function that reports the error, so if I remove from there we have no error reporting anymore

Santiago Pastorino (Apr 28 2020 at 21:40, on Zulip):

RalfJ said:

also when you wrote "that code doesn't exist anymore", I dont understand -- all the code that got added in #67134 in that file seems to still be there

I'm not sure what parts are we talking about, https://github.com/rust-lang/rust/pull/67134/files#diff-ac4e8531e3b428256ca33fa2bb472440R53 this is not there anymore

Santiago Pastorino (Apr 28 2020 at 21:41, on Zulip):

maybe is better if you point to the code in master that you're talking about?

Santiago Pastorino (Apr 28 2020 at 21:42, on Zulip):

was commenting meanwhile I was reading, now I see that you saw that the loop calls eval_mir_constant :)

Santiago Pastorino (Apr 28 2020 at 21:43, on Zulip):

@RalfJ this https://github.com/rust-lang/rust/blob/b7bd7c1024a1449449c3ae7b4b4c63a904a620e6/src/librustc_codegen_ssa/mir/constant.rs#L22-L23 is what we have in master

Santiago Pastorino (Apr 28 2020 at 21:43, on Zulip):

but the FIXME seems unrelated to my changes

Santiago Pastorino (Apr 28 2020 at 21:44, on Zulip):

I meant, I see the general idea you're mentioning here that we may be reporting things from different places and you want to unify :)

RalfJ (Apr 28 2020 at 21:59, on Zulip):

Santiago Pastorino said:

RalfJ said:

also when you wrote "that code doesn't exist anymore", I dont understand -- all the code that got added in #67134 in that file seems to still be there

I'm not sure what parts are we talking about, https://github.com/rust-lang/rust/pull/67134/files#diff-ac4e8531e3b428256ca33fa2bb472440R53 this is not there anymore

uh, yes it is? I linked to it above, it is right here:
https://github.com/rust-lang/rust/blob/0b958790b336738540d027d645718713849638d7/src/librustc_codegen_ssa/mir/constant.rs#L54

RalfJ (Apr 28 2020 at 21:59, on Zulip):

just got re-formatted a bit

RalfJ (Apr 28 2020 at 21:59, on Zulip):

we now repeated this exact conversation with the same links twice

RalfJ (Apr 28 2020 at 21:59, on Zulip):

I feel like in groundhog day

RalfJ (Apr 28 2020 at 22:00, on Zulip):

so maybe let's make sure we understand why we disagree about whether some piece of code still exists, before going on^^

RalfJ (Apr 28 2020 at 22:01, on Zulip):

I am talking specifically about the span_err(constant.span, "erroneous constant encountered")

RalfJ (Apr 28 2020 at 22:01, on Zulip):

it got added in that PR, and it is still around

RalfJ (Apr 28 2020 at 22:03, on Zulip):

Santiago Pastorino said:

I meant, I see the general idea you're mentioning here that we may be reporting things from different places and you want to unify :)

yes. previously we had to report errors from consts in a bunch of places to make sure we dont forget a const. now we have that loop over required_consts that takes care of that. so some of those places should be unnecessary now, and we should be able to clean them up.

RalfJ (Apr 28 2020 at 22:04, on Zulip):

eval_mir_constant in codegen is probably the most sensible place for such errors to be reported, this is where monomorphization happens... but then likely it should be the only place (plus that lint that walks over all consts to make sure we fire errors for unused consts), and given how recently it was added, I doubt it currently is the only place.

Wesley Wiser (Apr 29 2020 at 00:51, on Zulip):

RalfJ said:

Santiago Pastorino said:

I meant, I see the general idea you're mentioning here that we may be reporting things from different places and you want to unify :)

yes. previously we had to report errors from consts in a bunch of places to make sure we dont forget a const. now we have that loop over required_consts that takes care of that. so some of those places should be unnecessary now, and we should be able to clean them up.

But isn't the loop over required_consts in librustc_codegen_ssa? We want to eagerly report errors where we can otherwise users will see cargo check pass and then cargo build fail with errors.

RalfJ (Apr 29 2020 at 17:47, on Zulip):

we had codegen-time const errors before, too. so there must now be at least two places in codegen than emit such errors: eval_mir_constant and where it was done before

RalfJ (Apr 29 2020 at 17:48, on Zulip):

also @Wesley Wiser when we emit such errors from optimizations, cargo check might not show them either if it doesnt run optimizations

RalfJ (Apr 29 2020 at 17:49, on Zulip):

I dont think "randomly sprinkle const-error-emitting code all over the place" is a sustainable strategy for avoiding check vs build mismatches

Wesley Wiser (Apr 29 2020 at 17:51, on Zulip):

I completely agree with you there! I just want to make sure we don't accidentally regress check functionality from how it works today.

Félix Fischer (Apr 30 2020 at 04:11, on Zulip):

Ohh this is interesting.
So if I understood this correctly:

Say we introduced new functionality in const-prop that detects errors at compile time just by function of it having the ability of e.g. detecting when you're computing 1/0 with constant values.

Then, cargo check must also gain that ability. Lest it had less detecting capabilities than cargo build does.
Did I get you right, @RalfJ?

Félix Fischer (Apr 30 2020 at 04:13, on Zulip):

Because if I did, I have to now ask: what do we do in that scenario? Because that's precisely what we just added to const-prop like yesterday.

(Note: the analysis is not yet complete. We're able to find more unescapable panics than before, but not all of the ones we would be able to)

RalfJ (Apr 30 2020 at 06:41, on Zulip):

@Félix Fischer well, it's not a "must" -- already today, we have discrepancies between cargo check, cargo build, cargo build --release. It's not great but it happens.

RalfJ (Apr 30 2020 at 06:41, on Zulip):

I had an example once, not sure if I can find it again...

RalfJ (Apr 30 2020 at 06:42, on Zulip):

see https://github.com/rust-lang/rust/issues/70923 for example

RalfJ (Apr 30 2020 at 06:43, on Zulip):

I think the debug vs release one that I knew of has been fixed

Wesley Wiser (Apr 30 2020 at 10:45, on Zulip):

Also, cargo check queries for optimized_mir so const-prop is always run.

Félix Fischer (Apr 30 2020 at 13:37, on Zulip):

Woah! that's awesome, @Wesley Wiser
cargo check is OP.

RalfJ (Apr 30 2020 at 21:12, on Zulip):

anyway, let's go back to the const-checking cleanup thing

RalfJ (Apr 30 2020 at 21:12, on Zulip):

I think we are blocked on @oli answering the open questions around where there might still be stuff to clean up...

RalfJ (Apr 30 2020 at 21:13, on Zulip):

@Santiago Pastorino you didn't respond to my last responses to what you wrote

RalfJ (Apr 30 2020 at 21:20, on Zulip):

After grepping a bit I think I came to the conclusion that all the "erroneous constant" errors we saw before https://github.com/rust-lang/rust/pull/67134 actually come from const_prop; @oli is that right?

Santiago Pastorino (Apr 30 2020 at 21:21, on Zulip):

RalfJ said:

Santiago Pastorino you didn't respond to my last responses to what you wrote

hey, sorry and thanks for your help at the same time

Santiago Pastorino (Apr 30 2020 at 21:21, on Zulip):

couldn't read any this stuff yet, gonna check all this again as soon as possible, unsure if I will be able today

RalfJ (Apr 30 2020 at 21:26, on Zulip):

so I think I have two concrete questions/cleanups:

RalfJ (Apr 30 2020 at 21:27, on Zulip):

also the error raised by const_prop and codegen is a bit different ("erroneous constant used" vs "erroneous constant encountered")

oli (May 01 2020 at 08:32, on Zulip):

The advantage of reporting errors in const prop over leaving it to the fallback scheme is that the fallback scheme only triggers during monomorphization

oli (May 01 2020 at 08:32, on Zulip):

So I don't think we should remove https://github.com/rust-lang/rust/blob/bd0bacc694d7d8175804bb6f690cb846bfa4a9ee/src/librustc_mir/transform/const_prop.rs#L418 even if we can

oli (May 01 2020 at 08:33, on Zulip):

O_o I think https://github.com/rust-lang/rust/blob/be8589fc31162bb71b0f765beba6ce73ec8ba93a/src/librustc_codegen_ssa/mir/constant.rs#L22 is dead code

oli (May 01 2020 at 08:34, on Zulip):

we should be able to change the entire function body to https://github.com/rust-lang/rust/blob/be8589fc31162bb71b0f765beba6ce73ec8ba93a/src/librustc_codegen_ssa/mir/constant.rs#L33-L35

Santiago Pastorino (May 01 2020 at 12:26, on Zulip):

finally catched up with this now

Santiago Pastorino (May 01 2020 at 12:26, on Zulip):

oli said:

O_o I think https://github.com/rust-lang/rust/blob/be8589fc31162bb71b0f765beba6ce73ec8ba93a/src/librustc_codegen_ssa/mir/constant.rs#L22 is dead code

going to remove this code

Santiago Pastorino (May 01 2020 at 12:26, on Zulip):

but then it seems the only thing we are able to do right now?

oli (May 01 2020 at 12:27, on Zulip):

yes, I believe so

Santiago Pastorino (May 01 2020 at 12:32, on Zulip):

#71747

Santiago Pastorino (May 01 2020 at 12:32, on Zulip):

ui tests are :+1:, let's see what ci says

Last update: May 29 2020 at 17:30UTC