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

Refactor handling of statics #77096

Open
RalfJung opened this issue Sep 23, 2020 · 7 comments
Open

Refactor handling of statics #77096

RalfJung opened this issue Sep 23, 2020 · 7 comments
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

Our current handling of statics is suboptimal: a use of a static STATIC is desugared to *{const-ptr-to-static}, which is nice because it means we do not need any special case in the MIR for statics, but causes problems when statics need to be treated differently. One example of such special treatment is const checking and promotion, because consts are not allowed to access statics.

Another issue is that to ensure correct safety checking of static accesses, the static_ptr_ty function needs to carefully use raw pointers or references, and the unsafety checker needs to backtranslate that information into proper error messages. A different way to say this is that unsafety checking is another case where statics need special treatment.

And possibly there are more cases where statics need special treatment? it would be great to get an exhaustive list of that, or at least as exhaustive as we can make it, to take that all into account for our design decisions.

All that said, places used to have special treatment in the MIR and @oli-obk worked hard to move away from that to the current situation. Before considering how to resolve the above problems, I'd like to better understand the issues with the old approach that were solved by moving to the current approach.

@jonas-schievink jonas-schievink added A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 23, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Sep 23, 2020

The exhaustive list of special treatment is https://github.com/rust-lang/rust/search?q=LocalInfo%3A%3AStaticRef plus https://github.com/rust-lang/rust/search?q=check_static_ptr

Though some of these are just for diagnostics and would work out of the box without those checks. E.g. in the unsafey checks we just need to prevent a diagnostic about raw pointer derefs and instead emit something about mutable or extern statics. The borrowck errors (e.g mutably borrowing an immutable static) work the same, it's just a diagnostic improvement.

The only place where it falls down is constness and promotion checks. If we'd not have the special checks, we would suddenly allow reading and referencing statics in constant items and const fn.

@RalfJung
Copy link
Member Author

@oli-obk thanks!

Do you remember what the issues were that you fixed by introducing this scheme in the first place, replacing the old scheme where "static" was a kind of Place in MIR?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 28, 2020

one thing was the size of mir::StatementKind. Since places were used everywhere, this had quite an impact on memory usage, but iirc also an impact on perf. This also made a lot match patterns simpler. Generally this was part of the plan of mir 2.0

@RalfJung
Copy link
Member Author

By "made match pattern simpler", do you mean the logic that compiles match patterns, or do you mean parts of the compiler that matched on places?

@RalfJung
Copy link
Member Author

StatementKind nowadays seems to only use Place behind a Box, so their size is decoupled. But Place itself of course would still grow when a DefId is added.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 28, 2020

do you mean parts of the compiler that matched on places?

I meant this.

StatementKind nowadays seems to only use Place behind a Box, so their size is decoupled. But Place itself of course would still grow when a DefId is added.

Rvalue doesn't box Places, and uses them everywhere (mostly within Operand).

There are other optimizations we can do to all of this (e.g. using PlaceIds or even OperandIds everywhere and indexing into a table of Places or Operands stored in the mir::Body. This would allow us to cheaply encode statics as places again.

@RalfJung
Copy link
Member Author

Right, you also mentioned adding them as "virtual locals". With all these schemes we'd then have to be careful for equality tests (different local ID could still mean the same static unless we make sure we never duplicate those).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants