Skip to content

Refactor interning to properly mark memory as mutable or immutable - #58351

Merged
bors merged 22 commits into
rust-lang:masterfrom
oli-obk:double_check_const_eval
Jun 19, 2019
Merged

Refactor interning to properly mark memory as mutable or immutable#58351
bors merged 22 commits into
rust-lang:masterfrom
oli-obk:double_check_const_eval

Conversation

@oli-obk

@oli-obkoli-obk commented Feb 10, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

r? @RalfJung

This implementation is incomplete out of multiple reasons

  • add -Zunleash_the_miri_inside_of_you tests
  • report an error if there's an UnsafeCell behind a reference in a constant
  • make validity checks actually test whether the mutability of their allocations match what they see in the type

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
RalfJung
RalfJung reviewed Feb 10, 2019
View reviewed changes
Comment threadsrc/librustc_codegen_llvm/common.rs Outdated
RalfJung
RalfJung reviewed Feb 10, 2019
View reviewed changes
Comment threadsrc/librustc_mir/interpret/memory.rs Outdated
@bors

This comment has been minimized.

Sign in to view
@oli-obk
oli-obkforce-pushed the double_check_const_eval branch from 7d53e06 to 11518e2CompareFebruary 13, 2019 15:57
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk
oli-obkforce-pushed the double_check_const_eval branch 2 times, most recently from 98039de to 7e8dc45CompareFebruary 15, 2019 11:28
@RalfJung

RalfJung commented Feb 20, 2019

Copy link
Copy Markdown
Member

op_to_const still says

// FIXME shouldn't it be the case that `mark_static_initialized` has already
// interned this? I thought that is the entire point of that `FinishStatic` stuff?

I guess this is a good time to fix that as well?

RalfJung
RalfJung reviewed Feb 20, 2019
View reviewed changes
Comment threadsrc/librustc_mir/interpret/intern.rs Outdated
RalfJung
RalfJung reviewed Feb 20, 2019
View reviewed changes
Comment threadsrc/librustc_mir/interpret/intern.rs Outdated
RalfJung
RalfJung reviewed Feb 20, 2019
View reviewed changes
Comment threadsrc/librustc_mir/interpret/intern.rs Outdated
RalfJung
RalfJung reviewed Feb 20, 2019
View reviewed changes
Comment threadsrc/librustc_mir/interpret/intern.rs Outdated
@oli-obk
oli-obkforce-pushed the double_check_const_eval branch from 7e8dc45 to 99d3e10CompareFebruary 21, 2019 10:48
@bors

bors commented Feb 24, 2019

Copy link
Copy Markdown
Collaborator

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

bjorn3
bjorn3 requested changes Mar 5, 2019
View reviewed changes
Comment threadsrc/librustc_mir/const_eval.rs Outdated
@oli-obk
oli-obkforce-pushed the double_check_const_eval branch from a4ba6bf to acc5c58CompareMarch 6, 2019 13:04
@oli-obk

oli-obk commented Mar 7, 2019

Copy link
Copy Markdown
ContributorAuthor

I guess this is a good time to fix that as well?

done

@oli-obk

oli-obk commented Mar 7, 2019

Copy link
Copy Markdown
ContributorAuthor

@RalfJung I think it would make sense to merge this now and resolve the checkboxes from the OP in a separate PR.

@oli-obkoli-obk changed the title [WIP] Make interning explicitly care about types and the mutability of memory Refactor interning to properly mark memory as mutable or immutableMar 11, 2019
@bors

bors commented Mar 16, 2019

Copy link
Copy Markdown
Collaborator

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

RalfJung
RalfJung reviewed Mar 18, 2019
View reviewed changes
Comment threadsrc/librustc_mir/const_eval.rs Outdated
RalfJung
RalfJung reviewed Mar 18, 2019
View reviewed changes
Comment threadsrc/librustc_mir/const_eval.rs Outdated
RalfJung
RalfJung reviewed Mar 18, 2019
View reviewed changes
Comment threadsrc/librustc_mir/const_eval.rs Outdated
RalfJung
RalfJung reviewed Mar 18, 2019
View reviewed changes
Comment threadsrc/librustc_mir/interpret/validity.rs Outdated
RalfJung
RalfJung reviewed Mar 18, 2019
View reviewed changes
Comment threadsrc/librustc_mir/interpret/validity.rs Outdated
@borsbors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 19, 2019
@oli-obk

