Fix regression in promotion of rvalues referencing a static #44442

Aaron1011 · 2017-09-09T01:06:26Z

This commit makes librustc_passes::consts::CheckCrateVisitor properly
mark expressions as promotable if they reference a static, as it's
perfectly fine for one static to reference another. It fixes a
regression that prevented a temporary rvalue from referencing a static
if it was itself declared within a static.

Prior to commit b8c05fe90bc,
region::ScopeTree would only register a 'terminating scope' for function
bodies. Thus, while rvalues in a static that referenced a static would be marked
unpromotable, the lack of enclosing scope would cause
mem_categorization::MemCategorizationContext::cat_rvalue_node
to compute a 'temporary scope' of ReStatic. Since this had the same
effect as explicitly selecting a scope of ReStatic
due to the rvalue being marked by CheckCrateVisitor as promotable,
no issue occurred.

However, commit b8c05fe90bc
made ScopeTree unconditionally register a 'terminating scope'
Since mem_categorization would now compute a non-static 'temporary scope', the
aforementioned rvalues would be erroneously marked as living for too
short a time.

By fixing the behavior of CheckCrateVisitor, this commit avoids changing
mem_categorization's behavior, while ensuring that temporary values in
statics are still allowed to reference other statics.

Fixes issue #44373

This commit makes librustc_passes::consts::CheckCrateVisitor properly mark expressions as promotable if they reference a static, as it's perfectly fine for one static to reference another. It fixes a regression that prevented a temporary rvalue from referencing a static if it was itself declared within a static. Prior to commit rust-lang@b8c05fe90bc, `region::ScopeTree` would only register a 'terminating scope' for function bodies. Thus, while rvalues in a static that referenced a static would be marked unpromotable, the lack of enclosing scope would cause mem_categorization::MemCategorizationContext::cat_rvalue_node to compute a 'temporary scope' of `ReStatic`. Since this had the same effect as explicitly selecting a scope of `ReStatic` due to the rvalue being marked by CheckCrateVisitor as promotable, no issue occurred. However, commit rust-lang@b8c05fe90bc made ScopeTree unconditionally register a 'terminating scope' Since mem_categorization would now compute a non-static 'temporary scope', the aforementioned rvalues would be erroneously marked as living for too short a time. By fixing the behavior of CheckCrateVisitor, this commit avoids changing mem_categorization's behavior, while ensuring that temporary values in statics are still allowed to reference other statics. Fixes issue rust-lang#44373

rust-highfive · 2017-09-09T01:06:33Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (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.

Aaron1011 · 2017-09-09T01:07:28Z

r? @eddyb

eddyb · 2017-09-09T08:02:26Z

Ah, changing the MIR side like I suggested in the issue isn't enough. Still, you should do it.

eddyb · 2017-09-09T08:05:42Z

src/librustc_passes/consts.rs

+                    if !thread_local {
+                        debug!("Allowing promotion of reference to Static(id={:?})", did);
+                    }
+                }


Hmm, this will allow promoting reads from statics, which is bad. Can you make sure to only promote if the mode is Static? i.e. only a static can refer to another static.

~~Actually, wait, this is old borrowck, how is this even a problem at all? I need to investigate further.~~ Realized [&TEST] is what doesn't get promoted in your original testcase, so this makes sense, but, again, it should be limited to statics referring to other statics.
Hopefully the MIR constant checking will prevent edge cases from compiling.

Aaron1011 · 2017-09-09T21:11:11Z

@eddyb: Done

eddyb · 2017-09-10T02:54:20Z

src/librustc_passes/consts.rs

@@ -502,6 +517,7 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
        tcx,
        tables: &ty::TypeckTables::empty(None),
        in_fn: false,
+        in_static: false,


Ah, there's no mode anymore, I guess this is fine.

Aaron1011 · 2017-09-10T03:45:58Z

@eddyb: It looks like the MIR-side change you suggested broke a few tests. While it looks like it should be possible to refactor some of the constant-related code to fix them, I think it would be simpler to leave the PR as-is. The original regression was caused by improper logic at the HIR level, so I think it would be better to fix that before making additional changes to the MIR logic.

eddyb · 2017-09-10T06:54:37Z

First off, you should not leave revert commits around but rather remove the commit (e.g. git reset HEAD~1) and force-push.

As for the errors, it's very important to note that they result in promotion of static reads, which means my STATIC_REF is probably already broken. Quite bad if that is the case.
EDIT: Ahh it doesn't make the distinction between a read and an lvalue, so it's too conversative, which means allowing STATIC_REF to promote is too relaxing, allowing both &FOO and &{FOO}.

Anyway, I'll warn @oli-obk that your fix and new test might break miri - and of course, it will break the new MIR borrowck but that is not on by default yet, and I'll investigate STATIC_REF.

eddyb · 2017-09-11T06:13:24Z

@bors r+ Thanks!

bors · 2017-09-11T06:13:25Z

📌 Commit 813b323 has been approved by eddyb

arielb1 · 2017-09-11T11:37:44Z

src/librustc_passes/consts.rs

