zend call stack fix freebsd code path. #11766

devnexen · 2023-07-22T14:55:45Z

The typo in HAVE_PTHREAD_ATTR_GET_STACK (might be due to pthread_attr_get_np being different from Linux's pthread_getattr_np) led to this code path never get called on FreeBSD. Anyway, like Linux the first condition alone is enough, pthread_attr_getstack being present
even in the 9.x release's serie.

petk · 2023-07-22T15:28:58Z

Ah, now I understand the comment in #11762... It's a typo and the HAVE_PTHREAD_ATTR_GET_STACK is used in the code instead of the HAVE_PTHREAD_ATTR_GETSTACK. I have no idea how I've missed that. I've missed that pthread_attr_getstack is actually used. 😀

Then why not just changing the constant name to HAVE_PTHREAD_ATTR_GETSTACK? I think it would be more correct since theoretically it might not be defined on some system. It's not part of any standard or docs and it doesn't hurt to be as thorough as possible.

Edit: This changes the execution of the code then. Now, this code is not executed on any platform... 🤔

devnexen · 2023-07-22T15:37:02Z

Not sure I get what you mean this part of the code is exclusive to freebsd.

devnexen · 2023-07-22T15:44:57Z

Anyway cc @arnaud-lb since he s the author of the FreeBSD's implementation :)

petk · 2023-07-22T15:46:59Z

Yes, but the HAVE_PTHREAD_ATTR_GET_STACK is not defined anywhere so only that part of the code is not executed. Since the pthread_attr_getstack is already called in zend_call_stack_get_linux_pthread without the check it probably is fine to do that, yes.

I've changed my PR now so only one change is done. Then can you remove also pthread_attr_getstack check in Zend.m4?

devnexen · 2023-07-22T15:51:44Z

ah sure I did not see your PR change.

arnaud-lb

Nice catch, thank you!

You also add -lpthread, is this on purpose? Should we avoid linking to pthread when it's not necessary? (in non-ZTS builds) Can it affect SAPIs in any way?

arnaud-lb · 2023-07-22T17:35:41Z

Zend/Zend.m4

@@ -212,6 +212,7 @@ AX_CHECK_COMPILE_FLAG([-Wlogical-op], CFLAGS="-Wlogical-op $CFLAGS", , [-Werror]
 AX_CHECK_COMPILE_FLAG([-Wformat-truncation], CFLAGS="-Wformat-truncation $CFLAGS", , [-Werror])
 AX_CHECK_COMPILE_FLAG([-Wstrict-prototypes], CFLAGS="-Wstrict-prototypes $CFLAGS", , [-Werror])
 AX_CHECK_COMPILE_FLAG([-fno-common], CFLAGS="-fno-common $CFLAGS", , [-Werror])
+AX_CHECK_COMPILE_FLAG([-pthread], CFLAGS="-pthread $CFLAGS", , [-Werror])


It this change on purpose?

yes not much choice.

oh now I remember why I checked for the availability of both pthread_attr_get_np and pthread_attr_getstack: The libc provides a stub for pthread_attr_get_np, but not for pthread_attr_getstack, when not linking with pthread.

There are stubs for pthread_attr_getstackaddr and pthread_attr_getstacksize though.

We should check for the availability of pthread_attr_getstack, or switch to pthread_attr_getstackaddr and pthread_attr_getstacksize. After that we can avoid explicitly linking to pthread in non-ZTS builds.

Since pthread_attr_getstackaddr and pthread_attr_getstacksize are deprecated, we should go for checking the availability of pthread_attr_getstack and use it in zend_call_stack_get_freebsd_pthread.

We will compile the stub version of zend_call_stack_get_freebsd_pthread when not linking with pthread, which is fine.

In fact the deprecation warning occured on Linux should I change it too there ?

I see that you have already done it :) It's better if we can avoid depending on deprecated functions.

…_get_np being different from Linux's pthread_getattr_np) led to this code path never get called on FreeBSD.

arnaud-lb · 2023-07-23T14:34:04Z

Zend/zend_call_stack.c

+    error = pthread_attr_getguardsize(&attr, &guard_size);
+    if (error) {
+        return false;
+    }
+
+    addr = (char *)addr + guard_size;
+    max_size -= guard_size;


From reading the code and testing, addr and size reported by pthread_attr_getstack() appear to reflect the usable stack, and they are not affected by the guard size. Based on that, we should not adjust addr and max_size here. But you may have a different experience. Is this required on some FreeBSD version? On Linux this is required in Glibc < 2.8.

In 13.1, the stack is allocated here: https://github.com/freebsd/freebsd-src/blob/releng/13.1/lib/libthr/thread/thr_stack.c#L276. As we can see, the map size is computed from stacksize + guardsize. stacksize and guardsize come directly from the pthread_attr (they are just rounded to the nearest page multiple). pthread_attr_setstack() does not adjust addr and size with the guardsize, and pthread_attr_setguardsize() does not adjust addr and size either. The get versions of these functions also report these values directly.

I verified with this program: https://gist.github.com/arnaud-lb/a53eb410b7c26b38fd01eac2cee0467e. The program starts 3 threads: Thread 1 with the default attrs, thread 2 with the guard size set to 1MiB, and thread 3 with the guard size set to 16MiB (larger than the stack size). Each thread then recurses until it reaches the address reported by pthread_attr_getstack(). If this address is the actual base of the stack, this should not crash. If this address is in or bellow the guard pages, this should crash. During my tests, this did not crash.

The FreeBSD manpage does not help a lot, but it mentions that these functions conform to POSIX.1. The posix spec about pthread_attr_getstack() says this:

All pages within the stack described by stackaddr and stacksize shall be both readable and writable by the thread.

There could still be a bug or a discrepancy in FreeBSD's implementation, so I wouldn't trust the spec alone, but this matches what I was seeing above.

No my mistake even in old releases the guard size is already.

arnaud-lb

Thank you!

The typo in HAVE_PTHREAD_ATTR_GET_STACK (might be due to pthread_attr_get_np being different from Linux's pthread_getattr_np) led to this code path never get called on FreeBSD.

devnexen requested a review from iluuu1994 as a code owner July 22, 2023 14:55

github-actions bot added the Category: Engine label Jul 22, 2023

github-actions bot added the Category: Build System label Jul 22, 2023

petk mentioned this pull request Jul 22, 2023

Remove unused ZEND_STACK_GROWS_DOWNWARDS constant #11762

Merged

arnaud-lb reviewed Jul 22, 2023

View reviewed changes

devnexen force-pushed the zend_call_stack_fbsd_build_fix branch from bb40c12 to 090ed09 Compare July 22, 2023 19:37

The typo in HAVE_PTHREAD_ATTR_GET_STACK (might be due to pthread_attr…

a01a43f

…_get_np being different from Linux's pthread_getattr_np) led to this code path never get called on FreeBSD.

devnexen force-pushed the zend_call_stack_fbsd_build_fix branch from 090ed09 to a01a43f Compare July 22, 2023 21:02

github-actions bot removed the Category: Build System label Jul 22, 2023

arnaud-lb reviewed Jul 23, 2023

View reviewed changes

remove not needed stack size adjustment on freebsd

827fb3f

arnaud-lb approved these changes Jul 23, 2023

View reviewed changes

devnexen merged commit 6602dde into php:master Jul 23, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zend call stack fix freebsd code path. #11766

zend call stack fix freebsd code path. #11766

devnexen commented Jul 22, 2023

petk commented Jul 22, 2023 •

edited

devnexen commented Jul 22, 2023

devnexen commented Jul 22, 2023

petk commented Jul 22, 2023

devnexen commented Jul 22, 2023

arnaud-lb left a comment

arnaud-lb Jul 22, 2023

devnexen Jul 22, 2023

arnaud-lb Jul 22, 2023

arnaud-lb Jul 22, 2023

devnexen Jul 22, 2023

arnaud-lb Jul 23, 2023

arnaud-lb Jul 23, 2023

arnaud-lb Jul 23, 2023

devnexen Jul 23, 2023

devnexen Jul 23, 2023

arnaud-lb left a comment

zend call stack fix freebsd code path. #11766

zend call stack fix freebsd code path. #11766

Conversation

devnexen commented Jul 22, 2023

petk commented Jul 22, 2023 • edited

devnexen commented Jul 22, 2023

devnexen commented Jul 22, 2023

petk commented Jul 22, 2023

devnexen commented Jul 22, 2023

arnaud-lb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaud-lb left a comment

Choose a reason for hiding this comment

petk commented Jul 22, 2023 •

edited