Move with_globals setup from run_compiler to run

[Zoxc]

An alternative to #53526

Note this breaks some miri stuff and clippy since they call run_compiler directly, and they now need to also call with_globals cc @rust-lang/dev-tools

r? @oli-obk

[alexreg]

Does this solve all the issues in #53420?

[Zoxc]

@alexreg No, it just makes it print the ICE as it normally should be printed instead of cannot access a scoped thread local variable without calling set first

[nrc]

This will break RLS and Rustfmt too, I think.

[earthengine]

Although this passes the tests, but I am afraid that if the monitor itself captured an error and need to emit, we would still have the same problem. This is the consideration that makes me put the with_globals outside the monitor call.

I originally goes through this and in turn created run_compiler_impl as a private variant that don't do wraps, to avoid breakage of outside calls.

[oli-obk]

does the monitor ever need to emit diagnostics?

[Zoxc]

The monitor does emit diagnostics, but it only uses dummy spans, which does not seem to access libsyntax globals. I've tested that the ICE printing still works locally.

[Zoxc]

@nrc RLS uses run, so it doesn't need any changes. rustfmt doesn't seems to use the compiler at all, just libsyntax, so no changes should be needed.

[alexreg]

@Zoxc Didn't the patch I sent to you work? And avoid breaking other things?

[oli-obk]

I updated clippy. It should work with this PRs changes now. Can you update the clippy submodule to the current master? Then this PR doesn't break clippy at all

[alexreg]

@oli-obk Good stuff. How are you getting on with the underlying MIR issue?

[rust-highfive]

The job x86_64-gnu-llvm-5.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.

######################################################################    98.0%
######################################################################## 100.0%
[00:02:08] extracting /checkout/obj/build/cache/2018-08-01/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:02:13]     Updating registry `https://github.com/rust-lang/crates.io-index`
[00:02:36] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:36] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:36] Makefile:81: recipe for target 'prepare' failed
[00:02:36] make: *** [prepare] Error 1
[00:02:37] Command failed. Attempt 2/5:
[00:02:37] Command failed. Attempt 2/5:
[00:02:37] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:37] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:37] make: *** [prepare] Error 1
[00:02:37] Makefile:81: recipe for target 'prepare' failed
[00:02:39] Command failed. Attempt 3/5:
[00:02:39] Command failed. Attempt 3/5:
[00:02:39] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:39] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:39] make: *** [prepare] Error 1
[00:02:39] Makefile:81: recipe for target 'prepare' failed
[00:02:42] Command failed. Attempt 4/5:
[00:02:42] Command failed. Attempt 4/5:
[00:02:43] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:43] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:43] Makefile:81: recipe for target 'prepare' failed
[00:02:43] make: *** [prepare] Error 1
[00:02:47] Command failed. Attempt 5/5:
[00:02:47] Command failed. Attempt 5/5:
[00:02:47] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:47] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:47] make: *** [prepare] Error 1
[00:02:47] Makefile:81: recipe for target 'prepare' failed
[00:02:47] The command has failed after 5 attempts.
travis_time:end:14570ece:start=1535329687676662679,finish=1535329855435349045,duration=167758686366
---
travis_time:end:25ad7200:start=1535329855879001484,finish=1535329855892545901,duration=13544417
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:28f65ef0
$ 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 -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:01158c53
travis_time:start:01158c53
$ 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:052459a5
$ 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)

[earthengine]

I am a bit concerned about this change as it modifies the public API for run_compiler. If we want to stabilize this call to the public, to enable third party tools development, we should not expect the user to call with_globals by themselves. Therefore, I believe it would be better to make sure run_compiler as a public interface being always wrapped, and use an internal function for the unwrapped calls.

This will also provide a less breaking changes to easily fix the current check fails.

[alexreg]

Looks like CI is failing because of the submodule update? Or is it just transient?

[oli-obk]

@bors r+

@earthengine If we ever want to stabilize this API we'd need to do so many other changes, this seems like the least of the worries I have with that.

We could rather check if we can merge run and run_compiler by looking at all use cases.

[bors]

📌 Commit 0972658 has been approved by oli-obk

[bors]

⌛ Testing commit 0972658 with merge 291d958...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 291d958 to master...