Revert "Remove checked_add in Layout::repeat"

[shahn]

This fixes a a segfault in safe code, a stable regression. Reported in #69225.

This reverts commit a983e05.

[rust-highfive]

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

[shahn]

I don't know how to add a test that expects to segfault here. Generally some mentoring would be appreciated if necessary. Thanks!

[Centril]

To fix the CI problem, run ./x.py fmt locally (and then commit --amend into the first commit).

[lqd]

To add a test, you could add a file in the src/test/run-fail folder, using the minimized program in the issue, with the "error pattern" checking that the program panics with the expected message (and the expected opt level flag).

As @Mark-Simulacrum said, some builders on CI should be built without debug-assertions, and catch this.

edit: or where @Centril pointed to :)

[Centril]

(Instead of src/test/run-fail/, it is preferable to add the new test to src/test/ui/ and add a comment // run-fail to the test file.)

[shahn]

To fix the CI problem, run ./x.py fmt locally (and then commit --amend into the first commit).

I am trying that, but I get an error:

Updating only changed submodules
Submodules updated in 0.02 seconds
    Finished dev [unoptimized] target(s) in 0.08s
error: expected expression, found reserved keyword `try`
  --> /home/shahn/coding/git/rust/cargo/src/bin/cargo.rs:85:28
   |
85 |         let args: Vec<_> = try!(env::args_os()
   |                            ^^^ expected expression

Running `"/home/shahn/coding/git/rust/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/home/shahn/coding/git/rust" "--edition" "2018" "--unstable-features" "--skip-children" "/home/shahn/coding/git/rust/cargo/src/bin/cargo.rs"` failed.
If you're running `tidy`, try again with `--bless` flag. Or, you just want to format code, run `./x.py fmt` instead.
failed to run: /home/shahn/coding/git/rust/build/bootstrap/debug/bootstrap fmt
Build completed unsuccessfully in 0:00:03

[lqd]

Use this (possibly tweak the amount of indents at the start of the lines if need be)

        let padded_size = self
            .size()
            .checked_add(self.padding_needed_for(self.align()))
            .ok_or(LayoutErr { private: () })?;
        let alloc_size = padded_size.checked_mul(n).ok_or(LayoutErr { private: () })?;

[shahn]

I pushed a new version but I made some kind of mistake with the ui test. It passes on master and on this PR for me locally...

[lqd]

Note the let alloc_size = ... statement needs to be formatted on a single line.

For the test, I think it's because the miscompilation only happens with the specific opt-level, try it with // compile-flags: -C opt-level=3

[shahn]

For the test, I think it's because the miscompilation only happens with the specific opt-level, try it with // compile-flags: -C opt-level=3

Unfortunately, that also didn't work for me locally. Hm.

[lqd]

Does it fail because it's asking for a .stderr file ? If so, re-run your test with --bless.
If not, what error are you seeing ?

[shahn]

I am not seeing any error, whether I run the test on an unmodified master or on my branch. That's my problem - I think it must error on master with just the test applied, but somehow it doesn't

[lqd]

To make sure I understand:

• you built master, and then manually built and run the code in the issue, reproducing the segfault
• you reverted the "checked_add" commit (this PR), and this time, manually building and running the code in the issue didn't segfault but panicked with an index out of bounds
• but, when switching from "manually building + running the code in the issue", to a ui run-fail test, ./x.py test doesn't error ?

(those first 2 steps required me to have debug-assertions turned off to reproduce the issue)

(// error-pattern: can be used to check for the panic message the test expects, which will be useful when we figure out why you're not getting an error)

[shahn]

To make sure I understand:

• you built master, and then manually built and run the code in the issue, reproducing the segfault

yes

• you reverted the "checked_add" commit (this PR), and this time, manually building the code in the issue didn't segfault but panicked with an index out of bounds

yes

• but, when switching from "manually building + running the code in the issue", to a ui run-fail test, ./x.py test doesn't error ?

When running tests on master with only the test case from this PR added but no other changes, the test suite (./x.py test --stage 1 src/test/ui) passes. It also passes when running tests on this PR.

[lqd]

In case a segfault is considered a valid run-fail test, can you try it with something like // error-pattern: index out of bounds: the len is 0 but the index is 16777216 ? The segfaulting master can't produce this panic message, this has to fail :)

[shahn]

Indeed it does fail now on master but passes with this branch! Very sincere thanks for all your help lqd and Centril!

[Mark-Simulacrum]

I am happy with this PR, and I think we want to get this into nightly fairly quickly, especially as we contemplate beta and stable backports (with a likely point release around the corner).

@bors r+ p=1

But feel free to roll this up as well.

[bors]

📌 Commit bdb1192510f4c765c4f89a9dd382319a35472a80 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 3e17d19 with merge a2fb0c2...

[bors]

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum,lqd
Pushing a2fb0c2 to master...

[pnkfelix]

discussed in T-compiler meeting. accepted for beta-backport

[pnkfelix]

discussed in T-compiler meeting. accepted for stable backport

[RalfJung]

Given that we have #69450 as a proper fix, is someone taking care of reverting this revert?

[lqd]

Yes, I'll do it