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

Attempt to fix bug #79092 (Building with clang+lld-9 results in a broken PHP binary) #5123

Closed
wants to merge 3 commits into from

Conversation

dstogov
Copy link
Member

@dstogov dstogov commented Jan 28, 2020

No description provided.

@dstogov dstogov requested a review from nikic January 28, 2020 13:22
configure.ac Outdated
EXTRA_LDFLAGS_PROGRAM="$EXTRA_LDFLAGS_PROGRAM -Wl,-zcommon-page-size=2097152 -Wl,-zmax-page-size=2097152"
AC_MSG_CHECKING(linker support for -zcommon-page-size=2097152)
save_LDFLAGS=$LDFLAGS
$LDFLAGS="$LDFLAGS -Wl,-zcommon-page-size=2097152 -Wl,-zmax-page-size=2097152"

Choose a reason for hiding this comment

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

This should probably be LDFLAGS="..." (without the $).

configure.ac Outdated
AC_MSG_RESULT([no])
AC_MSG_CHECKING(linker support for -zmax-page-size=2097152)
save_LDFLAGS=$LDFLAGS
$LDFLAGS="$LDFLAGS -Wl,-zmax-page-size=2097152"

Choose a reason for hiding this comment

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

Same as above (no $).

@falken42
Copy link

Thanks for trying to fix this, but unfortunately it doesn't seem to help. Apparently lld-9 doesn't have any problems generating a valid executable binary with the -Wl,-zcommon-page-size=2097152 -Wl,-zmax-page-size=2097152 flags added during the configure testing process. This makes me think that it's more likely this is a bug in lld.

Could we perhaps add a configure flag (say --disable-huge-pages or similar? ) to workaround this issue for now?

@dstogov
Copy link
Member Author

dstogov commented Jan 28, 2020

@falken42 thanks for testing and fixes. "configure" also tries to run the compiled program. Could you, please, try to compile and execute this simple program by hand. (may be "configure" use another linker").

"lld" doesn't support "-zcommon-page-size" in the same way as GNU ld and gold.
There is some related discussion https://reviews.llvm.org/D56205

@falken42
Copy link

@dstogov Interesting, it looks like configure might not be using the specified LDFLAGS for the test executable? (Sorry, I'm not that great at understanding configure scripts.) Building and running the simple test program by hand definitely results in a bad binary:

root@128acc143413:~/x# cat test.c 
int main() {return 0;}
root@128acc143413:~/x# clang-9 -fuse-ld=lld -Wl,-zcommon-page-size=2097152 -Wl,-zmax-page-size=2097152 -o test test.c
root@128acc143413:~/x# ./test 
./test: error while loading shared libraries: cannot apply additional memory protection after relocation: Cannot allocate memory
root@128acc143413:~/x# 

But with your patches to configure.ac, configure still seems to think it's supported:

...
checking for mremap... yes
checking for sigaction... yes
checking whether to enable zend signal handling... yes

Configuring TSRM
checking linker support for -zcommon-page-size=2097152... yes

Configuring libtool
checking for a sed that does not truncate output... /usr/bin/sed
checking for ld used by clang-9... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
...

@falken42
Copy link

After checking config.log, it does indeed appear that the LDFLAGS aren't being passed when building the conftest binary.

Given the following configure command line:

root@7b7875aeb535:~/php-7.4.1# CC="clang-9" CFLAGS="-Oz" LDFLAGS="-fuse-ld=lld-9" ./configure --prefix=/opt/php --disable-cgi --disable-all

The relevant section in config.log is here, without -fuse-ld=lld-9 being passed to the compiler:

configure:83149: result: ^[[1mConfiguring TSRM^[[m
configure:83372: checking linker support for -zcommon-page-size=2097152
configure:83383: clang-9 -o conftest -Oz -fvisibility=hidden -Wall -Wno-strict-aliasing -DZEND_SIGNALS   -Wl,-zcommon-page-size=2097152 -Wl,-zmax-page-size=2097152 conftest.c  >&5
configure:83383: $? = 0
configure:83383: ./conftest
configure:83383: $? = 0
configure:83394: result: yes

@dstogov
Copy link
Member Author

dstogov commented Jan 29, 2020

@falken42 thanks for investigation. I'll check, how to fix this.

@dstogov
Copy link
Member Author

dstogov commented Jan 29, 2020

@falken42 please test. According to config.log it should work now.

@falken42
Copy link

@dstogov Looks like that fixed it, thank you! Here's the relevant lines from configure:

...
checking for zip archive read/write support... no
checking whether to enable mysqlnd... no
checking whether to disable compressed protocol support in mysqlnd... yes
checking linker support for -zcommon-page-size=2097152... no
checking linker support for -zmax-page-size=2097152... yes

And the resulting php binary executes as expected:

root@e5ec45e176d2:~/php-7.4.1# echo "<?php phpinfo();" | /opt/php/bin/php
phpinfo()
PHP Version => 7.4.3-dev

System => Linux e5ec45e176d2 4.19.76-linuxkit #1 SMP Thu Oct 17 19:31:58 UTC 2019 x86_64
Build Date => Jan 29 2020 09:01:35
Configure Command =>  './configure'  '--prefix=/opt/php' '--disable-cgi' '--disable-all'
...

@dstogov
Copy link
Member Author

dstogov commented Jan 29, 2020

Merged into PHP-7.4 as ce44cd3

@dstogov dstogov closed this Jan 29, 2020
petk added a commit to petk/php-src that referenced this pull request Jun 3, 2024
This enables cross-compiling edge cases to override checks with
php_cv_have_common_page_size and php_cv_have_max_page_size cache
variables when target matches one of the conditions in case pattern.

Not done as link check yet due to Clang 9 bug and similar issues:
php#5123
petk added a commit that referenced this pull request Jun 3, 2024
This enables cross-compiling edge cases to override checks with
php_cv_have_common_page_size and php_cv_have_max_page_size cache
variables when target matches one of the conditions in case pattern.

Not done as link check yet due to Clang 9 bug and similar issues:
#5123
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