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

Duplicated errors on UB inside a promoted #61821

Closed
estebank opened this issue Jun 14, 2019 · 13 comments · Fixed by #80579
Closed

Duplicated errors on UB inside a promoted #61821

estebank opened this issue Jun 14, 2019 · 13 comments · Fixed by #80579
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@estebank
Copy link
Contributor

estebank commented Jun 14, 2019

The following output should only emit the error once:

#![warn(const_err)]

fn main() {
    &{ [1, 2, 3][4] };
}
error: index out of bounds: the len is 3 but the index is 4
  --> $DIR/array-literal-index-oob.rs:2:7
   |
LL |     &{[1, 2, 3][4]};
   |       ^^^^^^^^^^^^
   |
   = note: `#[deny(const_err)]` on by default

error: reaching this expression at runtime will panic or abort
  --> $DIR/array-literal-index-oob.rs:2:7
   |
LL |     &{[1, 2, 3][4]};
   |     --^^^^^^^^^^^^-
   |       |
   |       index out of bounds: the len is 3 but the index is 4

error: aborting due to 2 previous errors

Follow up to #61598

@estebank estebank added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-diagnostics Area: Messages for errors, warnings, and lints P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-eval Area: Constant evaluation (MIR interpretation) labels Jun 14, 2019
@estebank

This comment has been minimized.

@estebank estebank changed the title 3 duplicated errors on index out of bound const evaluation instead Duplicated errors on index out of bound const evaluation instead Sep 1, 2019
@estebank estebank changed the title Duplicated errors on index out of bound const evaluation instead Duplicated errors on index out of bound const evaluation Sep 8, 2019
@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2019

One of the two errors is coming from evaluating the Assert MIR terminator that does the bounds-check and panics if it fails. The other message comes from actually evaluating the place[4] even though the index is out-of-bounds.

What is surprising here is that the evaluation of the place expression even happens even though that should be dead code. @oli-obk @wesleywiser any idea?

(This is not fixed by #66927.)

@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2019

What is surprising here is that the evaluation of the place expression even happens even though that should be dead code.

The reason is that the MIR of the promoted actually looks like this:

promoted[0] in  main: i32 = {
    let mut _0: i32;                     // return place in scope 0 at src/main.rs:3:5: 3:20
    let mut _1: i32;                     // in scope 0 at src/main.rs:3:6: 3:20
    let mut _2: [i32; 3];                // in scope 0 at src/main.rs:3:7: 3:16
    let mut _3: usize;                   // in scope 0 at src/main.rs:3:17: 3:18

    bb0: {
        _2 = [const 1i32, const 2i32, const 3i32]; // bb0[0]: scope 0 at src/main.rs:3:7: 3:16
        _3 = const 4usize;               // bb0[1]: scope 0 at src/main.rs:3:17: 3:18
        _1 = _2[_3];                     // bb0[2]: scope 0 at src/main.rs:3:7: 3:19
        _0 = move _1;                    // bb0[3]: scope 0 at src/main.rs:3:5: 3:20
        return;                          // bb0[4]: scope 0 at src/main.rs:3:5: 3:20
    }
}

Notice the unchecked array access. That's how this is not dead code.

I assume the other error comes from ConstProp (likely of the Assert terminator), but I am not sure.

@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2019

Cc @ecstatic-morse @eddyb because promotion is involved

@eddyb
Copy link
Member

eddyb commented Dec 2, 2019

I don't think const_err should include promoteds, since any checks for anything they might do are still in the runtime code (doing anything else would require something much more complex than what we have today).

But also, if we constant-fold the assert, we can know that using the promoted is dead code (I guess this doesn't apply to codegen generating those warnings instead of constant folding).

@RalfJung
Copy link
Member

RalfJung commented Feb 10, 2020

@eddyb so basically, const_prop should cease to emit warnings when it is working inside a promoted? Or maybe it shouldn't run there at all?
Cc @wesleywiser

@eddyb
Copy link
Member

eddyb commented Feb 10, 2020

You could still have warnings inside a promoted that are not from things covered by Asserts, but other odd conditions const_prop checks.

Basically "this expression will panic at runtime" type warnings shouldn't be emitted from a promoted, others might be fine.

@RalfJung
Copy link
Member

RalfJung commented Feb 11, 2020

Actually, I think the first ("error: index out of bounds") is not a warning in any meaningful way... trying to evaluate that promoted fails with a hard const-error ("UB: OOB array access") and produces no result. I does not seem wise to me to not report such hard errors.

I am pretty sure the "reaching this expression at runtime will panic or abort" part is not from inside the promoted. That one gets raised by const-prop when partially evaluating the Assert terminator that is left in the fn.

So @eddyb I don't think any of your proposals so far actually solves this problem. Or do you suggest we ignore UB in promoteds?

@RalfJung
Copy link
Member

Also, in latest master a third warning crept in again:

warning: erroneous constant used
  --> $DIR/array-literal-index-oob.rs:7:5
   |
LL |     &{ [1, 2, 3][4] };
   |     ^^^^^^^^^^^^^^^^^ referenced constant has errors

The third error was added by #67000

@RalfJung
Copy link
Member

RalfJung commented Feb 11, 2020

FWIW, this affects not just array indexing but also div-by-zero. At least the two are consistent. ;)

@RalfJung RalfJung changed the title Duplicated errors on index out of bound const evaluation Duplicated errors on UB inside a promoted Feb 11, 2020
@eddyb
Copy link
Member

eddyb commented Feb 11, 2020

Or do you suggest we ignore UB in promoteds?

If we want to be clever we could only check promoteds that are still reachable after constant folding + simplify_cfg.
Because the Assert guarding the use of the erroring promoted would have become unconditional.

Actually, do we have an unconditional Panic terminator?

@RalfJung RalfJung added the D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. label Feb 15, 2020
Centril added a commit to Centril/rust that referenced this issue Feb 20, 2020
Unify and improve const-prop lints

Add a single helper method for all lints emitted by const-prop, and make that lint different from the CTFE `const_err` lint. Also consistently check overflow on *arithmetic*, not on the assertion, to make behavior the same for debug and release builds.

See [this summary comment](rust-lang#69185 (comment)) for details and the latest status.

In terms of lint formatting, I went for what seems to be the better style: have a general message above the code, and then a specific message at the span:
```
error: this arithmetic operation will overflow
  --> $DIR/const-err2.rs:21:18
   |
LL |     let a_i128 = -std::i128::MIN;
   |                  ^^^^^^^^^^^^^^^ attempt to negate with overflow
```
We could also just have the specific message above and no text at the span if that is preferred.

I also converted some of the existing tests to use compiletest revisions, so that the same test can check a bunch of different compile flags.

Fixes rust-lang#69020.
Helps with rust-lang#69021: debug/release are now consistent, but the assoc-const test in that issue still fails (there is a FIXME in the PR for this). The reason seems to be that const-prop notices the assoc const in `T::N << 42` and does not even bother calling `const_prop` on that operation.
Has no effect on rust-lang#61821; the duplication there has entirely different reasons.
@steveklabnik
Copy link
Member

Triage: no change

@RalfJung
Copy link
Member

#80579 will solve this by not promoting division and array indexing any more when they could fail.

@bors bors closed this as completed in 4d0dd02 Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants