ScalarPairs are offset==0 field + other non-zst field #51307

oli-obk · 2018-06-02T17:42:11Z

eddyb · 2018-06-02T18:11:50Z

src/librustc_mir/interpret/place.rs

@@ -137,7 +137,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
            Value::Scalar(_) if offset.bytes() == 0 && field.size == base_layout.size => Ok(Some((base, field.ty))),
            // split fat pointers, 2 element tuples, ...
            Value::ScalarPair(a, b) if base_layout.fields.count() == 2 => {
-                let val = [a, b][field_index];
+                let val = [a, b][base_layout.fields.memory_index(field_index)];


This is still not a good idea (if you want to see what happens, try adding a bunch of ZSTs) - what you want is to check whether offset.bytes() == 0 or not. I could've sworn I already wrote the code in this module to do it correctly, are there several similar pieces of code in here?

eddyb · 2018-06-02T18:12:05Z

src/librustc_mir/interpret/terminator/mod.rs

-                                            if let Value::ScalarPair(_, value) = other {
-                                                write_next(value)?;
+                                            if let Value::ScalarPair(a, b) = other {
+                                                write_next([a, b][layout.fields.memory_index(1)])?;


eddyb · 2018-06-02T18:12:09Z

src/librustc_mir/transform/const_prop.rs

@@ -221,7 +221,7 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> {
                            trace!("layout computed");
                            use rustc_data_structures::indexed_vec::Idx;
                            let field_index = field.index();
-                            let val = [a, b][field_index];
+                            let val = [a, b][base_layout.fields.memory_index(field_index)];


eddyb · 2018-06-04T10:16:36Z

src/librustc_mir/interpret/terminator/mod.rs

+                                        Value::Scalar(_) => bug!("Scalar does not cover entire type"),
+                                    };
+                                    let dest =
+                                        self.eval_place(&mir::Place::Local(arg_local))?;


Why not do something like this for the source, too?

eddyb · 2018-06-04T10:16:52Z

src/librustc_mir/transform/const_prop.rs

-                        _ => None,
-                    }
+                    let (value, field_ty) = self.use_ecx(span, |this| {
+                        this.ecx.read_field(base, None, field, ty)


Like this, maybe?

eddyb · 2018-06-04T10:18:12Z

src/librustc_mir/interpret/place.rs

@@ -136,8 +136,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
            Value::ScalarPair(..) |
            Value::Scalar(_) if offset.bytes() == 0 && field.size == base_layout.size => Ok(Some((base, field.ty))),
            // split fat pointers, 2 element tuples, ...


This comment is too specific.

eddyb · 2018-06-04T13:03:56Z

@bors r+

bors · 2018-06-04T13:03:57Z

📌 Commit f7eedfa has been approved by eddyb

bors · 2018-06-04T15:32:39Z

⌛ Testing commit f7eedfa with merge 4baf141bf291b89ec611558c28ea995733055246...

bors · 2018-06-04T18:36:46Z

💔 Test failed - status-travis

rust-highfive · 2018-06-04T18:36:48Z

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

[02:58:30] test [run-make] run-make-fulldeps/windows-spawn ... ok
[02:58:30] test [run-make] run-make-fulldeps/volatile-intrinsics ... ok
[02:58:31] test [run-make] run-make-fulldeps/windows-subsystem ... ok
[02:58:33] test [run-make] run-make-fulldeps/sysroot-crates-are-unstable ... ok
The job exceeded the maximum time limit for jobs, and has been terminated.

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)

rust-highfive · 2018-06-04T18:37:22Z

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

[02:58:30] test [run-make] run-make-fulldeps/windows-spawn ... ok
[02:58:30] test [run-make] run-make-fulldeps/volatile-intrinsics ... ok
[02:58:31] test [run-make] run-make-fulldeps/windows-subsystem ... ok
[02:58:33] test [run-make] run-make-fulldeps/sysroot-crates-are-unstable ... ok
The job exceeded the maximum time limit for jobs, and has been terminated.

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)

oli-obk · 2018-06-04T18:55:02Z

all builders took less than 2h except for the dist builder, which timed out after 3h 😕

pietroalbini · 2018-06-04T18:59:25Z

@bors retry

bors · 2018-06-04T23:10:21Z

⌛ Testing commit f7eedfa with merge c610be9...

@eddyb

ScalarPairs are offset==0 field + other non-zst field r? @eddyb fixes #51300

bors · 2018-06-05T01:46:30Z

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

rust-highfive assigned eddyb Jun 2, 2018

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

oli-obk force-pushed the miri_fixes branch from 08b2554 to ecca26d Compare June 2, 2018 17:43

eddyb reviewed Jun 2, 2018

View reviewed changes

Correctly access ScalarPair fields during const eval

47f37d6

oli-obk force-pushed the miri_fixes branch from ecca26d to 47f37d6 Compare June 2, 2018 21:34

oli-obk changed the title ~~ScalarPairs are ordered in memory order~~ ScalarPairs are offset==0 field + other non-zst field Jun 3, 2018

eddyb reviewed Jun 4, 2018

View reviewed changes

Simplify value field access

f7eedfa

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 Jun 4, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 4, 2018

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 Jun 4, 2018

bors added a commit that referenced this pull request Jun 4, 2018

Auto merge of #51307 - oli-obk:miri_fixes, r=eddyb

c610be9

ScalarPairs are offset==0 field + other non-zst field r? @eddyb fixes #51300

bors merged commit f7eedfa into rust-lang:master Jun 5, 2018

oli-obk deleted the miri_fixes branch June 5, 2018 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ScalarPairs are offset==0 field + other non-zst field #51307

ScalarPairs are offset==0 field + other non-zst field #51307

oli-obk commented Jun 2, 2018

eddyb Jun 2, 2018

eddyb Jun 2, 2018

eddyb Jun 2, 2018

eddyb Jun 4, 2018

eddyb Jun 4, 2018

eddyb Jun 4, 2018

eddyb commented Jun 4, 2018

bors commented Jun 4, 2018

bors commented Jun 4, 2018

bors commented Jun 4, 2018

rust-highfive commented Jun 4, 2018

rust-highfive commented Jun 4, 2018

oli-obk commented Jun 4, 2018

pietroalbini commented Jun 4, 2018

bors commented Jun 4, 2018

bors commented Jun 5, 2018

ScalarPairs are offset==0 field + other non-zst field #51307

ScalarPairs are offset==0 field + other non-zst field #51307

Conversation

oli-obk commented Jun 2, 2018

eddyb Jun 2, 2018

Choose a reason for hiding this comment

eddyb Jun 2, 2018

Choose a reason for hiding this comment

eddyb Jun 2, 2018

Choose a reason for hiding this comment

eddyb Jun 4, 2018

Choose a reason for hiding this comment

eddyb Jun 4, 2018

Choose a reason for hiding this comment

eddyb Jun 4, 2018

Choose a reason for hiding this comment

eddyb commented Jun 4, 2018

bors commented Jun 4, 2018

bors commented Jun 4, 2018

bors commented Jun 4, 2018

rust-highfive commented Jun 4, 2018

rust-highfive commented Jun 4, 2018

oli-obk commented Jun 4, 2018

pietroalbini commented Jun 4, 2018

bors commented Jun 4, 2018

bors commented Jun 5, 2018