Skip to content

zend call stack fixing stack limit for macOs arm64. #13319

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

Closed
wants to merge 1 commit into from

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented Feb 3, 2024

8MB sounded a prudent size for older 10.9 macOs release, however with newer mac with arm64, it triggers a stack overflow.

@devnexen devnexen requested a review from iluuu1994 as a code owner February 3, 2024 15:33
@devnexen devnexen requested review from arnaud-lb and removed request for iluuu1994 February 3, 2024 15:33
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Thank you! The code change looks good to me, but the new comments are redundant with the existing comment bellow them.

We could make the change self-explanatory by merging the code paths. For example (in pseudo code) :

if (pthread_main_np() && !arm64) {
    /* existing comment explaining the workaround for older OSX versions */
    max_size = 8 * 1024 * 1024;
} else {
    max_size = pthread_get_stacksize_np(pthread_self());
}

@devnexen devnexen force-pushed the apple_arm64_stacksize branch from 04cd85a to 8165853 Compare February 5, 2024 11:38
@divinity76
Copy link
Contributor

divinity76 commented Feb 5, 2024

#ifndef __aarch64__
	/* pthread_get_stacksize_np() returns a too low value for the main
	 * thread in OSX 10.9, 10.10:
	 * https://mail.openjdk.org/pipermail/hotspot-dev/2013-October/011353.html
	 */
	if (pthread_main_np()) {
		max_size = 8 * 1024 * 1024;
	} else
#endif
	max_size = pthread_get_stacksize_np(pthread_self());

should be slightly faster on modern macos

edit: fixed a logic error

8MB sounded a prudent size for older 10.9 macOs release, however
with newer mac with arm64, it triggers a stack overflow.
@devnexen devnexen force-pushed the apple_arm64_stacksize branch from 8165853 to 7f770c1 Compare February 5, 2024 12:39
@divinity76
Copy link
Contributor

LGTM 👍

@devnexen devnexen closed this in b320aab Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants