Don't recurse into allocations, use a global table instead #49833

oli-obk · 2018-04-10T08:17:09Z

TimNN · 2018-04-10T08:25:15Z

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.

Resolving deltas: 100% (613484/613484), completed with 4872 local objects.
---
[00:00:47] configure: rust.quiet-tests     := True
---
[00:04:31] tidy error: /checkout/src/librustc/ty/maps/on_disk_cache.rs:633: line longer than 100 chars
[00:04:32] some tidy checks failed
[00:04:32]
[00:04:32]
[00:04:32] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:32] expected success, got: exit code: 1
[00:04:32]
[00:04:32]
[00:04:32] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:32] Build completed unsuccessfully in 0:01:46
[00:04:32] make: *** [tidy] Error 1
[00:04:32] Makefile:79: recipe for target 'tidy' failed

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)

oli-obk · 2018-04-10T08:37:01Z

src/librustc/ty/maps/on_disk_cache.rs

@@ -277,6 +280,31 @@ impl<'sess> OnDiskCache<'sess> {
                diagnostics_index
            };

+            let interpret_alloc_index = {


This exact code exists twice, once for on_disk_cache and once for rustc_metadata::encoder. Not sure how to dedup.

oli-obk · 2018-04-10T14:48:25Z

Ok, now this actually fixes #49663

cc @eddyb for e9ebc7f (minimal impl of indirectly referring to statics)

eddyb · 2018-04-10T15:30:25Z

LGTM

oli-obk · 2018-04-10T17:05:38Z

src/librustc_metadata/schema.rs

@@ -207,6 +207,7 @@ pub struct CrateRoot {
    pub impls: LazySeq<TraitImpls>,
    pub exported_symbols: EncodedExportedSymbols,
    pub wasm_custom_sections: LazySeq<DefIndex>,
+    pub interpret_alloc_index: Vec<u32>,


I'm seeing ICEs inside the deserialize impl for this field in 4 stage 2 run-pass tests with aux builds. I can't reproduce on stage 1 and Travis also takes it. Maybe there's some stale data where crates are loaded that were compiled without this field?

Also: should this be LazySeq?

If you have ignore_git set (which is the default now), incompatible metadata won't be detected. I think I had this issue with test auxiliary crates. And yes, please make it a LazySeq<u32>.

I changed it into a LazySeq, now I'm seeing the failures on stage 1, too. ignore_git doesn't change anything

The failure seems to be in the deserialization in the field after interpret_alloc_index.

TimNN · 2018-04-11T09:13:42Z

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.

Resolving deltas: 100% (613671/613671), completed with 4881 local objects.
---
[00:00:42] configure: rust.quiet-tests     := True
---
[00:23:16] thread 'main' panicked at 'assertion failed: last_min_end <= position', librustc_metadata/encoder.rs:268:17
[00:23:16] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:23:17] error: Could not compile `core`.
[00:23:17]
[00:23:17] Caused by:
[00:23:17]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name core libcore/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=64878136ec7adadb -C extra-filename=-64878136ec7adadb --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/release/deps` (exit code: 101)
[00:23:17] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:23:17] expected success, got: exit code: 101
[00:23:17] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1085:9
[00:23:17] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:23:17] travis_fold:end:stage1-std
[00:23:17] travis_time:end:stage1-std:start=1523437960816315660,finish=1523438017192265823,duration=56375950163
[00:23:17] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:23:17] Build completed unsuccessfully in 0:18:29
[00:23:17] Makefile:28: recipe for target 'all' failed
[00:23:17] make: *** [all] Error 1
---
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:2912f28a:start=1523438017746254813,finish=1523438017754102010,duration=7847197
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:18c7d788
$ dmesg | grep -i kill
[   10.128139] init: failsafe main process (1094) killed by TERM signal

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)

bors · 2018-04-12T19:12:51Z

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

michaelwoerister

Thanks, @oli-obk! Looks great.

I'm probably overlooking something: Where are the allocations for statics encoded?

michaelwoerister · 2018-04-13T08:21:50Z

src/librustc/mir/interpret/mod.rs

-    } else if let Some(did) = tcx.interpret_interner.get_corresponding_static_def_id(alloc_id) {
-        // extern "C" statics don't have allocations, just encode its def_id
-        EXTERN_STATIC_DISCRIMINANT.encode(encoder)?;
+    } else if let Some(did) = tcx.interpret_interner.get_static(alloc_id) {


Does this need to be moved up in order not to always hit the get_alloc branch?

I'd assume that the get_alloc branch is the most common. Also both get_alloc and get_static are just HashMap lookups.

michaelwoerister · 2018-04-13T08:47:02Z

src/librustc/ty/maps/on_disk_cache.rs

-                entry.insert(pos);
-                None
-            },
+        let index = if self.interpret_alloc_ids.insert(*alloc_id) {


Do we need both interpret_alloc_ids and interpret_allocs? Or could just use interpret_allocs.contains_key() instead of interpret_alloc_ids.contains()? You might also be able to use the entry API here, so that there is just one hashtable lookup.

That could work

michaelwoerister · 2018-04-13T09:10:22Z

src/librustc/mir/interpret/mod.rs

-        // extern "C" statics don't have allocations, just encode its def_id
-        EXTERN_STATIC_DISCRIMINANT.encode(encoder)?;
+    } else if let Some(did) = tcx.interpret_interner.get_static(alloc_id) {
+        // referring to statics doesn't need to know about their allocations, just hash the DefId


s/hash/encode ?

ha yea, this is actual copy paste

oli-obk

The AllocId for a static does not point to the actual static's Allocation anymore. Instead it works like function pointers, it's just an ID that you can use to get at the DefId of the static. To get at the Allocation of a static, you need to run the const_eval query.

oli-obk · 2018-04-13T11:20:31Z

src/librustc/mir/interpret/mod.rs

-        // extern "C" statics don't have allocations, just encode its def_id
-        EXTERN_STATIC_DISCRIMINANT.encode(encoder)?;
+    } else if let Some(did) = tcx.interpret_interner.get_static(alloc_id) {
+        // referring to statics doesn't need to know about their allocations, just hash the DefId


ha yea, this is actual copy paste

oli-obk · 2018-04-13T11:21:46Z

src/librustc/mir/interpret/mod.rs

-    } else if let Some(did) = tcx.interpret_interner.get_corresponding_static_def_id(alloc_id) {
-        // extern "C" statics don't have allocations, just encode its def_id
-        EXTERN_STATIC_DISCRIMINANT.encode(encoder)?;
+    } else if let Some(did) = tcx.interpret_interner.get_static(alloc_id) {


I'd assume that the get_alloc branch is the most common. Also both get_alloc and get_static are just HashMap lookups.

oli-obk · 2018-04-13T11:23:15Z

src/librustc/ty/maps/on_disk_cache.rs

-                entry.insert(pos);
-                None
-            },
+        let index = if self.interpret_alloc_ids.insert(*alloc_id) {


That could work

michaelwoerister · 2018-04-13T12:08:07Z

The AllocId for a static does not point to the actual static's Allocation anymore. Instead it works like function pointers, it's just an ID that you can use to get at the DefId of the static. To get at the Allocation of a static, you need to run the const_eval query.

OK, so the static has an AllocId with no corresponding Allocation? And const_eval() gives you a Value which might contain a MemoryPointer but the AllocId in there is not the same as the AllocId of the corresponding static?

oli-obk · 2018-04-13T12:13:19Z

Exactly. It's a little extreme, but forward compatible to allow statics to refer to the inside of other statics in the future. Also much harder to get wrong, because you can't accidentally get at the Allocation of a static.

static A: (i32, u32) = (42, 43);
static B: &'static u32 = &A.1;

michaelwoerister · 2018-04-13T12:26:57Z

OK, that makes sense to me then. It also explains why the ordering in specialized_encode_alloc_id() does not matter.

It would be great if you could try to get rid of the interpret_alloc_ids set. Otherwise this should be good to go :)

rust-highfive · 2018-04-13T17:20:14Z

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.

[00:00:46] configure: rust.quiet-tests     := True
---
[00:07:52] error[E0433]: failed to resolve. Use of undeclared type or module `Entry`
[00:07:52]    --> librustc/ty/maps/on_disk_cache.rs:842:13
[00:07:52]     |
[00:07:52] 842 |             Entry::Occupied(e) => e.get(),
[00:07:52]     |             ^^^^^ Use of undeclared type or module `Entry`
[00:07:52]
[00:07:52] error[E0433]: failed to resolve. Use of undeclared type or module `Entry`
[00:07:52]    --> librustc/ty/maps/on_disk_cache.rs:843:13
[00:07:52]     |
[00:07:52] 843 |             Entry::Vacant(e) => {
[00:07:52]     |             ^^^^^ Use of undeclared type or module `Entry`
[00:07:52]
[00:07:55] error: unused import: `FxHashSet`
[00:07:55]   --> librustc/ty/maps/on_disk_cache.rs:19:44
[00:07:55]    |
[00:07:55] 19 | use rustc_data_structures::fx::{FxHashMap, FxHashSet};
[00:07:55]    |                                            ^^^^^^^^^
[00:07:55]    |
[00:07:55]    = note: `-D unused-imports` implied by `-D warnings`
[00:07:55]
pos-9f3518d56a01456f.so --extern serialize=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libserialize-c04ded78717d5d67.so --extern serialize=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libserialize-c04ded78717d5d67.rlib --extern lazy_static=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/liblazy_static-2803512e03c19cf7.rlib --extern log=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/liblog-3e35efb9e5bc7a9a.rlib --extern rustc_const_math=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_const_math-895f7ddc4467bb8d.so --extern rustc_back=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_back-861dba9fe03aa669.so --extern bitflags=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libbitflags-9866e194db82a141.rlib --extern rustc_data_structures=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_data_structures-93cb1ddd29ab61a4.so --extern flate2=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libflate2-3c8223b0152f22a5.rlib --extern byteorder=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libbyteorder-ca61dfec7c40c4d4.rlib --extern tempdir=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libtempdir-65dde0349e75e965.rlib --extern proc_macro=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libproc_macro-6e4119b5ec8457a3.so --extern fmt_macros=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libfmt_macros-3706e912fdb98df1.so --extern jobserver=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libjobserver-5bcc8c1ccd509892.rlib --extern arena=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libarena-f2778bd6cd1c5bdd.so --extern backtrace=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libbacktrace-109fed6e8125a798.rlib -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/build/backtrace-sys-8b35e3c2ea935fab/out/.libs -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/build/miniz-sys-63734d0048644b22/out` (exit code: 101)
[00:08:32] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:08:32] expected success, got: exit code: 101
[00:08:32] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1085:9
---
121748 ./obj/build/bootstrap/debug/incremental/bootstrap-351vorei3hhuv/s-f032dm7esi-tltjm4-3ijxj2oqwtm6x

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)

