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

Set stack size on MUSL with PT_GNU_STACK header #92465

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

MichalStrehovsky
Copy link
Member

Fixes #72920.

Cc @dotnet/ilc-contrib

@ghost
Copy link

ghost commented Sep 22, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #72920.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr

Milestone: -

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -156,6 +156,7 @@ The .NET Foundation licenses this file to you under the MIT license.
<LinkerArg Include="-Wl,-z,relro" Condition="'$(_IsApplePlatform)' != 'true'" />
<!-- binskim warning BA3011 The BIND_NOW flag is missing -->
<LinkerArg Include="-Wl,-z,now" Condition="'$(_IsApplePlatform)' != 'true'" />
<LinkerArg Include="-Wl,-z,stack-size=1572864" Condition="'$(_linuxLibcFlavor)' == 'musl'" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<LinkerArg Include="-Wl,-z,stack-size=1572864" Condition="'$(_linuxLibcFlavor)' == 'musl'" />
<LinkerArg Include="-Wl,-z,stack-size=$(IlcDefaultStackSize)" Condition="'$(IlcDefaultStackSize)' != ''" />

And set the default IlcDefaultStackSize for musl earlier in this file?

It would provide equivalent of DOTNET_DefaultStackSize config in case somebody needs it.

# Fix up the thread stack size for MUSL to more reasonable size.
# TODO: https://github.com/dotnet/runtimelab/issues/791
add_definitions(-DENSURE_PRIMARY_STACK_SIZE)
endif ()
Copy link
Member

Choose a reason for hiding this comment

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

we have the same in mono, I'm not sure I understand how this is fixed for everything but nativeaot?

Copy link
Member

Choose a reason for hiding this comment

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

The issue was specific to native AOT.

We have the same ENSURE_PRIMARY_STACK_SIZE in CoreCLR too, however it is not straightforward to get rid of it there since CoreCLR allows configuring the stack size using env variable.

Copy link
Member

Choose a reason for hiding this comment

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

Deleting the TODO in #92597

@MichalStrehovsky MichalStrehovsky deleted the musl branch September 26, 2023 23:04
@ghost ghost locked as resolved and limited conversation to collaborators Oct 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set minimum stack size using PT_GNU_STACK header for Musl
4 participants