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

Don't allow overwriting SESSION_GLOBALS #84954

Closed
Aaron1011 opened this issue May 5, 2021 · 1 comment · Fixed by #84961
Closed

Don't allow overwriting SESSION_GLOBALS #84954

Aaron1011 opened this issue May 5, 2021 · 1 comment · Fixed by #84961
Assignees
@GuillaumeGomez
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented May 5, 2021 •
edited by jyn514
Loading

Since rustc_span::SESSION_GLOBALS uses scoped-tls, it can be overwritten by calling SESSION_GLOBALS.set from within the closure passed to the original call to SESSION_GLOBALS.set. Any Spans created in the 'outer' SessionGlobals may become unusable within the inner SessionGlobals, since they will be pointing to the wrong interner. See #84953 for an example of this happening.

There is no reason to modify SESSION_GLOBALs once it's set for the first time. We should make it private to the rustc_span crate, and only expose setters which assert that it is currently unset.
@Aaron1011 Aaron1011 added C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels May 5, 2021
@GuillaumeGomez
Copy link
Member

I'll do it. That'll allow me to close the loop. ;)

@GuillaumeGomez GuillaumeGomez self-assigned this May 5, 2021
@GuillaumeGomez GuillaumeGomez mentioned this issue May 5, 2021
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jul 8, 2021
@GuillaumeGomez
Rollup merge of rust-lang#84961 - GuillaumeGomez:rework-session-globa…
82e7f7e 
…ls, r=pnkfelix

Rework SESSION_GLOBALS API

Fixes rust-lang#84954.

<s>Needs rust-lang#84953 to be merged first (I cherry-picked its commits to have CI pass).</s> (done)

r? `@Aaron1011`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jul 8, 2021
@GuillaumeGomez
Rollup merge of rust-lang#84961 - GuillaumeGomez:rework-session-globa…
a20f0d7 
…ls, r=oli-obk

Rework SESSION_GLOBALS API

Fixes rust-lang#84954.

<s>Needs rust-lang#84953 to be merged first (I cherry-picked its commits to have CI pass).</s> (done)

r? `@Aaron1011`
@bors bors closed this as completed in d9297ae Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@GuillaumeGomez GuillaumeGomez

Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Rework SESSION_GLOBALS API GuillaumeGomez/rust
2 participants
@Aaron1011 @GuillaumeGomez