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

Remove Allocation.runtime_mutability #50193

Closed
wants to merge 1 commit into from

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Apr 24, 2018

This fixes a bug where NopLogger in static mut LOGGER: &'static Log = &NopLogger will be marked mutable by mark_static_initialized. With this bugfix, Allocation.runtime_mutability is entirely unused and is removed.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 24, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Apr 24, 2018

Won't this break static mut FOO: &mut u32 = &mut 42;?

We also need the flag for #49933 though it might be possible to get that flag back via the machine.

@@ -63,7 +63,6 @@ pub trait Machine<'mir, 'tcx>: Sized {
fn mark_static_initialized<'a>(
_mem: &mut Memory<'a, 'mir, 'tcx, Self>,
_id: AllocId,
_mutability: Mutability,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument needs to stay for miri the tool

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the tool use it for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Knowing whether the memory can be changed. The tool actually mutates statics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool shouldn't use mark_static_initialized at all though, since that only applies to compile time execution?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to do this because otherwise we deallocate the memory of the 42 in static FOO: &u32 = &42; after the computation is done.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 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.
[00:44:44] 
[00:44:44] running 2965 tests
[00:44:59] ....................................................................................................
[00:45:14] .............................................i......................................................
[00:45:31] .................................................................F..................................
[00:46:00] ....................................................................................................
[00:46:21] ....................................................................................................
[00:46:36] ....................................................................................................
[00:46:51] ....................................................................................................
---
[00:55:14] 
[00:55:14] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:488:22
[00:55:14] 
[00:55:14] 
[00:55:14] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:55:14] 
[00:55:14] 
[00:55:14] failed 6_64-unknown-linux-gnu/release/incremental
111128 ./obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu
111128 ./obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu
111124 ./obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release
107240 ./obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps
102808 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends
101924 ./obj/build/bootstrap/debug/incremental/bootstrap-2s8ik7x786gic
101920 ./obj/build/bootstrap/debug/incremental/bootstrap-2s8ik7x786gic/s-f0enuo18a0-1d1jek5-8chh8umqc096
89216 ./obj/build/x86_64-unknown-linux-gnu/stage1
89192 ./obj/build/x86_64-unknown-linux-gnu/stage1/lib
88056 ./obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/incremental/core-31lccp6wy7orz
88056 ./obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/incremental/core-31lccp6wy7orz
88052 ./obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/incremental/core-31lccp6wy7orz/s-f0ens9tbq7-p0tlls-3by8jin3pp2v3
84832 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps
81104 ./obj/build/x86_64-unknown-linux-gnu/doc/std
78700 ./obj/build/x86_64-unknown-linux-gnu/stage0-sysroot
78696 ./obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib

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)

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 24, 2018

Won't this break static mut FOO: &mut u32 = &mut 42;?

That is illegal.

Edit: but static mut TEST: &'static mut [isize] = &mut [1]; is not....

@oli-obk
Copy link
Contributor

oli-obk commented Apr 24, 2018

Immutable references to unsafecell also need to be marked I think

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 24, 2018

Immutable references to unsafecell also need to be marked I think

Those give: error[E0492]: cannot borrow a constant which may contain interior mutability, create a static instead

@oli-obk
Copy link
Contributor

oli-obk commented Apr 24, 2018

Isn't that just a limitation of the current system?

static mut FOO: &UnsafeCell<i32> = &UnsafeCell::new(42);

should be fine in my mental model of statics

@eddyb
Copy link
Member

eddyb commented Apr 24, 2018

@oli-obk Those should be allowed, yes, but only for "escaping" references (i.e. no StorageLive on the underlying local being borrowed), and properly checked for Sync.
And we need to make sure we don't accidentally allow too much in const.

@Zoxc Zoxc closed this Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants