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

Disable tsrm_ls_cache usage on Cygwin #16920

Closed
wants to merge 1 commit into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Nov 24, 2024

Otherwise, --enable-opache-jit won't even build there.


Note that this is supposed to supersede #16568.

Otherwise, `--enable-opache-jit` won't even build there.
@nielsdos
Copy link
Member

So that means we'd be returning 0 on Cygwin from tsrm_get_ls_cache_tcb_offset.
But does the JIT actually work? I.e. do we then have the dynamic TLS resolution working? I see a WIN64 case here

# elif defined(_WIN64)
, is that actually taken then?

@cmb69
Copy link
Member Author

cmb69 commented Nov 24, 2024

I have no idea about the details. I haven't used Cygwin for many years (and frankly, I don't see much point in using this at all nowadays, given that there is WSL2). What I know is that a swoole user/developer confirmed that the patch would work for them. And I know that _WIN64 is not defined in Cygwin environments; despite the name, Cygwin is more like a POSIX environment running on Windows without VM.

If JIT does not work at all on Cygwin, we should disable --enable-opcache-jit for that environment altogether. No need to change C code.

@nielsdos
Copy link
Member

Well I can't test this easily, but usually when a platform isn't supported it's not enough to just stub out the return value, see e.g. #16902 (comment)

@cmb69
Copy link
Member Author

cmb69 commented Nov 24, 2024

I had a closer look, and possibly #16568 is the proper solution (they actually configure --enable-opcache --disable-opcache-jit). But I wonder whether there may be other consumers of tsrm_get_ls_cache_tcb_offset() in which case we cannot simply drop that function if JIT is disabled. If there are no other consumers, that function would have better been defined in OPcache.

And thinking about that more, we cannot apply #16568, since that would kill phpize builds of OPcache JIT (possible done by distros).

Not sure what to do.

@nielsdos
Copy link
Member

I would expect it just uses the same TLS mechanism as the win32 subsystem does, so I can try to have a brief look to see how this would play out. Would need to install Cygwin and figure out how that works so see you in a bit I guess...

@cmb69
Copy link
Member Author

cmb69 commented Nov 24, 2024

I think I can do that. Have a little bit of experience with Cygwin at least.

@cmb69 cmb69 marked this pull request as draft November 24, 2024 17:04
@nielsdos
Copy link
Member

Well I also just got a Cygwin system going, just had to compile&install re2c manually, it's currently compiling php-src.

@nielsdos
Copy link
Member

It also goes wrong with the fiber asm, always fun to see at the end of a slow compile that I had to pass --disable-fiber-asm...

@nielsdos
Copy link
Member

nielsdos commented Nov 24, 2024

Well, it might not matter anyway as opcache doesn't seem to load for me (curiously I only get an .a archive file):

$ ./sapi/cli/php -c .
Failed loading /home/niels/php-src/modules/opcache.a:  Exec format error

and come to think of it, is the PHP_WIN32 flag even defined? I don't think so. But that would break opcache anyway as we have some #if guards for ASLR stuff on Windows...

EDIT: we should probably just discourage people from building with Cygwin anyway since this is probably just the tip of the iceberg...

@cmb69
Copy link
Member Author

cmb69 commented Nov 24, 2024

Well, it might not matter anyway as opcache doesn't seem to load for me (curiously I only get an .a archive file):

See https://bugs.php.net/bug.php?id=65497. :)

How did you build? On Cygwin, you can run .bat files, but you're not supposed to do that for native Cygwin builds. So you could do ./buildconf.bat or ./buildconf, but you want the latter (like on Linux). If the latter, PHP_WIN32 will not be defined.

Well I also just got a Cygwin system going, just had to compile&install re2c manually

Need to do this now. :)

@nielsdos
Copy link
Member

nielsdos commented Nov 24, 2024

See https://bugs.php.net/bug.php?id=65497. :)

Hm, so it's not working at all anyway? I'm not sure what to make of this tbh. How are the swoole people running opcache then (on Cygwin)?

How did you build? On Cygwin, you can run .bat files, but you're not supposed to do that for native Cygwin builds. So you could do ./buildconf.bat or ./buildconf, but you want the latter (like on Linux).