bors · 2018-04-14T09:07:26Z

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

oli-obk · 2018-04-14T10:26:35Z

Rebased and addressed all review comments

oli-obk · 2018-04-15T11:31:39Z

I got two "lgtm", so let's get this moving (beta needs this)

@bors r=michaelwoerister

bors · 2018-04-15T11:31:40Z

📌 Commit 7f7d4c3 has been approved by michaelwoerister

bors · 2018-04-15T12:32:21Z

⌛ Testing commit 7f7d4c3 with merge 7360d6d...

@michaelwoerister

…woerister Don't recurse into allocations, use a global table instead r? @michaelwoerister fixes #49663

bors · 2018-04-15T15:20:37Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 7360d6d to master...

pnkfelix · 2018-04-19T15:02:31Z

Marking as beta-accepted, with some reservations due to the size of the PR. But since there's already a bors run for a slew of beta backports that includes this currently in progress (due to a process failure), and @michaelwoerister says leaving this out will break incremental compilation....

[beta] backport various PRs original PRs: * #49949 (not yet merged at the time of writing) * #49947 (long running const eval error -> warning) * #49833 (static recursion) * #49876 (no clippy in stable rls) * #49904 (Work around LLVM debuginfo problem in librustc_driver. )

alexcrichton · 2018-04-20T20:23:02Z