-                Def::Fn(..) | Def::Method(..) => {}
+                Def::Fn(..) | Def::Method(..) =>  {}
+
+                // References to a static are inherently promotable,


Comment doesn't match code.

References to statics outside of a static aren't promotable.

arielb1 · 2017-09-11T11:37:58Z

@bors r-

Please have the comment match the code

Aaron1011 · 2017-09-11T14:51:56Z

@arielb1: Done

eddyb · 2017-09-11T17:24:50Z

@bors r+

bors · 2017-09-11T17:24:51Z

📌 Commit fb540e3 has been approved by eddyb

bors · 2017-09-11T20:02:26Z

⌛ Testing commit fb540e3 with merge eba374f...

Fix regression in promotion of rvalues referencing a static This commit makes librustc_passes::consts::CheckCrateVisitor properly mark expressions as promotable if they reference a static, as it's perfectly fine for one static to reference another. It fixes a regression that prevented a temporary rvalue from referencing a static if it was itself declared within a static. Prior to commit b8c05fe90bc, `region::ScopeTree` would only register a 'terminating scope' for function bodies. Thus, while rvalues in a static that referenced a static would be marked unpromotable, the lack of enclosing scope would cause mem_categorization::MemCategorizationContext::cat_rvalue_node to compute a 'temporary scope' of `ReStatic`. Since this had the same effect as explicitly selecting a scope of `ReStatic` due to the rvalue being marked by CheckCrateVisitor as promotable, no issue occurred. However, commit b8c05fe90bc made ScopeTree unconditionally register a 'terminating scope' Since mem_categorization would now compute a non-static 'temporary scope', the aforementioned rvalues would be erroneously marked as living for too short a time. By fixing the behavior of CheckCrateVisitor, this commit avoids changing mem_categorization's behavior, while ensuring that temporary values in statics are still allowed to reference other statics. Fixes issue #44373

bors · 2017-09-11T22:49:16Z

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

rust-highfive assigned arielb1 Sep 9, 2017

rust-highfive assigned eddyb and unassigned arielb1 Sep 9, 2017

eddyb reviewed Sep 9, 2017

View reviewed changes

Don't promote references to statics that occur in non-static locations

813b323

eddyb reviewed Sep 10, 2017

View reviewed changes

Aaron1011 force-pushed the promote-static-ref branch from a60f6ce to 813b323 Compare September 10, 2017 11:28

arielb1 reviewed Sep 11, 2017

View reviewed changes

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 11, 2017

Update comment to properly describe static promotion restrictions

fb540e3

bors merged commit fb540e3 into rust-lang:master Sep 11, 2017

bors mentioned this pull request Sep 11, 2017

remove or encapsulate the remaining non-query data in tcx #44501

Merged

oli-obk mentioned this pull request Sep 12, 2017

Clippy v0.0.158 fails to build on nightly (eba374fb2 2017-09-11) rust-lang/rust-clippy#2044

Closed

Aaron1011 deleted the promote-static-ref branch November 12, 2017 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression in promotion of rvalues referencing a static #44442

Fix regression in promotion of rvalues referencing a static #44442

Aaron1011 commented Sep 9, 2017

rust-highfive commented Sep 9, 2017

Aaron1011 commented Sep 9, 2017

eddyb commented Sep 9, 2017

eddyb Sep 9, 2017

eddyb Sep 9, 2017 •

edited

Loading

Aaron1011 commented Sep 9, 2017

eddyb Sep 10, 2017

Aaron1011 commented Sep 10, 2017

eddyb commented Sep 10, 2017 •

edited

Loading

eddyb commented Sep 11, 2017

bors commented Sep 11, 2017

arielb1 Sep 11, 2017

arielb1 Sep 11, 2017

arielb1 commented Sep 11, 2017

Aaron1011 commented Sep 11, 2017

eddyb commented Sep 11, 2017

bors commented Sep 11, 2017

bors commented Sep 11, 2017

bors commented Sep 11, 2017

Fix regression in promotion of rvalues referencing a static #44442

Fix regression in promotion of rvalues referencing a static #44442

Conversation

Aaron1011 commented Sep 9, 2017

rust-highfive commented Sep 9, 2017

Aaron1011 commented Sep 9, 2017

eddyb commented Sep 9, 2017

eddyb Sep 9, 2017

Choose a reason for hiding this comment

eddyb Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

Aaron1011 commented Sep 9, 2017

eddyb Sep 10, 2017

Choose a reason for hiding this comment

Aaron1011 commented Sep 10, 2017

eddyb commented Sep 10, 2017 • edited Loading

eddyb commented Sep 11, 2017

bors commented Sep 11, 2017

arielb1 Sep 11, 2017

Choose a reason for hiding this comment

arielb1 Sep 11, 2017

Choose a reason for hiding this comment

arielb1 commented Sep 11, 2017

Aaron1011 commented Sep 11, 2017

eddyb commented Sep 11, 2017

bors commented Sep 11, 2017

bors commented Sep 11, 2017

bors commented Sep 11, 2017

eddyb Sep 9, 2017 •

edited

Loading

eddyb commented Sep 10, 2017 •

edited

Loading