./buildconf
./configure --enable-debug --disable-all --enable-opcache --enable-zts --disable-fiber-asm
make -j4

If the latter, PHP_WIN32 will not be defined.

This means opcache will have issues with ASLR that are not properly mitigated.

@cmb69
Copy link
Member Author

cmb69 commented Nov 24, 2024

Hm, so it's not working at all anyway?

Not sure. This was long time ago, and maybe I've just screwed up back then.

How are the swoole people running opcache then (on Cygwin)?

I don't know if they actually run it. The CI was only about building, but no tests.

@nielsdos
Copy link
Member

Then I guess this is enough as I don't think opcache (and therefore the JIT) can work anyway...

@cmb69
Copy link
Member Author

cmb69 commented Nov 24, 2024

I don't think opcache (and therefore the JIT) can work anyway

Well, but why would they even build with OPcache enabled, if it can't be used?

I'm still trying to complete a minimal build on Cygwin. I've also stumbled upon --disable-fiber-asm (should not be enabled when --disable-all is given, in my opinion), and now had to install PHP. Latest available version is 7.3.7-2. So much for that.

@nielsdos
Copy link
Member

Well, but why would they even build with OPcache enabled, if it can't be used?

Because they're doing hacky stuff to compile opcache statically:

#15074 (comment)

I don't want to support any of this, they seem that they don't know what they're doing with this code.

@cmb69
Copy link
Member Author

cmb69 commented Nov 24, 2024

Ah, completely forgot about that ticket. Then I agree it's good enough to fix the build.

@cmb69 cmb69 marked this pull request as ready for review November 24, 2024 22:13
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Good enough for now

@cmb69
Copy link
Member Author

cmb69 commented Nov 24, 2024

Just in case someone wants to pursue this

Well, it might not matter anyway as opcache doesn't seem to load for me (curiously I only get an .a archive file):

/bin/sh /cygdrive/d/git/php/php-src/libtool --silent --preserve-dup-deps --tag=CC --mode=link cc -shared -I/cygdrive/d/git/php/php-src/include -I/cygdrive/d/git/php/php-src/main -I/cygdrive/d/git/php/php-src -I/cygdrive/d/git/php/php-src/ext/date/lib -I/cygdrive/d/git/php/php-src/TSRM -I/cygdrive/d/git/php/php-src/Zend  -D_GNU_SOURCE  -fno-common -Wstrict-prototypes -Wformat-truncation -Wlogical-op -Wduplicated-cond -Wno-clobbered -Wall -Wextra -Wno-strict-aliasing -Wno-unused-parameter -Wno-sign-compare -g -O2 -ffp-contract=off -fvisibility=hidden -Wimplicit-fallthrough=1 -DZEND_SIGNALS     -o ext/opcache/opcache.la -export-dynamic -avoid-version -prefer-pic -module -rpath /cygdrive/d/git/php/php-src/modules  ext/opcache/ZendAccelerator.lo ext/opcache/zend_accelerator_blacklist.lo ext/opcache/zend_accelerator_debug.lo ext/opcache/zend_accelerator_hash.lo ext/opcache/zend_accelerator_module.lo ext/opcache/zend_persist.lo ext/opcache/zend_persist_calc.lo ext/opcache/zend_file_cache.lo ext/opcache/zend_shared_alloc.lo ext/opcache/zend_accelerator_util_funcs.lo ext/opcache/shared_alloc_shm.lo ext/opcache/shared_alloc_mmap.lo ext/opcache/shared_alloc_posix.lo -lrt
libtool: link: warning: undefined symbols not allowed in x86_64-pc-cygwin shared libraries

I can imagine that you would need to link against import libraries, in this case likely php.dll, but there is no such DLL; instead there is a big php.exe.

we should probably just discourage people from building with Cygwin anyway since this is probably just the tip of the iceberg...

Indeed. There's likely a lot of work to do to have a reliable build.

@cmb69 cmb69 closed this in 32ff46b Nov 24, 2024
@cmb69 cmb69 deleted the cmb/tsrm_ls_cache-cygwin branch November 24, 2024 23:30
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