Make TLS accesses explicit in MIR #71192
Conversation
rust-highfive
added
the
S-waiting-on-review
|
Is there a good place where Miri-the-engine could assert that the pointers it encounters in constants do not point to thread-locals? Those Or even better, can we make it so that a thread-local static does not have a |
What I mean by this is that |
src/librustc_codegen_llvm/common.rs
Outdated
| @@ -260,6 +261,7 @@ impl ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> { | |||
| Some(GlobalAlloc::Function(fn_instance)) => self.get_fn_addr(fn_instance), | |||
| Some(GlobalAlloc::Static(def_id)) => { | |||
| assert!(self.tcx.is_static(def_id)); | |||
| assert!(!self.tcx.has_attr(def_id, sym::thread_local)); | |||
There was a problem hiding this comment.
You added this to a lot of places that fetch from the alloc_map; any reason you didn't add it at the place where things get inserted?
There was a problem hiding this comment.
the insertion functions themselves don't have access to the tcx. I could do it at the call sites, but since the only interesting call site is the one in librustc_mir_build, and that one already checks for thread locals and explicitly does not insert, an additional assertion would make no sense.
There was a problem hiding this comment.
Ah, that's a shame, but makes sense. Maybe just add a comment there then?
I am happy this assertion holds at all. :D
There was a problem hiding this comment.
I could use thread locals in create_static_alloc, but that seems odd and overkill.
Alternatively I can make the AllocMap private to TyCtxt and add functions to TyCtxt that forward to the AllocMap. That will cause a lot of churn, so I'd rather not do it in this PR.
There was a problem hiding this comment.
A comment in create_static_alloc sounds enough.
There was a problem hiding this comment.
I did the full thing in #71508
There was a problem hiding this comment.
You mean, adding a comment? I cannot see it there, am I looking in the wrong place?
There was a problem hiding this comment.
No, I mean the forwarding of the functions. Once the other PR is merged, I can rebase this PR and add the assertion in create_static_alloc
There was a problem hiding this comment.
Oh I see.
Presumably, alloc_map.create_static_alloc also still exists though, and its doc comment should ideally mention this condition as well.
There was a problem hiding this comment.
It does not. There's only the TyCtxt method, which forwards to reserve_and_set_dedup, which is the place that will actually be doing the checking.
|
cc @nikomatsakis @pnkfelix @matthewjasper for the borrowck side of this (we used to have some hacks for thread-local |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #71049) made this pull request unmergeable. Please resolve the merge conflicts. |
| _ecx: &mut InterpCx<'mir, 'tcx, Self>, | ||
| did: DefId, | ||
| ) -> InterpResult<'tcx, AllocId> { | ||
| throw_unsup!(ThreadLocalStatic(did)) |
There was a problem hiding this comment.
I thought about errors in these default implementations again. Currently, we are inconsistent.
int_to_ptr and abort (and with this PR thread_local_alloc_id) have default implementations that error "unsupported".
binary_ptr_op, box_alloc, ptr_to_int also error in both const-eval and const-prop, but with separately implemented error messages.
I am not sure what's better, but it seems odd not to be consistent here.
|
@oli-obk I am wondering if you think an MCP would be appropriate for this PR? It seems to me like 'probably but not necessarily' -- a quick skim of the rustc-dev-guide, for example, suggests that it's just below the level of detail at which we document the MIR. That said, I think I at least would appreciate a few paragraphs of summary of what the high-level details of the approach are, and I think they'd probably make a nice addition to the rustc-dev-guide. Context: I've twice now clicked to view the diffs and I realize I'm spending most of the time just doing a "first pass" to try and extract the high-level view of 'what the heck is going on here'. I have some guesses but I don't know, and it'd be nicer if I didn't have to do that. |
|
I know FCP and RFC, what's MCP? |
|
major changes proposal: rust-lang/compiler-team#209 |
oli-obk
force-pushed
the
eager_alloc_id_canonicalization
branch
from
a11f92b to
081ad6c
Compare
|
What is the status of this? |
Fix #[thread_local] statics as asm! sym operands The `asm!` RFC specifies that `#[thread_local]` statics may be used as `sym` operands for inline assembly. This also fixes a regression in the handling of `#[thread_local]` during monomorphization which caused link-time errors with multiple codegen units, most likely introduced by rust-lang#71192. r? @oli-obk
Fix link error with #[thread_local] introduced by rust-lang#71192 r? @oli-obk
Fix link error with #[thread_local] introduced by rust-lang#71192 r? @oli-obk
Fix link error with #[thread_local] introduced by rust-lang#71192 r? @oli-obk
Fix link error with #[thread_local] introduced by rust-lang#71192 r? @oli-obk
Fix link error with #[thread_local] introduced by rust-lang#71192 r? @oli-obk
Fix link error with #[thread_local] introduced by rust-lang#71192 r? @oli-obk
Fix link error with #[thread_local] introduced by rust-lang#71192 r? @oli-obk
Fix link error with #[thread_local] introduced by rust-lang#71192 r? @oli-obk
…ulacrum [beta] backports This PR backports the following: * Make novel structural match violations not a `bug` rust-lang#73446 * linker: Never pass `-no-pie` to non-gnu linkers rust-lang#73384 * Disable the `SimplifyArmIdentity` pass rust-lang#73262 * Allow inference regions when relating consts rust-lang#73225 * Fix link error with #[thread_local] introduced by rust-lang#71192 rust-lang#73065 * Ensure stack when building MIR for matches rust-lang#72941 * Don't create impl candidates when obligation contains errors rust-lang#73005 r? @ghost
r? @rust-lang/wg-mir-opt
cc @RalfJung @vakaras for miri thread locals
cc @bjorn3 for cranelift
fixes #70685