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

Revert "Remove checked_add in Layout::repeat" #69241

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

shahn
Copy link
Contributor

@shahn shahn commented Feb 17, 2020

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

This reverts commit a983e05.

@rust-highfive
Copy link
Collaborator

r? @KodrAus

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2020
@shahn
Copy link
Contributor Author

shahn commented Feb 17, 2020

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

src/libcore/alloc.rs Outdated Show resolved Hide resolved
@Centril Centril added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 17, 2020
@Centril
Copy link
Contributor

Centril commented Feb 17, 2020

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

@lqd
Copy link
Member

lqd commented Feb 17, 2020

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
Copy link
Contributor

Centril commented Feb 17, 2020

(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
Copy link
Contributor Author

shahn commented Feb 17, 2020

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

@rust-highfive

This comment has been minimized.

@lqd
Copy link
Member

lqd commented Feb 17, 2020

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
Copy link
Contributor Author

shahn commented Feb 17, 2020

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
Copy link
Member

lqd commented Feb 17, 2020

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
Copy link
Contributor Author

shahn commented Feb 17, 2020

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
Copy link
Member

lqd commented Feb 17, 2020

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
Copy link
Contributor Author

shahn commented Feb 17, 2020

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
Copy link
Member

lqd commented Feb 17, 2020

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
Copy link
Contributor Author

shahn commented Feb 17, 2020

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
Copy link
Member

lqd commented Feb 17, 2020

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
Copy link
Contributor Author

shahn commented Feb 18, 2020

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

@Mark-Simulacrum
Copy link
Member

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
Copy link
Contributor

bors commented Feb 18, 2020

📌 Commit bdb1192510f4c765c4f89a9dd382319a35472a80 has been approved by Mark-Simulacrum

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 18, 2020
@bors
Copy link
Contributor

bors commented Feb 19, 2020

⌛ Testing commit 3e17d19 with merge a2fb0c2...

bors added a commit that referenced this pull request Feb 19, 2020
Revert "Remove `checked_add` in `Layout::repeat`"

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

This reverts commit a983e05.
@bors
Copy link
Contributor

bors commented Feb 19, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 19, 2020
@bors bors merged commit 3e17d19 into rust-lang:master Feb 19, 2020
@shahn shahn deleted the checked_add_revert branch February 19, 2020 05:14
@pnkfelix
Copy link
Member

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

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 20, 2020
@pnkfelix
Copy link
Member

discussed in T-compiler meeting. accepted for stable backport

@pnkfelix pnkfelix added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Feb 20, 2020
@Mark-Simulacrum Mark-Simulacrum removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Feb 21, 2020
This was referenced Feb 21, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 21, 2020
bors added a commit that referenced this pull request Feb 22, 2020
[stable] 1.41.1 release

This backports the following PRs:
 * Revert "Remove `checked_add` in `Layout::repeat`" #69241
 * Do not ICE when encountering `yield` inside `async` block #69175
 * Correct ICE caused by macros generating invalid spans. #68611
 * Changelog: Demonstrate final build-override syntax #68603
 * Resolve long compile times when evaluating always valid constants #67667
 * Fix MIR typeck soundness holes #69145

This also includes a commit which rustfmt's files which the latter commits touched (and perhaps a bit more) to make rebasing the PRs go more smoothly (thankfully, this should be the last time we need to do so).

I have removed stable-nominated tags from PRs successfully backported.
bors added a commit that referenced this pull request Feb 24, 2020
[stable] 1.41.1 release

This backports the following PRs:
 * Revert "Remove `checked_add` in `Layout::repeat`" #69241
 * Do not ICE when encountering `yield` inside `async` block #69175
 * Correct ICE caused by macros generating invalid spans. #68611
 * Changelog: Demonstrate final build-override syntax #68603
 * Resolve long compile times when evaluating always valid constants #67667
 * Fix MIR typeck soundness holes #69145

This also includes a commit which rustfmt's files which the latter commits touched (and perhaps a bit more) to make rebasing the PRs go more smoothly (thankfully, this should be the last time we need to do so).

I have removed stable-nominated tags from PRs successfully backported.
bors added a commit that referenced this pull request Feb 24, 2020
[beta] beta backports

This backports the following PRs:

* Revert "Remove `checked_add` in `Layout::repeat`" #69241
* Do not ICE when encountering `yield` inside `async` block #69175
* Fix MIR typeck soundness holes #69145
* Fix extra subslice lowering #69128
* Correct ICE caused by macros generating invalid spans. #68611
* Make conflicting_repr_hints a deny-by-default c-future-compat lint #68586
bors added a commit that referenced this pull request Feb 25, 2020
Cherry-pick the LLVM fix for #69225

An additional reproducer was provided in #69225 -- the new testcase here -- which still crashes even after #69241 reverted #67174. Now this pull request updates LLVM with the cherry-picked reversion of its own. This is also going to stable in #69444.

I have not tried to reapply #67174 yet -- cc @kraai @shahn
@RalfJung
Copy link
Member

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

@lqd
Copy link
Member

lqd commented Feb 28, 2020

Yes, I'll do it

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 2, 2020
Unrevert "Remove `checked_add` in `Layout::repeat`"

This reapplies @kraai's original `libcore::alloc::Layout::repeat` change from rust-lang#67174 which was temporarily reverted in rust-lang#69241. Now that the proper LLVM fix has been cherry-picked, we can unrevert the revert.

This change was originally reviewed by @hanna-kruppe on the initial PR.

cc @RalfJung
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. stable-accepted Accepted for backporting to the compiler in the stable channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants