Loosened rules involving statics mentioning other statics #51110

alexreg · 2018-05-27T16:10:37Z

Before this PR, trying to mention a static in any way other than taking a reference to it caused a compile-time error. So, while

static A: u32 = 42;
static B: &u32 = &A;

compiles successfully,

static A: u32 = 42;
static B: u32 = A; // error

and

static A: u32 = 42;
static B: u32 = *&A; // error

are not possible to express in Rust. On the other hand, introducing an intermediate const fn can presently allow one to do just that:

static A: u32 = 42;
static B: u32 = foo(&A); // success!

const fn foo(a: &u32) -> u32 {
    *a
}

Preventing const fn from allowing to work around the ban on reading from statics would cripple const fn almost into uselessness.
Additionally, the limitation for reading from statics comes from the old const evaluator(s) and is not shared by miri.

This PR loosens the rules around use of statics to allow statics to evaluate other statics by value, allowing all of the above examples to compile and run successfully.
Reads from extern (foreign) statics are however still disallowed by miri, because there is no compile-time value to be read.

extern static A: u32;

static B: u32 = A; // error

This opens up a new avenue of potential issues, as a static can now not just refer to other statics or read from other statics, but even contain references that point into itself.
While it might seem like this could cause subtle bugs like allowing a static to be initialized by its own value, this is inherently impossible in miri.
Reading from a static causes the const_eval query for that static to be invoked. Calling the const_eval query for a static while already inside the const_eval query of said static will cause cycle errors.
It is not possible to accidentally create a bug in miri that would enable initializing a static with itself, because the memory of the static does not exist while being initialized.
The memory is not uninitialized, it is not there. Thus any change that would accidentally allow reading from a not yet initialized static would cause ICEs.

Tests have been modified according to the new rules, and new tests have been added for writing to static muts within definitions of statics (which needs to fail), and incremental compilation with complex/interlinking static definitions.
Note that incremental compilation did not need to be adjusted, because all of this was already possible before with workarounds (like intermediate const fns) and the encoding/decoding already supports all the possible cases.

r? @eddyb

eddyb · 2018-05-27T16:17:26Z

src/librustc_mir/transform/qualify_consts.rs

