prepare for Intptrcast model by pvdrz · Pull Request #61781 · rust-lang/rust

pvdrz · 2019-06-12T18:10:18Z

#61668 done right (I hope so). r? @RalfJung @oli-obk

oli-obk

@RalfJung are you ok with us not nuking to_ptr and to_bits immediately? It would make the PR much more complex since we'd have to fiddle things through Allocation (in librustc where we don't know about the force* methods or InterpCX)

oli-obk · 2019-06-13T09:09:22Z

+ }
+ }
+
+ pub fn force_bits(&self, scalar: Scalar<M::PointerTag>) -> InterpResult<'tcx, u128> {


I know this PR isn't going to full intptrcast in one step, but I think it should still use force_bits $somewhere. Maybe start with just
rust/src/librustc_mir/interpret/cast.rs
Line 251 in 24ddd16
Int(_) | Uint(_) => err!(ReadPointerAsBytes),
(call cast_from_int after you get the bits via force_bits)
rust/src/librustc_mir/interpret/place.rs
Line 472 in 24ddd16
let n = n.to_bits(self.tcx.data_layout.pointer_size)?;
rust/src/librustc_mir/interpret/operator.rs
Line 313 in 24ddd16
let l = left.to_bits().expect("we checked is_ptr");
rust/src/librustc_mir/interpret/operator.rs
Line 350 in 24ddd16
let val = val.to_bits(layout.size)?;

I'll do these changes and add a commit :P

No please don't do anything on casts! That's just a red herring, people can still transmute stuff. We need to do conversion lazily, nor eagerly.
I thought we had discussed in detail where force_ptr is needed? Namely, the methods in "place.rs" and "operand.rs" that access memory. And then for force_bits, that would arise only inside Miri in the newest plan, in ptr_op.

Oh wait, that cast stuff is for non-ptr-sized casts. Hm. Okay we should eventually force_bits there.
But that should be properly deduplicated with the int-to-int cass, so I don't think that method is the right place. Maybe let's keep that one for later.

D: ok I haven't committed anything. What should I do next then?

Ok, I've already substituted to_ptr by force_ptr in the last commit, if the bin_op change is going to happen in miri, are we done here?

Oh also force_bits shouldn't assume pointer size, that makes it rather inconvenient to use. It should take the size as argument, like to_bits does.
But it can assert that if the size is NOT pointer size, it will never see a pointer value.
I've already substituted to_ptr by force_ptr in the last commit
I hope you mean "carefully replaced in a few places", not "substituted everywhere with sed". ;)

Hahaha, yeah I've just replaced them on the places with the @RalfJung approval seal.

Oh sorry my bad, this was only about force_bits. I should learn to read. :(
I agree with the following two:
rust/src/librustc_mir/interpret/place.rs
Line 472 in 24ddd16
let n = n.to_bits(self.tcx.data_layout.pointer_size)?;
rust/src/librustc_mir/interpret/operator.rs
Line 350 in 24ddd16
let val = val.to_bits(layout.size)?;
But this one makes no sense to change as it already panics if there are no bits there.
I'm going to do those changes then

Oh also force_bits shouldn't assume pointer size, that makes it rather inconvenient to use. It should take the size as argument, like to_bits does.
I'm going to do this change before the other ones then and will do a couple commits in a few minutes

RalfJung · 2019-06-13T16:35:43Z

are you ok with us not nuking to_ptr and to_bits immediately?

Absolutely, I didn't even necessarily expect them to be nuked at all.

RalfJung

LGTM. :) @oli-obk any objections?

pvdrz · 2019-06-14T04:51:26Z

I believe CI needs to be invoked again, or force bors to do the merge test

RalfJung · 2019-06-14T06:19:12Z

Why that? Travis is green, and Azure can be ignored. ;)

pvdrz · 2019-06-14T14:27:20Z

Why that? Travis is green, and Azure can be ignored. ;)

I had the impression bors won't do its work until both CI pipelines are green

oli-obk · 2019-06-14T14:28:58Z

