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.

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

Refactor stack overflow handling #123265

Merged
merged 4 commits into from
Apr 1, 2024
Merged

Conversation

joboet
Copy link
Member

@joboet joboet commented Mar 31, 2024

Currently, every platform must implement a Guard that protects a thread from stack overflow. However, UNIX is the only platform that actually does so. Windows has a different mechanism for detecting stack overflow, while the other platforms don't detect it at all. Also, the UNIX stack overflow handling is split between sys::pal::unix::stack_overflow, which implements the signal handler, and sys::pal::unix::thread, which detects/installs guard pages.

This PR cleans this by getting rid of Guard and unifying UNIX stack overflow handling inside stack_overflow (commit 1). Therefore we can get rid of sys_common::thread_info, which stores Guard and the current Thread handle and move the thread::current TLS variable into thread (commit 2).

The second commit is not strictly speaking necessary. To keep the implementation clean, I've included it here, but if it causes too much noise, I can split it out without any trouble.

@joboet joboet added A-thread-locals Area: Thread local storage (TLS) A-thread Area: `std::thread` A-stack-probe Area: Stack probing and guard pages labels Mar 31, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 31, 2024

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-hermit Operating System: Hermit O-itron Operating System: ITRON O-SGX Target: SGX O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 31, 2024
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

I'm very much in favour of containing platform specific stuff within platform specific directories. I'm less opinionated on where exactly to put the guard page handling but, sure, stack_overflow.rs makes sense to me.

Anyways this looks good to me, just a small nit

library/std/src/rt.rs Outdated Show resolved Hide resolved
@ChrisDenton
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 1, 2024

📌 Commit d7b55e4 has been approved by ChrisDenton

It is now in the queue for this repository.

@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 Apr 1, 2024
@bors
Copy link
Contributor

bors commented Apr 1, 2024

⌛ Testing commit d7b55e4 with merge c518e5a...

@bors
Copy link
Contributor

bors commented Apr 1, 2024