@@ -228,38 +222,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {

    /// Check if a Local with the current qualifications is promotable.
    fn can_promote(&self, qualif: Qualif) -> bool {


Can you inline can_promote now?

eddyb · 2018-05-27T16:19:15Z

src/librustc_mir/transform/qualify_consts.rs

@@ -492,7 +455,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
        match *place {
            Place::Local(ref local) => self.visit_local(local, context, location),
            Place::Static(ref global) => {
-                self.add(Qualif::STATIC);
+                if !(self.mode == Mode::Static || self.mode == Mode::StaticMut) {


Can you add a comment above this if saying that only statics can refer to other statics?

eddyb · 2018-05-27T16:21:13Z

src/librustc_mir/transform/qualify_consts.rs

@@ -677,16 +618,11 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
            Rvalue::Len(_) => {
                // Static places in consts would have errored already,
                // don't treat length checks as reads from statics.


Also remove the comment here.

eddyb · 2018-05-27T16:21:26Z

src/librustc_mir/transform/qualify_consts.rs

@@ -677,16 +618,11 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
            Rvalue::Len(_) => {
                // Static places in consts would have errored already,
                // don't treat length checks as reads from statics.
-                self.qualif = self.qualif - Qualif::STATIC;
            }

            Rvalue::Ref(_, kind, ref place) => {
                // Static places in consts would have errored already,
                // only keep track of references to them here.


Also remove the comment here.

Oops, yep. Should have paid more attention to the comments!

eddyb · 2018-05-27T16:32:04Z

cc @oli-obk @nikomatsakis This PR allows statics to read from other statics (although miri may still error as before, when the operation is actually attempted), which was sort of already possible:

const fn read<T: Copy>(r: &T) -> T { *r }
static FOO: u32 = read(&BAR);
static BAR: u32: 10;

That is, the analysis being removed here was an unsound approximation.
However, const and const fn still cannot even mention a static, neither for borrowing or reading.

In terms of motivation, simplifying the analysis qualify_consts performs will allow us to move it to the dataflow framework to support arbitrary CFG shapes, not just linear control-flow.
Does this PR require going through FCP or even an RFC?

src/librustc_mir/transform/qualify_consts.rs

-            } else {
-                "cannot refer to statics by value, use a constant instead"
-            };
-            struct_span_err!(self.tcx.sess, self.span, E0394, "{}", msg)


src/librustc_mir/transform/qualify_consts.rs

@@ -566,12 +518,6 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {

                        ProjectionElem::Field(..) |
                        ProjectionElem::Index(_) => {
-                            if this.mode != Mode::Fn &&
-                               this.qualif.intersects(Qualif::STATIC) {
-                                span_err!(this.tcx.sess, this.span, E0494,


src/librustc_mir/transform/qualify_consts.rs

@@ -305,7 +263,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {
            }) if self.mir.local_kind(index) == LocalKind::Temp
               && self.mir.local_decls[index].ty.is_box()
               && self.local_qualif[index].map_or(false, |qualif| {
-                    qualif.intersects(Qualif::NOT_CONST)
+                    qualif.contains(Qualif::NOT_CONST)


eddyb · 2018-05-27T20:23:30Z

src/librustc_mir/transform/qualify_consts.rs

@@ -416,7 +374,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {

        // Account for errors in consts by using the
        // conservative type qualification instead.
-        if self.qualif.intersects(Qualif::CONST_ERROR) {
+        if self.qualif.contains(Qualif::CONST_ERROR) {


Okay I just checked, and using contains is fine except for CONST_ERROR because it's a union of several other flags, and any of the flags in that union being set are relevant.

eddyb · 2018-05-27T20:23:41Z

src/librustc_mir/transform/qualify_consts.rs

@@ -1026,7 +951,7 @@ This does not pose a problem by itself because they can't be accessed directly."

            if let Some((ref dest, _)) = *destination {
                // Avoid propagating irrelevant callee/argument qualifications.
-                if self.qualif.intersects(Qualif::CONST_ERROR) {
+                if self.qualif.contains(Qualif::CONST_ERROR) {


Same here, needs intersects.

eddyb · 2018-05-28T05:58:10Z

src/librustc_mir/transform/qualify_consts.rs

-                            self.add(Qualif::NOT_CONST);
-                            return;
-                        }
+                    let static_kind = if self.tcx.is_foreign_item(global.def_id) {


s/static_kind/runtime_static

I'm pretty sure static_kind makes the most sense here... it's not a boolean, it's an Option<&'static str>.

This is irrelevant anyway now since we can't actually deny anything here.

eddyb · 2018-05-28T06:00:00Z

src/test/compile-fail/check-static-recursion-foreign.rs

@@ -29,5 +29,6 @@ extern "C" {
 }

 static B: &'static c_int = unsafe { &test_static };
+//~^ ERROR foreign statics cannot be accessed at compile-time


Wait, this is correct... I think we have to allow it, and instead make miri disallow reads from them. Sorry about the detour, I didn't realize borrowing extern static was already allowed.

Okay, no worries.

bors · 2018-06-29T01:17:51Z

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

alexreg · 2018-06-30T02:15:31Z

@eddyb What caused the latest Travis failures? Did something change in master when I rebased? (Because everything was passing before the clean-up, i.e. last commit.) I'm wary of simply hanging the expected error messages.

oli-obk · 2018-06-30T07:36:14Z

@alexreg due to conflicts you need to rebase anyway. Rerun the test suite after you do, it looks like master changed indeed. I did recently change something around error reporting, so I think you can just adjust the error message comments to fit reality

alexreg · 2018-06-30T22:01:35Z

@oli-obk Okay, will do.

Updated tests accordingly.

…borrowed.

alexreg · 2018-07-01T01:27:43Z

@oli-obk Rebase didn't help. I fixed the errors now though. If you see the last commit, you'll notice the need to duplicate the error messages though. I wonder if this is a bug.

oli-obk · 2018-07-01T10:39:28Z

src/test/compile-fail/issue-14227.rs

@@ -13,6 +13,10 @@
 extern {
    pub static symbol: ();
 }
-static CRASH: () = symbol; //~ cannot refer to other statics by value
+static CRASH: () = symbol;
+//~^ ERROR could not evaluate static initializer


open an issue about it. this feels like a regression by my diagnostics changes

Okay. Or do you want to, since it's your changes? Either way.

oli-obk · 2018-07-01T19:33:57Z

@bors r=eddyb

bors · 2018-07-01T19:33:58Z

📌 Commit 1c34227 has been approved by eddyb

bors · 2018-07-01T23:00:32Z

⌛ Testing commit 1c34227 with merge c697a56...

@eddyb

Loosened rules involving statics mentioning other statics Before this PR, trying to mention a static in any way other than taking a reference to it caused a compile-time error. So, while ```rust static A: u32 = 42; static B: &u32 = &A; ``` compiles successfully, ```rust static A: u32 = 42; static B: u32 = A; // error ``` and ```rust static A: u32 = 42; static B: u32 = *&A; // error ``` are not possible to express in Rust. On the other hand, introducing an intermediate `const fn` can presently allow one to do just that: ```rust static A: u32 = 42; static B: u32 = foo(&A); // success! const fn foo(a: &u32) -> u32 { *a } ``` Preventing `const fn` from allowing to work around the ban on reading from statics would cripple `const fn` almost into uselessness. Additionally, the limitation for reading from statics comes from the old const evaluator(s) and is not shared by `miri`. This PR loosens the rules around use of statics to allow statics to evaluate other statics by value, allowing all of the above examples to compile and run successfully. Reads from extern (foreign) statics are however still disallowed by miri, because there is no compile-time value to be read. ```rust extern static A: u32; static B: u32 = A; // error ``` This opens up a new avenue of potential issues, as a static can now not just refer to other statics or read from other statics, but even contain references that point into itself. While it might seem like this could cause subtle bugs like allowing a static to be initialized by its own value, this is inherently impossible in miri. Reading from a static causes the `const_eval` query for that static to be invoked. Calling the `const_eval` query for a static while already inside the `const_eval` query of said static will cause cycle errors. It is not possible to accidentally create a bug in miri that would enable initializing a static with itself, because the memory of the static *does not exist* while being initialized. The memory is not uninitialized, it is not there. Thus any change that would accidentally allow reading from a not yet initialized static would cause ICEs. Tests have been modified according to the new rules, and new tests have been added for writing to `static mut`s within definitions of statics (which needs to fail), and incremental compilation with complex/interlinking static definitions. Note that incremental compilation did not need to be adjusted, because all of this was already possible before with workarounds (like intermediate `const fn`s) and the encoding/decoding already supports all the possible cases. r? @eddyb

bors · 2018-07-02T00:58:20Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing c697a56 to master...

@oli-obk

…ification, r=oli-obk Removed `uninitialized_statics` field from `Memory` struct in miri **IMPORTANT: Do not merge until rust-lang#51110 is merged** r? @oli-obk CC @eddyb

@oli-obk

…r=oli-obk Removed `uninitialized_statics` field from `Memory` struct in miri based on #51110 r? @oli-obk CC @eddyb

rust-highfive assigned eddyb May 27, 2018

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

eddyb reviewed May 27, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

eddyb reviewed May 27, 2018

View reviewed changes

src/librustc_mir/transform/qualify_consts.rs

} else {

"cannot refer to statics by value, use a constant instead"

};

struct_span_err!(self.tcx.sess, self.span, E0394, "{}", msg)

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

eddyb reviewed May 27, 2018

View reviewed changes

alexreg force-pushed the new-static-eval-rules branch from 27ce051 to 7f0fe80 Compare May 27, 2018 17:17

This comment has been minimized.

Sign in to view

alexreg force-pushed the new-static-eval-rules branch from d06d230 to 9b37382 Compare May 27, 2018 18:29

This comment has been minimized.

Sign in to view

eddyb reviewed May 27, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

alexreg force-pushed the new-static-eval-rules branch from 9b37382 to a40be59 Compare May 27, 2018 20:30

This comment has been minimized.

Sign in to view

alexreg force-pushed the new-static-eval-rules branch from 4413c45 to 57c531f Compare May 27, 2018 23:17

This comment has been minimized.

Sign in to view

alexreg force-pushed the new-static-eval-rules branch from eaf3096 to 8d7434a Compare May 28, 2018 01:16

This comment has been minimized.

Sign in to view

alexreg force-pushed the new-static-eval-rules branch from 8d7434a to c72a028 Compare May 28, 2018 02:54

This comment has been minimized.

Sign in to view

eddyb reviewed May 28, 2018

View reviewed changes

kennytm 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 Jun 25, 2018

Alexander Regueiro added 8 commits June 30, 2018 23:52

Loosened rules involving statics mentioning other statics.

1393176

Updated tests accordingly.

Added miri error for evaluating foreign statics.

349d53c

Updated tests accordingly.

Fixed bug with miri const evaluation where allocation is recursively …

a2cd95f

…borrowed.

Added tests for writing to static mut's in statics.

9e48472

Added tests fo referring to statics by value in other statics.

f8132d3

Added incremental test for interlinking static references.

9f751a9

Minor refactoring.

13b82ec

Modified expected error messages in accordance with rebase.

1c34227

alexreg force-pushed the new-static-eval-rules branch from c7a35eb to 1c34227 Compare July 1, 2018 01:28

oli-obk reviewed Jul 1, 2018

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2018

bors merged commit 1c34227 into rust-lang:master Jul 2, 2018

bors mentioned this pull request Jul 2, 2018

Do not promote constants that contain unpromotable code #51570

Closed

bors added a commit that referenced this pull request Jul 3, 2018

Auto merge of #51428 - alexreg:uninitialized-statics-simplification, …

81d5c3e

…r=oli-obk Removed `uninitialized_statics` field from `Memory` struct in miri based on #51110 r? @oli-obk CC @eddyb

mbrobbel mentioned this pull request Nov 15, 2019

rustc crash when trying to reference static export from another crate #24843

Closed

ehuss mentioned this pull request Aug 3, 2020

Some constant/static updates. rust-lang/reference#867

Merged

		@@ -228,38 +222,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {

		/// Check if a Local with the current qualifications is promotable.
		fn can_promote(&self, qualif: Qualif) -> bool {

Loosened rules involving statics mentioning other statics #51110

Loosened rules involving statics mentioning other statics #51110

Conversation

alexreg commented May 27, 2018 • edited Loading

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

eddyb commented May 27, 2018 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

bors commented Jun 29, 2018

alexreg commented Jun 30, 2018

oli-obk commented Jun 30, 2018

alexreg commented Jun 30, 2018

alexreg commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jul 1, 2018

bors commented Jul 1, 2018

bors commented Jul 1, 2018

bors commented Jul 2, 2018

alexreg commented May 27, 2018 •

edited

Loading

eddyb commented May 27, 2018 •

edited

Loading