I had the impression bors won't do its work until both CI pipelines are green

bors will start even if all CI is red.

pvdrz · 2019-06-14T14:31:52Z

I had the impression bors won't do its work until both CI pipelines are green
bors will start even if all CI is red.

Man these bots like to live dangerously :P

oli-obk · 2019-06-14T14:33:36Z

@bors r+ go live dangerously

bors · 2019-06-14T14:33:37Z

📌 Commit 212f233 has been approved by oli-obk

@RalfJung

…=oli-obk prepare for Intptrcast model rust-lang#61668 done right (I hope so). r? @RalfJung @oli-obk

@RalfJung

…=oli-obk prepare for Intptrcast model rust-lang#61668 done right (I hope so). r? @RalfJung @oli-obk

@RalfJung

…=oli-obk prepare for Intptrcast model rust-lang#61668 done right (I hope so). r? @RalfJung @oli-obk

RalfJung · 2019-06-16T08:04:35Z

@bors r-

This seems to break Miri. And we should get at least one nightly with a working Miri, because currently the latest distributed Miri is broken.

pvdrz · 2019-06-16T16:43:15Z

Ok problem fixed, miri tests are passing in local now

RalfJung · 2019-06-17T07:59:33Z

Awesome!

@bors r=oli-obk,RalfJung

bors · 2019-06-17T07:59:34Z

📌 Commit 1e38870 has been approved by oli-obk,RalfJung

bors · 2019-06-17T12:49:27Z

⌛ Testing commit 1e38870 with merge 4d5404b...

@RalfJung

…lfJung prepare for Intptrcast model #61668 done right (I hope so). r? @RalfJung @oli-obk

bors · 2019-06-17T13:32:28Z

💔 Test failed - checks-travis

rust-highfive · 2019-06-17T13:32:30Z

The job dist-x86_64-apple-alt 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.

