Fix panic when restricted-std is enabled.#104971
Fix panic when restricted-std is enabled.#104971avafloww wants to merge 1 commit intorust-lang:masterfrom
panic when restricted-std is enabled.#104971Conversation
When `restricted-std` is in use, we do not have access to thread local storage, as `sys/unsupported` does not implement it. This previously would cause an infinite panic loop, as `sys/unsupported/thread_local_key.rs` would panic upon attempting to increment the local thread's panic count.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon. Please see the contribution instructions for more information. |
rustbot
added
S-waiting-on-review
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
| // In a restricted_std environment, we likely don't have thread_local. | ||
| // If we attempt to use LOCAL_PANIC_COUNT here, it is probable that another panic will | ||
| // occur, sending us into an infinite panic loop that never calls the panic handler. | ||
| #[cfg(not(feature = "restricted-std"))] | ||
| if !must_abort { | ||
| panics = LOCAL_PANIC_COUNT.with(|c| { |
There was a problem hiding this comment.
What does "likely" mean here? Your PR description says
When restricted-std is in use, we do not have access to thread local storage, as sys/unsupported does not implement it.
Is that always the case when using restricted-std? and if not, when can the behavior be different?
There was a problem hiding this comment.
Looking at it a bit more, it seems it's true most of the time, but not necessarily always. The restricted-std requirement is defined here if a platform match is not found, and since thread_local is unimplemented in sys/unsupported, a panic loop occurs.
There do appear to be some cases where this may not always be true - i.e. in the case of aarch64-apple-tvos, which is not allowed to use regular unrestricted std, despite being part of the unix family and thus using the unix system module which does properly implement thread-local support. I would imagine this same situation would also apply to any of the other targets which are called out in the same area in build.rs and also use a non-unsupported system module.
I'm not really sure how big of an impact this would be; since restricted-std is infectious, requiring itself to be in every std-dependent crate throughout the entire dependency tree in order to be used, it seems like a minor detail, especially since a double panic on these platforms will still trigger an abort via the global panic counter.
|
I worry that this will just cause a panic loop in a different way, because double-panics no longer abort the process. Maybe we should force enable |
|
Isn't restricted-std meant to be enabled for cases where libstd isn't allowed to be used and thus doesn't define the panic handler in the first place. |
|
Sorry for taking a few days to respond - work has been busy! 😅
@jyn514 The I wonder if there's any way to effectively unit-test this? Seems a bit tricky, but I'm not too familiar with the repo...
@bjorn3 The panic handler is still defined in a |
|
restricted-std was introduced with |
|
r? @bjorn3 |
anden3
added
S-waiting-on-author
|
Hello @avafloww! I pinged you as part of the triage procedure because this PR has received a review :) |
|
Ping from triage: I'm closing this due to inactivity, last time was november 2022 @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
7 participants
When
restricted-stdis in use, we do not have access to thread local storage, assys/unsupporteddoes not implement it. This previously would cause an infinite panic loop, assys/unsupported/thread_local_key.rswould panic upon attempting to increment the local thread's panic count.