Skip to content

Handle statics in MIR as const pointers #66587

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
Merged
merged 8 commits into from
Nov 23, 2019
Merged

Handle statics in MIR as const pointers #66587

merged 8 commits into from
Nov 23, 2019

Conversation

matthewjasper
Copy link
Contributor

This is the first PR towards the goal of removing PlaceBase::Static. In this PR:

  • Statics are lowered to dereferencing a const pointer.
  • The temporaries holding such pointers are tracked in MIR, for the most part this is only used for diagnostics. There are two exceptions:
    • The borrow checker has some checks for thread-locals that directly use this data.
    • Const checking will suppress "cannot dereference raw pointer" diagnostics for pointers to static mut/extern static. This is to maintain the current behaviour (12 tests fail otherwise).

The following are left to future PRs (I think that @spastorino will be working on the first 3):

  • Applying the same treatments to promoted statics.
  • Removing PlaceBase::Static.
  • Replacing PlaceBase with Local.
  • Moving the ever growing collection of metadata that we have for diagnostics in MIR passes somewhere more appropriate.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 20, 2019
Centril
Centril reviewed Nov 20, 2019
View reviewed changes
oli-obk
oli-obk reviewed Nov 21, 2019
View reviewed changes
oli-obk
oli-obk approved these changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

This is great, a much more principled approach than what I was attempting

@spastorino
Copy link
Member

@matthewjasper great work, indeed I'm working on the first 3.

spastorino
spastorino reviewed Nov 21, 2019
View reviewed changes
@bors
Copy link
Collaborator

bors commented Nov 21, 2019

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

@spastorino
Copy link
Member

IMO this is ready to r+, would leave this up to @oli-obk though

@oli-obk
Copy link
Contributor

oli-obk commented Nov 22, 2019

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 22, 2019

📌 Commit bccc59a has been approved by oli-obk

@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 Nov 22, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 22, 2019
@Centril
Rollup merge of rust-lang#66587 - matthewjasper:handle-static-as-cons…
3031720 
…t, r=oli-obk

Handle statics in MIR as const pointers

This is the first PR towards the goal of removing `PlaceBase::Static`. In this PR:

* Statics are lowered to dereferencing a const pointer.
* The temporaries holding such pointers are tracked in MIR, for the most part this is only used for diagnostics. There are two exceptions:
    * The borrow checker has some checks for thread-locals that directly use this data.
    * Const checking will suppress "cannot dereference raw pointer" diagnostics for pointers to `static mut`/`extern static`. This is to maintain the current behaviour (12 tests fail otherwise).

The following are left to future PRs (I think that @spastorino will be working on the first 3):

* Applying the same treatments to promoted statics.
* Removing `PlaceBase::Static`.
* Replacing `PlaceBase` with `Local`.
* Moving the ever growing collection of metadata that we have for diagnostics in MIR passes somewhere more appropriate.

r? @oli-obk
@Centril Centril mentioned this pull request Nov 22, 2019
Merged
bors added a commit that referenced this pull request Nov 22, 2019
@bors
Auto merge of #66640 - Centril:rollup-862009l, r=Centril
a449535 
Rollup of 8 pull requests

Successful merges:

 - #66183 (*Syntactically* permit visibilities on trait items & enum variants)
 - #66566 (Document pitfall with `impl PartialEq<B> for A`)
 - #66575 (Remove pretty printing of specific nodes in AST)
 - #66587 (Handle statics in MIR as const pointers)
 - #66619 (follow the convention in this file to use third-person singular verbs)
 - #66633 (Error code's long explanation cleanup)
 - #66637 (fix reoccuring typo: dereferencable -> dereferenceable)
 - #66639 (resolve: more declarative `fresh_binding`)

Failed merges:

r? @ghost
@bors bors merged commit bccc59a into rust-lang:master Nov 23, 2019
@matthewjasper matthewjasper deleted the handle-static-as-const branch November 23, 2019 10:39
@ecstatic-morse ecstatic-morse mentioned this pull request Nov 27, 2019
Merged
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this pull request Nov 28, 2019
@ecstatic-morse
Don't treat a reference to a static as a reborrow
a70ac50 
They now look the same in the MIR after rust-lang#66587.
@bjorn3
Copy link
Member

bjorn3 commented Dec 1, 2019

Before this PR is was possible to always force a Operand::Constant to a const allocation and codegen a reference to that alloc. After this PR that would make TLS statics impossible to handle, as you would get reference to a reference to the static. You would then not know that the second reference should be treated as TLS access.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 1, 2019

Do pointers track the TLS-ness of what they point to? If so, we can make const alloc codegen add those markers

@bjorn3
Copy link
Member

bjorn3 commented Dec 1, 2019

No. Only when an alloc gets directly referenced from a function, does the cranelift_codegen::ir::GlobalValue pointing to it get marked as TLS value. When creating a pointer to it using global_value cranelift then calls the necessary function to get the right pointer for the current thread.

I have been able to solve the problem for cg_clif in bjorn3/rustc_codegen_cranelift@30de2a0. That commit makes the logic more like cg_llvm.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 1, 2019

So... the status is you solved the problem, but we could still improve on it by making this part of codegen_ssa to duplicate less code?

@bjorn3
Copy link
Member

bjorn3 commented Dec 1, 2019

the status is you solved the problem

Yes

but we could still improve on it by making this part of codegen_ssa to duplicate less code?

cg_ssa already does this, but I can't use it much in cg_clif yet, as it depends to much on several llvm properties.

@matthewjasper matthewjasper mentioned this pull request Dec 25, 2019
Closed
@oli-obk oli-obk mentioned this pull request Dec 26, 2019
Closed
@oli-obk oli-obk mentioned this pull request Jan 2, 2020
Merged
@matthewjasper matthewjasper mentioned this pull request Jan 7, 2020
Closed
@ecstatic-morse ecstatic-morse mentioned this pull request Feb 12, 2020
Closed
@jonas-schievink jonas-schievink mentioned this pull request Mar 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spastorino spastorino spastorino approved these changes

@oli-obk oli-obk oli-obk approved these changes

+1 more reviewer

@Centril Centril Centril left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: 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.
7 participants
@matthewjasper @spastorino @bors @oli-obk @bjorn3 @Centril @rust-highfive