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

Changes the type `mir::Mir` into `mir::Body` #60928

Merged
merged 3 commits into from May 29, 2019

Conversation

Projects
None yet
5 participants
@TheSirC
Copy link

commented May 17, 2019

Fixes part 1 of #60229 (previously attempted in #60242).

I stumbled upon the issue and it seems that the previous attempt at solving it was not merged. This is a second try more up-to-date.

The commit should have changed comments as well.
At the time of writting, it passes the tidy and check tool.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@@ -71,7 +71,7 @@ macro_rules! make_mir_visitor {
// Override these, and call `self.super_xxx` to revert back to the
// default behavior.

fn visit_mir(&mut self, mir: & $($mutability)? Mir<'tcx>) {
fn visit_mir(&mut self, mir: & $($mutability)? Body<'tcx>) {

This comment has been minimized.

Copy link
@eddyb

eddyb May 22, 2019

Member

This and super_mir should be renamed to visit_body, and super_body, respectively.

This comment has been minimized.

Copy link
@TheSirC

TheSirC May 22, 2019

Author

Rodger that. Done in the last force push.

Ref(&'mir String),
Owned(Rc<String>),
}

impl<'mir> CachedMir<'mir> {
impl<'mir> CachedBody<'mir> {

This comment has been minimized.

Copy link
@eddyb

eddyb May 22, 2019

Member

No need to change this test - alternatively, CachedMir can be renamed to Cached, if you want.
(Since this is a reduction, where Mir was replaced with String)

This comment has been minimized.

Copy link
@TheSirC

TheSirC May 22, 2019

Author

Well received. Renamed to Cached then !

@eddyb
Copy link
Member

left a comment

r=me after a rebase and with nits fixed

@TheSirC TheSirC force-pushed the TheSirC:fix/60229 branch from 82b658a to c26eaaa May 22, 2019

@TheSirC

This comment has been minimized.

Copy link
Author

commented May 22, 2019

@eddyb I rebased the branch with the modifications you asked to one commit.

@eddyb

eddyb approved these changes May 23, 2019

@eddyb

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@TheSirC You still need to rebase on latest master, before I can approve the PR.

@TheSirC TheSirC force-pushed the TheSirC:fix/60229 branch from c26eaaa to 4bcd5fb May 24, 2019

@TheSirC

This comment has been minimized.

Copy link
Author

commented May 24, 2019

Sorry for the back-and-forth, just rebased. I will try to be reactive in case another one is needed.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (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.
travis_time:end:0e00a7aa:start=1558688467617550443,finish=1558688470662037256,duration=3044486813
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:15:47]    Compiling rustc_allocator v0.0.0 (/checkout/src/librustc_allocator)
[00:15:49] error[E0433]: failed to resolve: use of undeclared type or module `AggregateKind`
[00:15:49]    --> src/librustc_mir/transform/const_prop.rs:537:38
[00:15:49]     |
[00:15:49] 537 |                             Box::new(AggregateKind::Tuple),
[00:15:49]     |                                      ^^^^^^^^^^^^^ use of undeclared type or module `AggregateKind`
[00:16:03] error: aborting due to previous error
[00:16:03] 
[00:16:03] For more information about this error, try `rustc --explain E0433`.
[00:16:03] error: Could not compile `rustc_mir`.
---
travis_time:end:16f2f0fa:start=1558689610583775473,finish=1558689610589161448,duration=5385975
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:2de6ab26
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:07b8df90
travis_time:start:07b8df90
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:12271400
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@TheSirC TheSirC force-pushed the TheSirC:fix/60229 branch from 4bcd5fb to 6d14e73 May 24, 2019

@TheSirC

This comment has been minimized.

Copy link
Author

commented May 24, 2019

Obviously in the rebase I messed up an import; normally fixed now. Let's see what Travis has to say !

@eddyb

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

📌 Commit 6d14e73 has been approved by eddyb

Centril added a commit to Centril/rust that referenced this pull request May 25, 2019

Rollup merge of rust-lang#60928 - TheSirC:fix/60229, r=eddyb
Changes the type `mir::Mir` into `mir::Body`

Fixes part 1 of rust-lang#60229 (previously attempted in rust-lang#60242).

I stumbled upon the issue and it seems that the previous attempt at solving it was not merged. This is a second try more up-to-date.

The commit should have changed comments as well.
At the time of writting, it passes the tidy and check tool.

Centril added a commit to Centril/rust that referenced this pull request May 25, 2019

Rollup merge of rust-lang#61153 - Centril:rollup-b9qpiez, r=Centril
Rollup of 5 pull requests

Successful merges:

 - rust-lang#60928 (Changes the type `mir::Mir` into `mir::Body`)
 - rust-lang#61035 (Avoid more symbol interning)
 - rust-lang#61036 (PGO - Add a smoketest for combining PGO with cross-language LTO.)
 - rust-lang#61077 (Don't arena-allocate static symbols.)
 - rust-lang#61080 (Ship profiler with windows-gnu)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

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

@TheSirC

This comment has been minimized.

Copy link
Author

commented May 25, 2019

Should I rebase again ?

@TheSirC TheSirC force-pushed the TheSirC:fix/60229 branch from 6d14e73 to d5f7181 May 25, 2019

@TheSirC

This comment has been minimized.

Copy link
Author

commented May 25, 2019

Rebased to fix conflicts. Tests were passing locally.

@eddyb

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

📌 Commit d5f7181 has been approved by eddyb

@eddyb

This comment has been minimized.

Copy link
Member

commented May 28, 2019

This is very weird (I'll start investigating myself, just to speed things up):

[00:35:05] thread 'rustc' panicked at 'attempt to subtract with overflow', src/libsyntax/source_map.rs:769:41
@eddyb

This comment was marked as resolved.

Copy link
Member

commented May 28, 2019

I found a few more things to rename, btw (in comments/strings), if you want to do them:
(if not, they can be done in later PRs)

  • these should say mir::Body (with backticks around it):
src/librustc/query/mod.rs
91:        /// Maps DefId's that have an associated Mir to the result

src/librustc/mir/visit.rs
264:                // than a for-loop, to avoid calling Mir::invalidate for

src/librustc_mir/transform/mod.rs
103:/// Where a specific Mir comes from.

src/librustc_mir/build/mod.rs
167:        // Convert the Mir to global types.
246:            // Convert the Mir to global types.

src/librustc/mir/interpret/mod.rs
50:    /// The index for promoted globals within their function's `Mir`.

src/librustc_mir/hair/cx/mod.rs
47:    /// See field with the same name on `Mir`.
  • these should be "MIR" instead of "Mir".
src/librustc_mir/transform/qualify_consts.rs
1434:        tcx.sess.delay_span_bug(mir.span, "mir_const_qualif: Mir had errors");
1450:            tcx.sess.delay_span_bug(mir.span, "QualifyAndPromoteConstants: Mir had errors");
@@ -1,4 +1,4 @@
use rustc::mir::{BasicBlock, Location, Mir};
use rustc::mir::{BasicBlock, Location, Body};

This comment has been minimized.

Copy link
@eddyb

eddyb May 28, 2019

Member

I don't remember if @oli-obk wanted to leave this for another PR (or maybe I never asked for this before), but IMO Body shouldn't be referred to as anything other than mir::Body outside of the module it's defined in.

Show resolved Hide resolved src/librustc/ty/steal.rs Outdated
@eddyb

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Oh wow, the failure has nothing to do with this PR, it just accidentally got triggered:

let local_begin = self.lookup_byte_offset(sp.lo());
let local_end = self.lookup_byte_offset(sp.hi());
debug!("find_width_of_character_at_span: local_begin=`{:?}`, local_end=`{:?}`",
local_begin, local_end);
let start_index = local_begin.pos.to_usize();
let end_index = local_end.pos.to_usize();

That code is missing a check that local_begin and local_end actually are in the same SourceFile, like this (from a different function):

let local_begin = self.lookup_byte_offset(sp.lo());
let local_end = self.lookup_byte_offset(sp.hi());
if local_begin.sf.start_pos != local_end.sf.start_pos {

Macros are involved, somehow causing a span to end up with its two endpoints in different files. That might otherwise result in ICEs, if not for them being at the exact same byte offset (within their respective files) which the algorithm doesn't account for.
cc @davidtwco @petrochenkov

@eddyb

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@TheSirC I've pushed two commits - if you need to rebase, please pull first (and don't squash the syntax: ... commit away, since it's a separate bugfix).

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

📌 Commit 95013e6 has been approved by eddyb

Centril added a commit to Centril/rust that referenced this pull request May 28, 2019

Rollup merge of rust-lang#60928 - TheSirC:fix/60229, r=eddyb
Changes the type `mir::Mir` into `mir::Body`

Fixes part 1 of rust-lang#60229 (previously attempted in rust-lang#60242).

I stumbled upon the issue and it seems that the previous attempt at solving it was not merged. This is a second try more up-to-date.

The commit should have changed comments as well.
At the time of writting, it passes the tidy and check tool.

matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request May 28, 2019

bors added a commit that referenced this pull request May 28, 2019

Auto merge of #61296 - Centril:rollup-5ad68b0, r=Centril
Rollup of 9 pull requests

Successful merges:

 - #60742 (Allow const parameters in array sizes to be unified)
 - #60756 (Add better tests for hidden lifetimes in impl trait)
 - #60928 (Changes the type `mir::Mir` into `mir::Body`)
 - #61024 (tests: Centralize proc macros commonly used for testing)
 - #61157 (BufReader: In Seek impl, remove extra discard_buffer call)
 - #61195 (Special-case `.llvm` in mangler)
 - #61202 (Print PermissionExt::mode() in octal in Documentation Examples)
 - #61259 (Mailmap fixes)
 - #61273 (mention that MaybeUninit is a bit like Option)

Failed merges:

r? @ghost

@bors bors merged commit 95013e6 into rust-lang:master May 29, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

bors added a commit to rust-lang/rust-clippy that referenced this pull request May 29, 2019

bors added a commit to rust-lang/rust-clippy that referenced this pull request May 29, 2019

bors added a commit to rust-lang/rust-clippy that referenced this pull request May 29, 2019

bors added a commit to rust-lang/rust-clippy that referenced this pull request May 29, 2019

bors added a commit to rust-lang/rust-clippy that referenced this pull request May 29, 2019

bors added a commit to rust-lang/rust-clippy that referenced this pull request May 29, 2019

bors added a commit to rust-lang/rust-clippy that referenced this pull request May 29, 2019

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 29, 2019

submodules: update clippy from fb33fad to 018fa30
Changes:
````
rustup rust-lang#60928
rustup rust-lang#61164 (which is included in rust-lang#61274)
````

bors added a commit that referenced this pull request May 30, 2019

Auto merge of #61298 - matthiaskrgr:submodule_upd, r=Manishearth
submodules: update clippy from fb33fad to 018fa30

Changes:
````
rustup #60928
rustup #61164 (which is included in #61274)
````

Fixes #61287

r? @oli-obk

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 30, 2019

submodules: update clippy from fb33fad to d2f5122
Changes:
````
Rustup to rust-lang#61203
rustup rust-lang#60928
rustup rust-lang#61164 (which is included in rust-lang#61274)
````

bors added a commit that referenced this pull request May 31, 2019

Auto merge of #61298 - matthiaskrgr:submodule_upd, r=oli-obk
submodules: update clippy from fb33fad to d2f5122

Changes:
````
Rustup to #61203
rustup #60928
rustup #61164 (which is included in #61274)
````

Fixes #61287

r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.