☀️ Test successful - checks-actions
Approved by: ChrisDenton
Pushing c518e5a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 1, 2024
@bors bors merged commit c518e5a into rust-lang:master Apr 1, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 1, 2024
@joboet joboet deleted the guardians_of_the_unix branch April 1, 2024 16:45
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c518e5a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
7.8% [7.8%, 7.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 7.8% [7.8%, 7.8%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.3% [2.3%, 2.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.0%, 1.2%] 13
Regressions ❌
(secondary)
1.1% [0.2%, 1.4%] 39
Improvements ✅
(primary)
-0.0% [-0.0%, -0.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [-0.0%, 1.2%] 14

Bootstrap: 667.497s -> 668.277s (0.12%)
Artifact size: 315.72 MiB -> 315.71 MiB (-0.00%)

@Kobzol
Copy link
Contributor

Kobzol commented Apr 1, 2024

The binary size regressions are for actual binaries, not just metadata, which is a bit unfortunate. Was this expected?

@joboet
Copy link
Member Author

joboet commented Apr 1, 2024

That is extremely weird, because this PR didn't really add any new code, it mainly just moved things around.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 1, 2024

./target/release/collector binary_stats \
    +6bb6b816bfcf9e26fac5175e3e907dcefe5ecdbc \
    --rustc2 +c518e5aeecd3421a8ed5acb96946badb217dc64f \
    --include helloworld \
    --profile Opt \
    --backend Llvm
┌─────────────────────┬───────────────┬──────────────┬───────┬──────────┐
│ Section             │ Size (before) │ Size (after) │  Diff │ Diff (%) │
├─────────────────────┼───────────────┼──────────────┼───────┼──────────┤
│ .text               │    245.96 KiB │   246.67 KiB │  +720 │    +0.3% │
│ .strtab             │     47.13 KiB │    47.81 KiB │  +700 │    +1.5% │
│ .rela.dyn           │     17.04 KiB │    17.44 KiB │  +408 │    +2.3% │
│ .data.rel.ro        │      8.95 KiB │     9.23 KiB │  +280 │    +3.1% │
│ .eh_frame           │     21.14 KiB │    21.39 KiB │  +256 │    +1.2% │
│ .symtab             │     17.46 KiB │    17.70 KiB │  +240 │    +1.3% │
│ .rodata             │     24.20 KiB │    24.03 KiB │  -176 │    -0.7% │
│ .eh_frame_hdr       │      3.89 KiB │     3.95 KiB │   +56 │    +1.4% │
│ .dynstr             │      1.02 KiB │     1.04 KiB │   +26 │    +2.5% │
│ .dynsym             │      1.57 KiB │     1.59 KiB │   +24 │    +1.5% │
│ .got                │      1.67 KiB │     1.69 KiB │   +16 │    +0.9% │
│ .gcc_except_table   │      4.06 KiB │     4.07 KiB │   +12 │    +0.3% │
│ .bss                │         216 B │        208 B │    -8 │    -3.7% │
│ .tbss               │          80 B │         73 B │    -7 │    -8.8% │
│ .gnu.version        │         134 B │        136 B │    +2 │    +1.5% │
│ <17 unchanged rows> │      1.59 KiB │     1.59 KiB │     0 │     0.0% │
│─────────────────────│───────────────│──────────────│───────│──────────│
│ Total               │    396.12 KiB │   398.61 KiB │ +2549 │    +0.6% │
└─────────────────────┴───────────────┴──────────────┴───────┴──────────┘

┌───────────────────────────────────────────────────────────────┬───────────────┬──────────────┬──────┬──────────┐
│ Symbol                                                        │ Size (before) │ Size (after) │ Diff │ Diff (%) │
├───────────────────────────────────────────────────────────────┼───────────────┼──────────────┼──────┼──────────┤
│ std::sys::pal::unix::stack_overflow::imp::make_handler        │         464 B │        903 B │ +439 │   +94.6% │
│ core::cell::once::OnceCell<T>::get_or_try_init::outlined_call │           0 B │        382 B │ +382 │  +100.0% │
│ std::sys::pal::unix::thread::Thread::get_name                 │         371 B │          0 B │ -371 │  -100.0% │
│ std::sys_common::thread_info::set                             │         323 B │          0 B │ -323 │  -100.0% │
│ std::sys::pal::unix::stack_overflow::imp::signal_handler      │         685 B │        420 B │ -265 │   -38.7% │
│ std::sys_common::thread_info::current_thread                  │         259 B │          0 B │ -259 │  -100.0% │
│ std::thread::try_current                                      │           0 B │        258 B │ +258 │  +100.0% │
│ std::thread::set_current                                      │           0 B │        249 B │ +249 │  +100.0% │
│ core::fmt::builders::DebugStruct::finish_non_exhaustive       │           0 B │        182 B │ +182 │  +100.0% │
│ <std::thread::Thread as core::fmt::Debug>::fmt                │           0 B │        158 B │ +158 │  +100.0% │
│ <&T as core::fmt::Debug>::fmt                                 │         465 B │        558 B │  +93 │   +20.0% │
│ <core::option::Option<T> as core::fmt::Debug>::fmt            │           0 B │         80 B │  +80 │  +100.0% │
│ std::sys_common::thread_info::THREAD_INFO::__getit::destroy   │          68 B │          0 B │  -68 │  -100.0% │
│ std::thread::CURRENT::__getit::destroy                        │           0 B │         63 B │  +63 │  +100.0% │
│ <std::thread::ThreadId as core::fmt::Debug>::fmt              │           0 B │         53 B │  +53 │  +100.0% │
│ std::sys_common::thread_info::THREAD_INFO::__getit::VAL       │          32 B │          0 B │  -32 │  -100.0% │
│ core::ptr::drop_in_place<std::thread::Thread>                 │           0 B │         21 B │  +21 │  +100.0% │
│ std::sys::pal::unix::stack_overflow::imp::GUARD::__getit::VAL │           0 B │         16 B │  +16 │  +100.0% │
│ std::rt::lang_start_internal                                  │      1.73 KiB │     1.72 KiB │  -14 │    -0.8% │
│ std::sys::pal::unix::thread::guard::PAGE_SIZE                 │           8 B │          0 B │   -8 │  -100.0% │
│ std::thread::CURRENT::__getit::VAL                            │           0 B │          8 B │   +8 │  +100.0% │
│ std::sys::pal::unix::stack_overflow::imp::PAGE_SIZE           │           0 B │          8 B │   +8 │  +100.0% │
│ std::sys::sync::once::futex::Once::call                       │      1.96 KiB │     1.95 KiB │   -7 │    -0.3% │
│ std::sys_common::thread_info::THREAD_INFO::__getit::STATE     │           1 B │          0 B │   -1 │  -100.0% │
│ std::thread::CURRENT::__getit::STATE                          │           0 B │          1 B │   +1 │  +100.0% │
│ <689 unchanged rows>                                          │    243.75 KiB │   243.75 KiB │    0 │     0.0% │
│───────────────────────────────────────────────────────────────│───────────────│──────────────│──────│──────────│
│ Total                                                         │    250.05 KiB │   250.70 KiB │ +663 │    +0.3% │
└───────────────────────────────────────────────────────────────┴───────────────┴──────────────┴──────┴──────────┘

@joboet
Copy link
Member Author

joboet commented Apr 2, 2024

Nearly all of that bloat is debug formatting. Filed #123356 to address what I believe to be the cause.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 2, 2024
Reduce code size of `thread::set_current`

rust-lang#123265 introduced a rather large binary size regression, because it added an `unwrap()` call on a `Result<(), Thread>`, which in turn pulled its rather heavy `Debug` implementation. This PR fixes this by readding the `rtassert!` that was removed.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 2, 2024
Reduce code size of `thread::set_current`

rust-lang#123265 introduced a rather large binary size regression, because it added an `unwrap()` call on a `Result<(), Thread>`, which in turn pulled its rather heavy `Debug` implementation. This PR fixes this by readding the `rtassert!` that was removed.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 3, 2024
…bilee

Reduce code size of `thread::set_current`

rust-lang#123265 introduced a rather large binary size regression, because it added an `unwrap()` call on a `Result<(), Thread>`, which in turn pulled its rather heavy `Debug` implementation. This PR fixes this by readding the `rtassert!` that was removed.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 4, 2024
…nton

Reduce code size of `thread::set_current`

rust-lang#123265 introduced a rather large binary size regression, because it added an `unwrap()` call on a `Result<(), Thread>`, which in turn pulled its rather heavy `Debug` implementation. This PR fixes this by readding the `rtassert!` that was removed.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 4, 2024
…nton

Reduce code size of `thread::set_current`

rust-lang#123265 introduced a rather large binary size regression, because it added an `unwrap()` call on a `Result<(), Thread>`, which in turn pulled its rather heavy `Debug` implementation. This PR fixes this by readding the `rtassert!` that was removed.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
Rollup merge of rust-lang#123356 - joboet:set_current_size, r=ChrisDenton

Reduce code size of `thread::set_current`

rust-lang#123265 introduced a rather large binary size regression, because it added an `unwrap()` call on a `Result<(), Thread>`, which in turn pulled its rather heavy `Debug` implementation. This PR fixes this by readding the `rtassert!` that was removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stack-probe Area: Stack probing and guard pages A-thread Area: `std::thread` A-thread-locals Area: Thread local storage (TLS) merged-by-bors This PR was explicitly merged by bors. O-hermit Operating System: Hermit O-itron Operating System: ITRON O-SGX Target: SGX O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows 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
Development

Successfully merging this pull request may close these issues.

8 participants