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.

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

Handle statics in MIR as const pointers #66587

Merged
merged 8 commits into from Nov 23, 2019

Conversation

@matthewjasper
Copy link
Contributor

matthewjasper commented Nov 20, 2019

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

Copy link
Contributor

oli-obk left a comment

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

@spastorino

This comment has been minimized.

Copy link
Member

spastorino commented Nov 21, 2019

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

src/librustc/mir/mod.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 21, 2019

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

@matthewjasper matthewjasper force-pushed the matthewjasper:handle-static-as-const branch from c87e5ef to bccc59a Nov 21, 2019
@spastorino

This comment has been minimized.

Copy link
Member

spastorino commented Nov 21, 2019

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

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Nov 22, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 22, 2019

📌 Commit bccc59a has been approved by oli-obk

Centril added a commit to Centril/rust that referenced this pull request Nov 22, 2019
…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
bors added a commit that referenced this pull request Nov 22, 2019
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 added a commit that referenced this pull request Nov 22, 2019
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
4 checks passed
4 checks passed
pr #20191121.44 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@matthewjasper matthewjasper deleted the matthewjasper:handle-static-as-const branch Nov 23, 2019
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this pull request Nov 27, 2019
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this pull request Nov 28, 2019
@bjorn3

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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.

9999years added a commit to 9999years/rust that referenced this pull request Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.