Backported in #50027

rust-highfive assigned michaelwoerister Apr 10, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 10, 2018

oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 10, 2018

oli-obk force-pushed the incremental_miri_regression branch from 66dc729 to efbb574 Compare April 10, 2018 08:34

oli-obk commented Apr 10, 2018

View reviewed changes

oli-obk force-pushed the incremental_miri_regression branch from 8c4d438 to c10c9d3 Compare April 11, 2018 11:32

michaelwoerister reviewed Apr 13, 2018

View reviewed changes

oli-obk commented Apr 13, 2018

View reviewed changes

oli-obk force-pushed the incremental_miri_regression branch from c10c9d3 to 9fee6ec Compare April 13, 2018 16:50

oli-obk force-pushed the incremental_miri_regression branch from 9fee6ec to 2d00f2d Compare April 13, 2018 18:28

oli-obk added 6 commits April 14, 2018 12:21

Don't recurse into allocations, use a global table instead

a67ded0

Stop referring to statics' AllocIds directly

62c0501

Use LazySeq instead of Vec

6f251c2

Encode items before encoding the list of AllocIds

04b3ab6

Reduce the number of calls to cdata

748e71e

Get rid of redundant HashSet

7f7d4c3

oli-obk force-pushed the incremental_miri_regression branch from 2d00f2d to 7f7d4c3 Compare April 14, 2018 10:22

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 Apr 15, 2018

