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

std: Tweak expansion of thread-local const #90774

Merged
merged 1 commit into from
Nov 19, 2021
Merged

std: Tweak expansion of thread-local const #90774

merged 1 commit into from
Nov 19, 2021

Conversation

alexcrichton
Copy link
Member

This commit tweaks the expansion of thread_local! when combined with a
const { ... } value to help ensure that the rules which apply to
const { ... } blocks will be the same as when they're stabilized.
Previously with this invocation:

thread_local!(static NAME: Type = const { init_expr });

this would generate (on supporting platforms):

#[thread_local]
static NAME: Type = init_expr;

instead the macro now expands to:

const INIT_EXPR: Type = init_expr;
#[thread_local]
static NAME: Type = INIT_EXPR;

with the hope that because init_expr is defined as a const item then
it's not accidentally allowing more behavior than if it were put into a
static. For example on the stabilization issue this example now
gives the same error both ways.

@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 10, 2021
@alexcrichton alexcrichton mentioned this pull request Nov 10, 2021
Closed
3 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@alexcrichton
std: Tweak expansion of thread-local const
1ac5d7d 
This commit tweaks the expansion of `thread_local!` when combined with a
`const { ... }` value to help ensure that the rules which apply to
`const { ... }` blocks will be the same as when they're stabilized.
Previously with this invocation:

    thread_local!(static NAME: Type = const { init_expr });

this would generate (on supporting platforms):

    #[thread_local]
    static NAME: Type = init_expr;

instead the macro now expands to:

    const INIT_EXPR: Type = init_expr;
    #[thread_local]
    static NAME: Type = INIT_EXPR;

with the hope that because `init_expr` is defined as a `const` item then
it's not accidentally allowing more behavior than if it were put into a
`static`. For example on the stabilization issue [this example][ex] now
gives the same error both ways.

[ex]: rust-lang#84223 (comment)
nbdd0121
nbdd0121 approved these changes Nov 10, 2021
View reviewed changes
library/std/src/thread/local.rs
@@ -165,6 +165,7 @@ macro_rules! __thread_local_inner {
#[cfg_attr(not(windows), inline)] // see comments below
unsafe fn __getit() -> $crate::option::Option<&'static $t> {
const _REQUIRE_UNSTABLE: () = $crate::thread::require_unstable_const_init_thread_local();
const INIT_EXPR: $t = $init;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably INIT_VAL would be better, since it isn't an expression.

@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 11, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Nov 16, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2021

📌 Commit 1ac5d7d has been approved by m-ou-se

@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 16, 2021
@m-ou-se m-ou-se assigned m-ou-se and unassigned yaahc Nov 16, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2021
@matthiaskrgr
Rollup merge of rust-lang#90774 - alexcrichton:tweak-const, r=m-ou-se
cb989e0 
std: Tweak expansion of thread-local const

This commit tweaks the expansion of `thread_local!` when combined with a
`const { ... }` value to help ensure that the rules which apply to
`const { ... }` blocks will be the same as when they're stabilized.
Previously with this invocation:

    thread_local!(static NAME: Type = const { init_expr });

this would generate (on supporting platforms):

    #[thread_local]
    static NAME: Type = init_expr;

instead the macro now expands to:

    const INIT_EXPR: Type = init_expr;
    #[thread_local]
    static NAME: Type = INIT_EXPR;

with the hope that because `init_expr` is defined as a `const` item then
it's not accidentally allowing more behavior than if it were put into a
`static`. For example on the stabilization issue [this example][ex] now
gives the same error both ways.

[ex]: rust-lang#84223 (comment)
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 16, 2021
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 17, 2021
@matthiaskrgr
Rollup merge of rust-lang#90774 - alexcrichton:tweak-const, r=m-ou-se
3a55f2f 
std: Tweak expansion of thread-local const

This commit tweaks the expansion of `thread_local!` when combined with a
`const { ... }` value to help ensure that the rules which apply to
`const { ... }` blocks will be the same as when they're stabilized.
Previously with this invocation:

    thread_local!(static NAME: Type = const { init_expr });

this would generate (on supporting platforms):

    #[thread_local]
    static NAME: Type = init_expr;

instead the macro now expands to:

    const INIT_EXPR: Type = init_expr;
    #[thread_local]
    static NAME: Type = INIT_EXPR;

with the hope that because `init_expr` is defined as a `const` item then
it's not accidentally allowing more behavior than if it were put into a
`static`. For example on the stabilization issue [this example][ex] now
gives the same error both ways.

[ex]: rust-lang#84223 (comment)
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 17, 2021
Closed
@hkratz
Copy link
Contributor

hkratz commented Nov 17, 2021 •
edited
Loading

Could this have caused the following aarch64 assertion failure in the rollup #90975 CI (CI log):

fatal runtime error: assertion failed: thread_info.is_none()

apparently stemming from:

rust/library/std/src/sys_common/thread_info.rs

Line 13 in 1149193

thread_local! { static THREAD_INFO: RefCell<Option<ThreadInfo>> = const { RefCell::new(None) } }

rust/library/std/src/sys_common/thread_info.rs

Lines 41 to 47 in 1149193

pub fn set(stack_guard: Option<Guard>, thread: Thread) {
THREAD_INFO.with(move |thread_info| {
let mut thread_info = thread_info.borrow_mut();
rtassert!(thread_info.is_none());
*thread_info = Some(ThreadInfo { stack_guard, thread });
});
}

@alexcrichton
Copy link
Member Author

@bors: r-

That seems likely, yes, I will work on investigating.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 17, 2021
@alexcrichton
Copy link
Member Author

@bors: r=m-ou-se rollup=never

Hm ok so I can't reproduce this locally and the IR looks the same before/after this change, so I'm going to put this back in the queue, but I'll exclude it from rollups in case it's still the culprit.

@bors
Copy link
Contributor

bors commented Nov 17, 2021

📌 Commit 1ac5d7d has been approved by m-ou-se

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 17, 2021
@bors
Copy link
Contributor

bors commented Nov 18, 2021

⌛ Testing commit 1ac5d7d with merge 66fdedc993e987dce337cbd88324cff0b3488559...

@bors
Copy link
Contributor

bors commented Nov 18, 2021

💥 Test timed out

@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 Nov 18, 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)

@alexcrichton
Copy link
Member Author

It looks like dist-aarch64-apple took over an hour to rebuild LLVM, so presumably caches were being primed.

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

bors commented Nov 18, 2021

⌛ Testing commit 1ac5d7d with merge 548c108...

@bors
Copy link
Contributor

bors commented Nov 19, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 548c108 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 19, 2021
@bors bors merged commit 548c108 into rust-lang:master Nov 19, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 19, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (548c108): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@alexcrichton alexcrichton deleted the tweak-const branch November 30, 2021 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbdd0121 nbdd0121 nbdd0121 approved these changes

Assignees

@m-ou-se m-ou-se

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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.