wry
xmoto
zxing-cpp
==> Downloading https://homebrew.bintray.com/bottles/xz-5.2.4.high_sierra.bottle.tar.gz
==> Downloading from https://akamai.bintray.com/e7/e7be50f4ee00e35887f3957263334eb3baba59e8c061919060f9259351be6880?__gda__=exp=1560776987~hmac=f5a0eb6bafb8280c0687aa7883ff7cebe733f12f5474bc3431b68a3a10d30374&response-content-disposition=attachment%3Bfilename%3D%22xz-5.2.4.high_sierra.bottle.tar.gz%22&response-content-type=application%2Fgzip&requestInfo=U2FsdGVkX1-fpewvHiK0gS5H8XIrZwr5DBdqsEQc-LiptDhDjIIYh2fpi3PjL5WvbQ4HUfwbRmCkxO0QiNs2JtlXeqV4pcPZ0PhpTHDw4d94m92W8RNCOXhbVq-VRJZS4AWvrRB_sdg6_p6lEaBSFA&response-X-Checksum-Sha1=32dc0b28e61f32b40c20e2993418aa8cb6e746d5&response-X-Checksum-Sha2=e7be50f4ee00e35887f3957263334eb3baba59e8c061919060f9259351be6880
🍺 /usr/local/Cellar/xz/5.2.4: 92 files, 1MB
==> `brew cleanup` has not been run in 30 days, running now...
Removing: /Users/travis/Library/Caches/Homebrew/boost-1.66.0.high_sierra.bottle.tar.gz... (84.6MB)
Removing: /Users/travis/Library/Caches/Homebrew/carthage-0.28.0.high_sierra.bottle.tar.gz... (8.3MB)
---
Pruned 0 symbolic links and 5 directories from /usr/local
==> Installing dependencies for swig: pcre
==> Installing swig dependency: pcre
==> Downloading https://homebrew.bintray.com/bottles/pcre-8.43.high_sierra.bottle.tar.gz
==> Downloading from https://akamai.bintray.com/03/0389911a93a88efd4a69b52dea8ecb872fdb55bcfff45d2f7313be5f79730861?__gda__=exp=1560776999~hmac=8335a62405a58e2f985537e436a88a715905fbfeaf3c8883bf846719a5dff7b1&response-content-disposition=attachment%3Bfilename%3D%22pcre-8.43.high_sierra.bottle.tar.gz%22&response-content-type=application%2Fgzip&requestInfo=U2FsdGVkX18Qsmno3zxnWl1O2nk_XJ9bHKCCLwSfPeaK5L0w6ddJPMJ6XB3mXxVu9tX7bIgs4pUqSHo49ki-SZuOEmgPYBx4qJUezmlgnoXUalEpqebSgVU_F_-57aESrfarUaXZDDplz4ahpOFKww&response-X-Checksum-Sha1=c67d4b99bb245f0ea56b34118dd6325b06a7250c&response-X-Checksum-Sha2=0389911a93a88efd4a69b52dea8ecb872fdb55bcfff45d2f7313be5f79730861
🍺 /usr/local/Cellar/pcre/8.43: 204 files, 5.5MB
==> Installing swig
==> Downloading https://homebrew.bintray.com/bottles/swig-4.0.0.high_sierra.bottle.tar.gz
==> Downloading https://homebrew.bintray.com/bottles/swig-4.0.0.high_sierra.bottle.tar.gz
==> Downloading from https://akamai.bintray.com/ae/aed79cb436b3a0ac5812c4085e3121ffd62866397b8c7eaa06815ed8ec1e22b7?__gda__=exp=1560777002~hmac=a9ecf974f9c74c125a4633d9a1f3dfc192d7b405425f6978d6791b0d0c74e878&response-content-disposition=attachment%3Bfilename%3D%22swig-4.0.0.high_sierra.bottle.tar.gz%22&response-content-type=application%2Fgzip&requestInfo=U2FsdGVkX1-6-wmxVnWn-4IzKOjDx2j1kOanyPqDn6gNeqzVh9tfhhDzCX_kLLjyooorwdOCdQiJl1jAjvIZw5LDfBjkD7x2IA2H5tUaHjH1y9-eIps0iWqOuxRej0gjHKTLCCDTNbjxPR7wmf6fzA&response-X-Checksum-Sha1=a9c428aee4337d91061a69c02d7ae508b627d03f&response-X-Checksum-Sha2=aed79cb436b3a0ac5812c4085e3121ffd62866397b8c7eaa06815ed8ec1e22b7
🍺 /usr/local/Cellar/swig/4.0.0: 722 files, 5.4MB
travis_time:end:11556a98:start=1560775909732083000,finish=1560776322479357000,duration=412747274000
travis_fold:end:install
travis_fold:start:before_script.1
---
[00:01:50] Compiling serde_derive v1.0.81
[00:02:17] Compiling toml v0.4.10
[00:02:17] Compiling serde_json v1.0.33
[00:02:22] Compiling bootstrap v0.0.0 (/Users/travis/build/rust-lang/rust/src/bootstrap)
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build 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 · 2019-06-17T13:57:32Z

@bors retry timeout

bors · 2019-06-17T18:04:22Z

⌛ Testing commit 1e38870 with merge b25ee64...

@RalfJung

…lfJung prepare for Intptrcast model #61668 done right (I hope so). r? @RalfJung @oli-obk

bors · 2019-06-17T20:55:02Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk,RalfJung
Pushing b25ee64 to master...

rust-highfive assigned RalfJung Jun 12, 2019

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

This comment has been minimized.
Sign in to view

oli-obk reviewed Jun 13, 2019
View reviewed changes

RalfJung reviewed Jun 13, 2019
View reviewed changes

Comment threadsrc/librustc_mir/interpret/memory.rs

RalfJung changed the title ~~Intptrcast model~~prepare for Intptrcast modelJun 13, 2019

RalfJung approved these changes Jun 13, 2019
View reviewed changes

oli-obk reviewed Jun 14, 2019
View reviewed changes

Comment threadsrc/librustc_mir/interpret/operator.rs Outdated

