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

Decrease SetThreadStackGuarantee value #1044

Closed
wants to merge 2 commits into from

Conversation

stima
Copy link
Contributor

@stima stima commented Sep 27, 2024

Decrease guaranteed stack size to smaller value to be able execute chromium embedded task.

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.71%. Comparing base (dbb9580) to head (88c7b35).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1044      +/-   ##
==========================================
- Coverage   83.33%   81.71%   -1.63%     
==========================================
  Files          53       53              
  Lines        6338     6338              
  Branches     1198     1198              
==========================================
- Hits         5282     5179     -103     
- Misses       1042     1046       +4     
- Partials       14      113      +99     

@supervacuus
Copy link
Collaborator

Hi @stima! Thanks for the PR. I understand what you want to do here, but I wonder if this is the right approach. How about we introduce a similar contract as with the sigaltstack for UNIX signal handling and only set an expected stack size if the client didn't specify one for the thread up to that point?

We can also provide a compile-time option to turn off the convenience per-thread setting via DllMain so that you can fully control your configuration without ours interfering. Lowering the stack size to 4 pages will likely break use cases with more involved crash hooks, which are often necessary for downstream SDKs.

@stima
Copy link
Contributor Author

stima commented Oct 2, 2024

I wanted to keep it simple (although increasing to 32k might be sufficient for CEF). However, my best guess is that only the client knows the size they are willing to provide, so autosetting disabling is best option as for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants