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

Check that nested statics in thread locals are duplicated per thread. #123362

Merged
merged 2 commits into from
Apr 2, 2024
Merged

Check that nested statics in thread locals are duplicated per thread. #123362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c5c48e
Check that nested statics in thread locals are duplicated per thread.
oli-obk Apr 2, 2024
64b75f7
Forbid implicit nested statics in thread local statics
oli-obk Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_const_eval/messages.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ const_eval_mut_deref =

const_eval_mutable_ptr_in_final = encountered mutable pointer in final value of {const_eval_intern_kind}

const_eval_nested_static_in_thread_local = #[thread_local] does not support implicit nested statics, please create explicit static items and refer to them instead
const_eval_non_const_fmt_macro_call =
cannot call non-const formatting macro in {const_eval_const_context}s

Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_const_eval/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ pub(crate) struct DanglingPtrInFinal {
pub kind: InternKind,
}

#[derive(Diagnostic)]
#[diag(const_eval_nested_static_in_thread_local)]
pub(crate) struct NestedStaticInThreadLocal {
#[primary_span]
pub span: Span,
}

#[derive(LintDiagnostic)]
#[diag(const_eval_mutable_ptr_in_final)]
pub(crate) struct MutablePtrInFinal {
Expand Down
6 changes: 5 additions & 1 deletion compiler/rustc_const_eval/src/interpret/intern.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use rustc_span::sym;

use super::{AllocId, Allocation, InterpCx, MPlaceTy, Machine, MemoryKind, PlaceTy};
use crate::const_eval;
use crate::errors::{DanglingPtrInFinal, MutablePtrInFinal};
use crate::errors::{DanglingPtrInFinal, MutablePtrInFinal, NestedStaticInThreadLocal};

pub trait CompileTimeMachine<'mir, 'tcx: 'mir, T> = Machine<
'mir,
Expand Down Expand Up @@ -108,6 +108,10 @@ fn intern_as_new_static<'tcx>(
);
tcx.set_nested_alloc_id_static(alloc_id, feed.def_id());

if tcx.is_thread_local_static(static_id.into()) {
tcx.dcx().emit_err(NestedStaticInThreadLocal { span: tcx.def_span(static_id) });
}

// These do not inherit the codegen attrs of the parent static allocation, since
// it doesn't make sense for them to inherit their `#[no_mangle]` and `#[link_name = ..]`
// and the like.
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/statics/nested_thread_local.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Check that we forbid nested statics in `thread_local` statics.

#![feature(const_refs_to_cell)]
#![feature(thread_local)]

#[thread_local]
Copy link
Member

Choose a reason for hiding this comment

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

Does this not also affect thread_local!() macro invokations? It should, since those just desugar to #[thread_local] internally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but those always desugar to a Key<StaticsType>, which is always initialized with None internally, and then set to Some by computing a value at runtime at thread start.

Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at the implementation of thread_local_inner in library/std/src/sys/thread_local/fast_local.rs, which initializes:

#[thread_local]
// We use `UnsafeCell` here instead of `static mut` to ensure any generated TLS shims
// have a nonnull attribute on their return value.
static VAL: $crate::cell::UnsafeCell<$t> = $crate::cell::UnsafeCell::new(INIT_EXPR);

Copy link
Member

Choose a reason for hiding this comment

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

Notice INIT_EXPR, not $init_expr. It is first stored in a const and then assigned here.

Copy link
Member

Choose a reason for hiding this comment

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

🤦 of course!

static mut FOO: &u32 = {
//~^ ERROR: does not support implicit nested statics
// Prevent promotion (that would trigger on `&42` as an expression)
let x = 42;
&{ x }
};

fn main() {}
8 changes: 8 additions & 0 deletions tests/ui/statics/nested_thread_local.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: #[thread_local] does not support implicit nested statics, please create explicit static items and refer to them instead
--> $DIR/nested_thread_local.rs:7:1
|
LL | static mut FOO: &u32 = {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to extract a more exact span from the allocation? Specifically, I'd like a label on the &{ x } part that says "this is promoted" or something.

Copy link
Member

Choose a reason for hiding this comment

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

Miri extends the interpreter with more detailed tracking of where each allocation comes from, but so far the compile-time interpreter does not have that information. Maybe we should add a Span to the core allocation type rather than just doing this as a Miri extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just wondering about that when I saw this error. We can see how perf is affected, but it would be really nice for many diagnostics

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... but in a separate PR, please. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lmao yes

| ^^^^^^^^^^^^^^^^^^^^

error: aborting due to 1 previous error