oli-obk reviewed Jun 14, 2019
View reviewed changes

Comment threadsrc/librustc_mir/interpret/place.rs Outdated

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 14, 2019

Centril added a commit to Centril/rust that referenced this pull request Jun 14, 2019
Rollup merge ofrust-lang#61781- christianpoveda:intptrcast-model, r…
68f6e50
…=oli-obk prepare for Intptrcast model rust-lang#61668 done right (I hope so). r? @RalfJung @oli-obk

Centril mentioned this pull request Jun 14, 2019
Rollup of 8 pull requests #61840
Closed

Centril added a commit to Centril/rust that referenced this pull request Jun 14, 2019
Rollup merge ofrust-lang#61781- christianpoveda:intptrcast-model, r…
ed76b45
…=oli-obk prepare for Intptrcast model rust-lang#61668 done right (I hope so). r? @RalfJung @oli-obk

Centril mentioned this pull request Jun 14, 2019
Rollup of 10 pull requests #61850
Closed

Centril added a commit to Centril/rust that referenced this pull request Jun 16, 2019
Rollup merge ofrust-lang#61781- christianpoveda:intptrcast-model, r…
671426e
…=oli-obk prepare for Intptrcast model rust-lang#61668 done right (I hope so). r? @RalfJung @oli-obk

Centril mentioned this pull request Jun 16, 2019
Rollup of 6 pull requests #61880
Closed

pvdrz added 3 commits June 16, 2019 02:33

Add force_bits and force_ptr methods
3bd1734

Change to_ptr by force_ptr
681bd83

Replace to_bits by force_bits and move size as parameter
c5c06a5

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 Jun 16, 2019

RalfJung reviewed Jun 16, 2019
View reviewed changes

Comment threadsrc/librustc_mir/interpret/machine.rs Outdated

Add special behaviour when int is zero
1e38870

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 Jun 17, 2019

bors added a commit that referenced this pull request Jun 17, 2019
Auto merge of#61781- christianpoveda:intptrcast-model, r=oli-obk,Ra…
4d5404b
…lfJung prepare for Intptrcast model #61668 done right (I hope so). r? @RalfJung @oli-obk

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 17, 2019

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 17, 2019

bors added a commit that referenced this pull request Jun 17, 2019
Auto merge of#61781- christianpoveda:intptrcast-model, r=oli-obk,Ra…
b25ee64
…lfJung prepare for Intptrcast model #61668 done right (I hope so). r? @RalfJung @oli-obk

bors mentioned this pull request Jun 17, 2019
Replace some uses of NodeId with HirId #61836
Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 17, 2019

bors merged commit 1e38870 into rust-lang:masterJun 17, 2019

pvdrz deleted the intptrcast-model branch June 17, 2019 20:59

Uh oh!

Uh oh!

Conversation

pvdrz commented Jun 12, 2019

Uh oh!

This comment has been minimized.

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvdrzJun 13, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Jun 13, 2019

Uh oh!

Uh oh!

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

pvdrz commented Jun 14, 2019

Uh oh!

RalfJung commented Jun 14, 2019

Uh oh!

Uh oh!

pvdrz commented Jun 14, 2019

Uh oh!

oli-obk commented Jun 14, 2019

Uh oh!

Uh oh!

pvdrz commented Jun 14, 2019

Uh oh!

oli-obk commented Jun 14, 2019

Uh oh!

bors commented Jun 14, 2019

Uh oh!

RalfJung commented Jun 16, 2019

Uh oh!

Uh oh!

pvdrz commented Jun 16, 2019

Uh oh!

RalfJung commented Jun 17, 2019

Uh oh!

bors commented Jun 17, 2019

Uh oh!

bors commented Jun 17, 2019

Uh oh!

bors commented Jun 17, 2019

Uh oh!

rust-highfive commented Jun 17, 2019

Uh oh!

oli-obk commented Jun 17, 2019

Uh oh!

bors commented Jun 17, 2019

Uh oh!

bors commented Jun 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

pvdrzJun 13, 2019•
edited
Loading