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

Assert that locals have storage when used #78147

Merged
merged 1 commit into from
Nov 1, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Oct 20, 2020

The validator in visit_local asserts that local has a stroage when used,
but visit_local is never called so validation is ineffective.

Use super_statement and super_terminator to ensure that locals are visited.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Oct 20, 2020
@lcnr
Copy link
Contributor

lcnr commented Oct 20, 2020

LGTM

r? @jonas-schievink for a final review, am not too familiar with this

@jonas-schievink
Copy link
Contributor

Hmm, I was sure it caught a bug when I implemented this, weird

@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 20, 2020

I wondered about that too; it seems to me that was use in debuginfo, until that part was fixed.

This will need a new perf run. It also just found a bug ...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2020
…cation, r=wesleywiser

Disable MatchBranchSimplification

This optimization can result in unsoundness, because it introduces
additional uses of a place holding the discriminant value without
ensuring that it is valid to do so.

Found by validation from rust-lang#77369 / rust-lang#78147.
@tmiasko tmiasko force-pushed the validate-storage branch 4 times, most recently from c474f0b to 87394e1 Compare October 20, 2020 20:19
@rust-lang rust-lang deleted a comment from ddd0kl Oct 20, 2020
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 20, 2020

⌛ Trying commit 87394e1b0a60b121d2ffefdc020d423517b181a0 with merge 2c2a5c65b82ec277ab9c7904b56e1a506e1f25aa...

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-linux of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
   Compiling tokio-codec v0.1.2
   Compiling jsonrpc-core v14.2.0
   Compiling lsp-types v0.60.0
   Compiling addr2line v0.13.0
error: internal compiler error: broken MIR in Item(WithOptConstParam { did: DefId(0:317 ~ mio[c9d7]::poll::{impl#21}::update), const_param_did: None }) (end of phase Optimization) at bb2[20]:
use of local _12, which has no storage here
    --> /cargo/registry/src/github.com-1ecc6299db9ec823/mio-0.6.22/src/poll.rs:1888:21
     |
1888 |         let other = *other;
     |
     |
     = note: delayed at compiler/rustc_mir/src/transform/validate.rs:167:36

thread 'rustc' panicked at 'no errors encountered even though `delay_span_bug` issued', compiler/rustc_errors/src/lib.rs:961:13

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.
note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.49.0-nightly (2c2a5c65b 2020-10-20) running on x86_64-unknown-linux-gnu
note: compiler flags: -Z macro-backtrace -Z binary-dep-depinfo -C opt-level=3 -C embed-bitcode=no -C debuginfo=0 -C linker=clang -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type lib

note: some of the compiler flags provided by cargo are hidden

---
   Compiling chrono v0.4.15
   Compiling mio-extras v2.0.6
   Compiling notify v5.0.0-pre.3
   Compiling vfs-notify v0.0.0 (/checkout/src/tools/rust-analyzer/crates/vfs-notify)
error: internal compiler error: broken MIR in Item(WithOptConstParam { did: DefId(0:317 ~ mio[a47d]::poll::{impl#21}::update), const_param_did: None }) (end of phase Optimization) at bb2[20]:
use of local _12, which has no storage here
    --> /cargo/registry/src/github.com-1ecc6299db9ec823/mio-0.6.22/src/poll.rs:1888:21
     |
1888 |         let other = *other;
     |
     |
     = note: delayed at compiler/rustc_mir/src/transform/validate.rs:167:36

thread 'rustc' panicked at 'no errors encountered even though `delay_span_bug` issued', compiler/rustc_errors/src/lib.rs:961:13

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.
note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.49.0-nightly (2c2a5c65b 2020-10-20) running on x86_64-unknown-linux-gnu
note: compiler flags: -Z macro-backtrace -Z binary-dep-depinfo -C opt-level=3 -C embed-bitcode=no -C debuginfo=0 -C linker=clang -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type lib

note: some of the compiler flags provided by cargo are hidden

---
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/rust-analyzer/crates/rust-analyzer/Cargo.toml" "--message-format" "json-render-diagnostics"
expected success, got: exit code: 101
thread 'main' panicked at 'rust-analyzer always builds', src/bootstrap/dist.rs:1399:14
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --host x86_64-unknown-linux-gnu --target x86_64-unknown-linux-gnu --include-default-paths src/tools/build-manifest
Build completed unsuccessfully in 0:33:56
== clock drift check ==
  local time: Tue Oct 20 23:41:17 UTC 2020
  local time: Tue Oct 20 23:41:17 UTC 2020
  network time: Tue, 20 Oct 2020 01:04:22 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (4762) (python)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Oct 20, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2020
@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 21, 2020

I cherry picked a fix. We can try running perf again. Thanks!

@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 21, 2020

⌛ Trying commit e3630a9a6bd3e03efb6c5f5a313adc48c1c7fb4b with merge 5f06fab28613e7f3ac319440e305f7b5909e1b0e...

@bors
Copy link
Contributor

bors commented Oct 22, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 22, 2020
@bors
Copy link
Contributor

bors commented Oct 25, 2020

☔ The latest upstream changes (presumably #78350) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors
Copy link
Contributor

bors commented Oct 31, 2020

☔ The latest upstream changes (presumably #78594) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

The validator in visit_local asserts that local has a stroage when used,
but visit_local is never called so validation is ineffective.

Use super_statement and super_terminator to ensure that locals are visited.
@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 31, 2020

Rebased.

@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 31, 2020

⌛ Trying commit 3b7157d with merge 9885b20e857447b481b8b2e326294e9bb8d31145...

@bors
Copy link
Contributor

bors commented Oct 31, 2020

☀️ Try build successful - checks-actions
Build commit: 9885b20e857447b481b8b2e326294e9bb8d31145 (9885b20e857447b481b8b2e326294e9bb8d31145)

@rust-timer
Copy link
Collaborator

Queued 9885b20e857447b481b8b2e326294e9bb8d31145 with parent 4f7612a, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (9885b20e857447b481b8b2e326294e9bb8d31145): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@jonas-schievink
Copy link
Contributor

@bors r+ rollup-

@bors
Copy link
Contributor

bors commented Oct 31, 2020

📌 Commit 3b7157d has been approved by jonas-schievink

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2020
@jonas-schievink
Copy link
Contributor

@bors rollup=never

There does seem to be a slight regression in tuple-stress, so let's not roll it up.

@bors
Copy link
Contributor

bors commented Nov 1, 2020

⌛ Testing commit 3b7157d with merge a6403b0...

@bors
Copy link
Contributor

bors commented Nov 1, 2020

☀️ Test successful - checks-actions
Approved by: jonas-schievink
Pushing a6403b0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 1, 2020
@bors bors merged commit a6403b0 into rust-lang:master Nov 1, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 1, 2020
@tmiasko tmiasko deleted the validate-storage branch November 1, 2020 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants