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

Rare case of not properly rounding up thread stack size on Windows #94454

Closed
AronParker opened this issue Feb 28, 2022 · 3 comments · Fixed by #94618
Closed

Rare case of not properly rounding up thread stack size on Windows #94454

AronParker opened this issue Feb 28, 2022 · 3 comments · Fixed by #94618
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@AronParker
Copy link
Contributor

AronParker commented Feb 28, 2022 •
edited by rustbot
Loading

The following code states that stack_size rounds up to the next 64 kB:

rust/library/std/src/sys/windows/thread.rs

Lines 31 to 33 in 97cde9f

// Round up to the next 64 kB because that's what the NT kernel does,
// might as well make it explicit.
let stack_size = (stack + 0xfffe) & (!0xfffe);

However, if stack is any of 0xffff * x + 1 (where x is a non-negative number) it will not properly round up. Additionally, the least significant byte is retained, I doubt that is intended either. Am I missing something? Otherwise I believe it's meant to be 0xffff instead of 0xfffe to properly round up.
@ChrisDenton
Copy link
Member

To be honest, I think this line may as well be removed. As the comment says, the OS will do the rounding up in any case. And as the MSDN docs say:

The operating system rounds up the specified size to the nearest multiple of the system's allocation granularity (typically 64 KB).

So 64 KB isn't technically guaranteed. I'm not sure there's anything to be gained by doing the rounding up ourselves.

@ChrisDenton
Copy link
Member

@rustbot label +O-windows +T-libs

@rustbot rustbot added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 28, 2022
@yaahc yaahc added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Mar 1, 2022
@Rustin170506

This comment was marked as outdated.

Sign in to view
@Rustin170506 Rustin170506 removed their assignment Mar 2, 2022
@lewisclark lewisclark mentioned this issue Mar 4, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 4, 2022
@Dylan-DPC
Rollup merge of rust-lang#94618 - lewisclark:remove-stack-size-roundi…
5d038c3 
…ng, r=yaahc

Don't round stack size up for created threads in Windows

Fixes rust-lang#94454

Windows does the rounding itself, so there isn't a need to explicity do the rounding beforehand, as mentioned by `@ChrisDenton` in rust-lang#94454

> The operating system rounds up the specified size to the nearest multiple of the system's allocation granularity (typically 64 KB). To retrieve the allocation granularity of the current system, use the [GetSystemInfo](https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsysteminfo) function.

https://docs.microsoft.com/en-us/windows/win32/procthread/thread-stack-size
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 4, 2022
@Dylan-DPC
Rollup merge of rust-lang#94618 - lewisclark:remove-stack-size-roundi…
9fc3109 
…ng, r=yaahc

Don't round stack size up for created threads in Windows

Fixes rust-lang#94454

Windows does the rounding itself, so there isn't a need to explicity do the rounding beforehand, as mentioned by ``@ChrisDenton`` in rust-lang#94454

> The operating system rounds up the specified size to the nearest multiple of the system's allocation granularity (typically 64 KB). To retrieve the allocation granularity of the current system, use the [GetSystemInfo](https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsysteminfo) function.

https://docs.microsoft.com/en-us/windows/win32/procthread/thread-stack-size
@bors bors closed this as completed in 629e7aa Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't round stack size up for created threads in Windows lewisclark/rust
5 participants
@yaahc @AronParker @ChrisDenton @Rustin170506 @rustbot