Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change vtable memory representation to use tcx allocated allocations. #86475

Merged
merged 3 commits into from
Jun 29, 2021
Merged

Change vtable memory representation to use tcx allocated allocations. #86475

merged 3 commits into from
Jun 29, 2021

Conversation

crlf0710
Copy link
Member

This fixes #86324. However i suspect there's more to change before it can land.

r? @bjorn3
cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the miri_vtable_refactor branch from ecba2e5 to 15f1f71 Compare June 20, 2021 09:44
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member

RalfJung commented Jun 20, 2021 •
edited
Loading

I am very confused by the PR title. Surely Miri should still use its own allocations for stack and heap allocations. Also the change seems to have a lot to do with vtables, but that term does not appear in the PR title. It would help to describe more precisely what is going on. :)

EDIT: Oh, looks like what you mean use "Change Miri to use txc-allocation allocation for vtables". That makes a lot more sense. You had me worried at first here. ;)
That's not the whole change though, is it? It also removes vtable generation from other parts of the compiler.

@bjorn3
Copy link
Member

bjorn3 commented Jun 20, 2021

If you don't know yet ./x.py check should check cg_clif by default unlike ./x.py build. You can make ./x.py build also build cg_clif by adding "cranelift" to the rust.codegen-backends array in config.toml.

@crlf0710 crlf0710 changed the title Change miri to use tcx allocated allocations. Change vtable memory representation to use tcx allocated allocations. Jun 20, 2021
@crlf0710
Copy link
Member Author

Thank you for your comments! I'll update the pr and address the review comments tomorrow evening in my timezone (after about 18 hours)

@oli-obk
Copy link
Contributor

oli-obk commented Jun 21, 2021

Thank you for your comments! I'll update the pr and address the review comments tomorrow evening in my timezone (after about 18 hours)

No rush. Take all the time you need. There's no expectation that reviews are addressed in any timeframe other than what the author feels good with.

@crlf0710
Copy link
Member Author

Thanks! I was a little busy this week, i'll get to it this weekend.

@crlf0710 crlf0710 force-pushed the miri_vtable_refactor branch from 15f1f71 to 0df271d Compare June 27, 2021 11:28
@crlf0710 crlf0710 force-pushed the miri_vtable_refactor branch 2 times, most recently from 10b692c to c1eafa7 Compare June 27, 2021 11:38
@crlf0710
Copy link
Member Author

Updated and addressed the comments.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the miri_vtable_refactor branch from 263a8d3 to a6a5cba Compare June 28, 2021 01:10
@bjorn3
Copy link
Member

bjorn3 commented Jun 28, 2021

Only x86_64-apple timed out. All other jobs succeeded. Probably spurious.

@bors retry

@bors 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 28, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jun 29, 2021

⌛ Testing commit 97772bb with merge 12d658d005db54ae0594fa815a70d2ab79747b70...

@bors
Copy link
Contributor

bors commented Jun 29, 2021

💔 Test failed - checks-actions

@bors 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 29, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@crlf0710
Copy link
Member Author 

2021-06-29T10:40:18.9223083Z curl: (56) OpenSSL SSL_read: Connection was reset, errno 10054
2021-06-29T10:40:18.9316146Z ##[error]Process completed with exit code 56.

Mmm, it seems another retry is needed? Or do i need to rebase forward? @bjorn3

@bjorn3
Copy link
Member

bjorn3 commented Jun 29, 2021

@bors retry

@bors 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 29, 2021
@bors
Copy link
Contributor

bors commented Jun 29, 2021

⌛ Testing commit 97772bb with merge e98897e...

@bors
Copy link
Contributor

bors commented Jun 29, 2021

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing e98897e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 29, 2021
@bors bors merged commit e98897e into rust-lang:master Jun 29, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 29, 2021
@rust-highfive rust-highfive mentioned this pull request Jun 29, 2021
Closed
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #86475!

Tested on commit e98897e.
Direct link to PR: #86475

💔 miri on windows: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 29, 2021
@rust-highfive
📣 Toolstate changed by rust-lang/rust#86475!
6c9ff64 
Tested on commit rust-lang/rust@e98897e.
Direct link to PR: <rust-lang/rust#86475>

💔 miri on windows: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).
@bjorn3
Copy link
Member

bjorn3 commented Jun 29, 2021

Third time's the charm.

@bors bors mentioned this pull request Jun 29, 2021
Merged
@crlf0710 crlf0710 deleted the miri_vtable_refactor branch June 30, 2021 04:03
@crlf0710 crlf0710 mentioned this pull request Jun 30, 2021
Open
21 tasks
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jul 7, 2021
@bors
Auto merge of rust-lang#86475 - crlf0710:miri_vtable_refactor, r=bjorn3
b186f86 
Change vtable memory representation to use tcx allocated allocations.

This fixes rust-lang#86324. However i suspect there's more to change before it can land.

r? `@bjorn3`
cc `@rust-lang/miri`
eddyb
eddyb reviewed Aug 2, 2021
View reviewed changes
compiler/rustc_middle/src/ty/context.rs
Comment on lines +1048 to +1050

pub(super) vtables_cache:
Lock<FxHashMap<(Ty<'tcx>, Option<ty::PolyExistentialTraitRef<'tcx>>), AllocId>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, any reason this is a mutable cache in the global context, instead of a query? It looks like the kind of thing we replaced with queries long ago.

(Maybe we should find a way to ban !Freeze types in GlobalCtxt, outside of incremental/query state?)

cc @oli-obk @wesleywiser

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually gave it a try as a query locally, but without succeed. The compilation complained about keys becoming bigger, something like that, i can't remember the exact error content.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's because that key type is a bit too "spread out". Sadly you can't just use TraitRef<'tcx>, without passing some dummy trait for the DefId.

You could "just" update the expected size, but someone would need to check off on that (and I don't know who) - the problem there is it can affect performance even if the new query is completely unused.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a cache without any dependency tracking looks like a bug to me.

eddyb
eddyb reviewed Aug 2, 2021
View reviewed changes
compiler/rustc_codegen_ssa/src/traits/consts.rs
@@ -26,6 +26,8 @@ pub trait ConstMethods<'tcx>: BackendTypes {
fn const_to_opt_uint(&self, v: Self::Value) -> Option<u64>;
fn const_to_opt_u128(&self, v: Self::Value, sign_ext: bool) -> Option<u128>;

fn const_data_from_alloc(&self, alloc: &Allocation) -> Self::Value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just from_const_alloc, isn't it? Just with a hardcoded offset of 0 and no type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from_const_alloc also returns a PlaceRef while this returns a raw pointer value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but the PlaceRef is made from that pointer value and the type/layout passed in.

@michaelwoerister michaelwoerister mentioned this pull request Oct 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 approved these changes

@michaelwoerister michaelwoerister michaelwoerister left review comments

@oli-obk oli-obk oli-obk left review comments

@RalfJung RalfJung RalfJung left review comments

@eddyb eddyb eddyb left review comments

Assignees

@bjorn3 bjorn3

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.

Refactor: have miri create an Allocation for the vtable
10 participants
@crlf0710 @rust-highfive @rust-log-analyzer @RalfJung @bjorn3 @oli-obk @bors @eddyb @michaelwoerister @rustbot