Skip to content

pcre2test: print max stack size in the right units #171

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

Merged
merged 1 commit into from
Dec 11, 2022

Conversation

carenas
Copy link
Contributor

@carenas carenas commented Dec 7, 2022

on a side note; do we really need to test with 64MB of stack nowadays? if I recall correctly the limit might had been increased because of some platforms that couldn't run without it (maybe Linux/m68k) but that was before 10.30 where the possibility of stack overflows was more likely.

AFAIK macOS uses 8M by default and was failing the -S call because the maximum allowed is a little less than 64M and therefore has been running fine with only that much (even before the latest improvements in 10.41)

if anything, and considering the trend to have multiple threads I would expect we should be instead looking at restricting the stack size (less than 1MB per thread stack size is IMHO common).

@carenas
Copy link
Contributor Author

carenas commented Dec 7, 2022

FWIW successfully tested 10.41 in a Solaris 11 system built with Developer Studio 12.6 WITHOUT setting -M 64 to pcre2test

@PhilipHazel PhilipHazel merged commit 02b196d into PCRE2Project:master Dec 11, 2022
PhilipHazel added a commit that referenced this pull request Dec 11, 2022
@PhilipHazel
Copy link
Collaborator

This patch wasn't quite right for cases where the hard limit is not a whole number of MiB. I have done some more work so that now the value is shown in MiB if it is a whole number of MiB, else in KiB if it is a whole number of KiB - which is most likely since "ulimit -s" works in KiB - or else in bytes. I find I still need a large stack in order to run the tests when compiling with Clang's sanitize options.

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