Ensure that statics are always ByRef #50690

oli-obk · 2018-05-12T16:49:29Z

Statics aren't values to be used, they are names for memory locations.

r? @eddyb

cc @Zoxc

fixes #50706

rust-highfive · 2018-05-12T16:49:32Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

oli-obk · 2018-05-12T16:49:53Z

r? @eddyb

eddyb · 2018-05-12T16:54:37Z

src/librustc_trans/mir/constant.rs

@@ -171,11 +128,11 @@ pub fn trans_static_initializer<'a, 'tcx>(
    let param_env = ty::ParamEnv::reveal_all();
    let static_ = cx.tcx.const_eval(param_env.and(cid))?;


I'm thinking const_eval isn't the best, but it's probably an okay compromise.

We could create a separate query for statics.

eddyb · 2018-05-12T16:55:06Z

@bors r+

bors · 2018-05-12T16:55:07Z

📌 Commit 89201f3 has been approved by eddyb

Zoxc · 2018-05-12T17:08:50Z

This makes ConstValue for statics special cased to avoid the optimizations in ConstValue. This reduces performance and adds unnecessary complexity.

Zoxc · 2018-05-13T14:06:04Z

src/librustc_mir/interpret/const_eval.rs

-    if layout.is_zst() {
-        return ty::Const::from_const_value(
-            tcx,
-            ConstValue::ByVal(PrimVal::Undef),


Is ZST always ByVal(PrimVal::Undef) in Miri?

No. Accessing a zst field of a ByRef will return a ByRef

Then this code should still be here.

Why? Won't we simply get an empty allocation in the ByRef?

Each Rust value should only map to one ConstValue. so a ZST type cannot be both ByVal(PrimVal::Undef) and ByRef.

that would require us to spray is_zst checks everywhere. Miri already "violates" your assumption left and right.

The current code tries to reduce special cases, only ignoring zst memory reads in Memory, not earlier.

Wait. wouldn't it actually make more sense to ~~try to~~ move to ByRef for zst everywhere? You can't ever have a zst value. They have no values (well one value, but we don't ever read/write it).

that would require us to spray is_zst checks everywhere. Miri already "violates" your assumption left and right.

Miri doesn't use ConstValue. It is not an assumption but an invariant of the ConstValue type. You just need to check for ZST types when converting to a ConstValue. Having this invariant for ConstValue is very useful since:

it reduces memory usage when it is interned

if you want to check for a specific value, you only need to look at one of it's variants, for example true only needs to check for ByVal(PrimVal::Bytes(1)) and can ignore ByRef entirely. There is a fair amount of code which relies on this properly which has to be updated if you want to make static always return ByRef.

We can get rid of the special case for ZST by making them have a Scalar ABI so they can be stored in ByVal instead of ByRef which would otherwise be required since they have Aggregate ABI currently.

Wait. wouldn't it actually make more sense to try to move to ByRef for zst everywhere? You can't ever have a zst value. They have no values (well one value, but we don't ever read/write it).

Then we'd have to create allocations for ZST types, which we want to avoid for performance reasons. That is why ConstValue::ByVal and ConstValue::ByValPair exist at all.

Zoxc · 2018-05-13T14:34:10Z

src/librustc_mir/interpret/const_eval.rs

-            if let Value::ByValPair(a, b) = val {
-                ConstValue::ByValPair(a, b)
-            } else {
-                bug!("expected ByValPair value, got {:?}", val);


Please keep these sanity checks. At least when debug-assertions is on. I really don't trust Miri code to be correct.

oli-obk · 2018-05-13T14:41:28Z

@bors r-

more sanity checks

bors · 2018-05-16T23:52:52Z

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

oli-obk · 2018-05-17T15:04:43Z

@eddyb @Zoxc this now also fixes #50706 (fix included here because without this PR the fix might do stupid things when accessing the fields of statics)

bors · 2018-05-17T16:50:21Z

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

eddyb · 2018-05-19T12:16:05Z

src/librustc/mir/interpret/value.rs

-    // Used only for the remaining cases
-    ByRef(&'tcx Allocation),
+    /// Used only for the remaining cases. An allocation + offset into the allocation
+    ByRef(&'tcx Allocation, u64),


Please use Size for offsets.

eddyb · 2018-05-19T15:18:28Z

src/librustc/mir/interpret/mod.rs

        MemoryPointer { alloc_id, offset }
    }

    pub(crate) fn wrapping_signed_offset<C: HasDataLayout>(self, i: i64, cx: C) -> Self {
        MemoryPointer::new(
            self.alloc_id,
-            cx.data_layout().wrapping_signed_offset(self.offset, i),
+            Size::from_bytes(cx.data_layout().wrapping_signed_offset(self.offset.bytes(), i)),


Why would wrapping_signed_offset not work on a Size?

because SIze doesn't have a sign. I mean we could create an Offset type (which would then also be the Output of impl Sub for Size)...

eddyb · 2018-05-19T15:45:24Z

@bors r+

bors · 2018-05-19T15:45:25Z

📌 Commit 7c25aa7 has been approved by eddyb

kennytm · 2018-05-19T17:09:09Z

@bors p=38

bors · 2018-05-19T17:30:26Z

⌛ Testing commit 7c25aa7 with merge 5c3960c...

@eddyb

Ensure that statics are always ByRef Statics aren't values to be used, they are names for memory locations. r? @eddyb cc @Zoxc fixes #50706

bors · 2018-05-19T19:37:01Z

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

rust-highfive assigned eddyb May 12, 2018

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

eddyb reviewed May 12, 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-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2018

Zoxc reviewed May 13, 2018

View reviewed changes

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 13, 2018

oli-obk force-pushed the mir_stuff branch 2 times, most recently from 7d96991 to dd048e9 Compare May 17, 2018 14:51

oli-obk force-pushed the mir_stuff branch from dd048e9 to b854924 Compare May 18, 2018 12:32

eddyb reviewed May 19, 2018

View reviewed changes

oli-obk added 4 commits May 19, 2018 14:24

Ensure that statics are always ByRef

8b99c61

Reintroduce some sanity checks

76ddede

Go through an allocation when accessing fields of constants

6630678

Keep statics' constant as ByRef

3e933f4

oli-obk force-pushed the mir_stuff branch from b854924 to 8aa1b0c Compare May 19, 2018 14:37

Use Size instead of u64 in mir interpretation

7c25aa7

oli-obk force-pushed the mir_stuff branch from 8aa1b0c to 7c25aa7 Compare May 19, 2018 14:39

eddyb reviewed May 19, 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 May 19, 2018

bors added a commit that referenced this pull request May 19, 2018

Auto merge of #50690 - oli-obk:mir_stuff, r=eddyb

5c3960c

Ensure that statics are always ByRef Statics aren't values to be used, they are names for memory locations. r? @eddyb cc @Zoxc fixes #50706

bors merged commit 7c25aa7 into rust-lang:master May 19, 2018

oli-obk deleted the mir_stuff branch June 15, 2020 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that statics are always ByRef #50690

Ensure that statics are always ByRef #50690

oli-obk commented May 12, 2018 •

edited

Loading

rust-highfive commented May 12, 2018

oli-obk commented May 12, 2018

eddyb May 12, 2018

oli-obk May 12, 2018

eddyb commented May 12, 2018

bors commented May 12, 2018

Zoxc commented May 12, 2018

Zoxc May 13, 2018

oli-obk May 13, 2018

Zoxc May 13, 2018

oli-obk May 13, 2018

Zoxc May 13, 2018

oli-obk May 13, 2018

oli-obk May 13, 2018

Zoxc May 13, 2018

Zoxc May 13, 2018

Zoxc May 13, 2018

oli-obk commented May 13, 2018

bors commented May 16, 2018

oli-obk commented May 17, 2018

bors commented May 17, 2018

eddyb May 19, 2018

oli-obk May 19, 2018

eddyb May 19, 2018

oli-obk May 19, 2018

eddyb commented May 19, 2018

bors commented May 19, 2018

kennytm commented May 19, 2018

bors commented May 19, 2018

bors commented May 19, 2018

		@@ -171,11 +128,11 @@ pub fn trans_static_initializer<'a, 'tcx>(
		let param_env = ty::ParamEnv::reveal_all();
		let static_ = cx.tcx.const_eval(param_env.and(cid))?;

Ensure that statics are always ByRef #50690

Ensure that statics are always ByRef #50690

Conversation

oli-obk commented May 12, 2018 • edited Loading

rust-highfive commented May 12, 2018

oli-obk commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented May 12, 2018

bors commented May 12, 2018

Zoxc commented May 12, 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

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 commented May 13, 2018

bors commented May 16, 2018

oli-obk commented May 17, 2018

bors commented May 17, 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

eddyb commented May 19, 2018

bors commented May 19, 2018

kennytm commented May 19, 2018

bors commented May 19, 2018

bors commented May 19, 2018

oli-obk commented May 12, 2018 •

edited

Loading