bors added a commit that referenced this pull request Apr 15, 2018

Auto merge of #49833 - oli-obk:incremental_miri_regression, r=michael…

7360d6d

…woerister Don't recurse into allocations, use a global table instead r? @michaelwoerister fixes #49663

bors merged commit 7f7d4c3 into rust-lang:master Apr 15, 2018

dwrensha added a commit to dwrensha/seer that referenced this pull request Apr 16, 2018

update for rust-lang/rust#49833

5da1a8d

oli-obk mentioned this pull request Apr 17, 2018

[beta] Incremental miri regression #50025

Closed

oli-obk removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 17, 2018

oli-obk mentioned this pull request Apr 17, 2018

[beta] backport various PRs #50027

Merged

pietroalbini added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 17, 2018

nagisa added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 19, 2018

nagisa added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 19, 2018

alexcrichton removed the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 20, 2018

alexcrichton added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Apr 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't recurse into allocations, use a global table instead #49833

Don't recurse into allocations, use a global table instead #49833

oli-obk commented Apr 10, 2018 •

edited

Loading

TimNN commented Apr 10, 2018

oli-obk Apr 10, 2018

oli-obk commented Apr 10, 2018

eddyb commented Apr 10, 2018

oli-obk Apr 10, 2018

eddyb Apr 10, 2018

oli-obk Apr 11, 2018

oli-obk Apr 11, 2018

TimNN commented Apr 11, 2018

bors commented Apr 12, 2018

michaelwoerister left a comment

michaelwoerister Apr 13, 2018

oli-obk Apr 13, 2018

michaelwoerister Apr 13, 2018

oli-obk Apr 13, 2018

michaelwoerister Apr 13, 2018

oli-obk Apr 13, 2018

oli-obk left a comment

oli-obk Apr 13, 2018

oli-obk Apr 13, 2018

oli-obk Apr 13, 2018

michaelwoerister commented Apr 13, 2018

oli-obk commented Apr 13, 2018

michaelwoerister commented Apr 13, 2018

rust-highfive commented Apr 13, 2018

bors commented Apr 14, 2018

oli-obk commented Apr 14, 2018

oli-obk commented Apr 15, 2018

bors commented Apr 15, 2018

bors commented Apr 15, 2018

bors commented Apr 15, 2018

pnkfelix commented Apr 19, 2018

alexcrichton commented Apr 20, 2018

Don't recurse into allocations, use a global table instead #49833

Don't recurse into allocations, use a global table instead #49833

Conversation

oli-obk commented Apr 10, 2018 • edited Loading

TimNN commented Apr 10, 2018

Choose a reason for hiding this comment

oli-obk commented Apr 10, 2018

eddyb commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimNN commented Apr 11, 2018

bors commented Apr 12, 2018

michaelwoerister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Apr 13, 2018

oli-obk commented Apr 13, 2018

michaelwoerister commented Apr 13, 2018

rust-highfive commented Apr 13, 2018

bors commented Apr 14, 2018

oli-obk commented Apr 14, 2018

oli-obk commented Apr 15, 2018

bors commented Apr 15, 2018

bors commented Apr 15, 2018

bors commented Apr 15, 2018

pnkfelix commented Apr 19, 2018

alexcrichton commented Apr 20, 2018

oli-obk commented Apr 10, 2018 •

edited

Loading