oli-obk commented Jun 19, 2019

Copy link
Copy Markdown
ContributorAuthor

@bors r=RalfJung

@bors

bors commented Jun 19, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 62af19b has been approved by RalfJung

@rust-highfive

rust-highfive commented Jun 19, 2019

Copy link
Copy Markdown
Contributor

The job x86_64-gnu-llvm-6.0 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.
travis_time:end:2a3a969e:start=1560931504599432808,finish=1560931506257770766,duration=1658337958
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout
Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
199536 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc
156500 ./src/llvm-project/clang
150156 ./obj/build/bootstrap/debug/incremental
134688 ./obj/build/bootstrap/debug/incremental/bootstrap-1llt3ypt1ftzv
134684 ./obj/build/bootstrap/debug/incremental/bootstrap-1llt3ypt1ftzv/s-fday4gdtdx-ap62ha-2adl2f17sobif
126580 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release
123856 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps
123656 ./src/llvm-project/llvm/test/CodeGen
108532 ./src/llvm-project/lldb
---
travis_time:end:2869cc17:start=1560932515414575910,finish=1560932515419314315,duration=4738405
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:27ed481e
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1bf52a60
travis_time:start:1bf52a60
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:077c4b40
$ dmesg | grep -i kill

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

oli-obk commented Jun 19, 2019

Copy link
Copy Markdown
ContributorAuthor

@bors r-

@borsbors 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 19, 2019
@oli-obk
oli-obkforce-pushed the double_check_const_eval branch from fc15d7c to ce39fffCompareJune 19, 2019 09:18
@RalfJung

RalfJung commented Jun 19, 2019

Copy link
Copy Markdown
Member

"Was lange währt wird endlich gut"
@bors r+

@bors

bors commented Jun 19, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit cd290c7 has been approved by RalfJung

@borsbors 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 19, 2019
@bors

bors commented Jun 19, 2019

Copy link
Copy Markdown
Collaborator

⌛ Testing commit cd290c7 with merge 9cb052a...

bors added a commit that referenced this pull request Jun 19, 2019
@bors
Auto merge of#58351- oli-obk:double_check_const_eval, r=RalfJung
9cb052a
Refactor interning to properly mark memory as mutable or immutable
r? @RalfJung
This implementation is incomplete out of multiple reasons
* [ ] add `-Zunleash_the_miri_inside_of_you` tests
* [ ] report an error if there's an `UnsafeCell` behind a reference in a constant
* [ ] make validity checks actually test whether the mutability of their allocations match what they see in the type
@bors

bors commented Jun 19, 2019

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 9cb052a to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Jun 19, 2019
@bors
bors merged commit cd290c7 into rust-lang:masterJun 19, 2019
@borsbors mentioned this pull request Jun 19, 2019
Merged
@oli-obk
oli-obk deleted the double_check_const_eval branch June 19, 2019 13:14
@mati865

mati865 commented Jun 20, 2019

Copy link
Copy Markdown
Member

Was this kind of perf regression expected?
https://perf.rust-lang.org/compare.html?start=a6cbf2d1344418cd2807cc5380ef1247647a1e12&end=9cb052acfb25c12d5e8960f9ea53b69a2f19b0e8&stat=instructions:u

@oli-obk

oli-obk commented Jun 20, 2019

Copy link
Copy Markdown
ContributorAuthor

partly, all the ctfe ones were definitely expected, since we are doing more work now in order to place constants into the correct memory location (before we were doing a kind of best effort thing that overaggressively placed constants into mutable memory). I'll look into improving this though.

@whitequarkwhitequark mentioned this pull request Jun 23, 2019
Closed
@cuvipercuviper mentioned this pull request Jun 28, 2019
Closed
@RalfJungRalfJung mentioned this pull request Aug 10, 2019
Closed
This was referenced Aug 25, 2019
Open
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJungRalfJungRalfJung left review comments
@bjorn3bjorn3bjorn3 requested changes

Assignees

@RalfJungRalfJung

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@oli-obk@rust-highfive@bors@RalfJung@Centril@Dylan-DPC-zz@mati865@bjorn3