Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Disable tsrm_ls_cache usage on Cygwin #16920

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
cmb69 wants to merge 1 commit into php:master from cmb69:cmb/tsrm_ls_cache-cygwin

Conversation

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.
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?

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.

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)

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.

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...

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
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.

cmb69 reacted with thumbs up emoji

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...

Copy link
Member

nielsdos commented Nov 24, 2024
edited
Loading

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...

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. :)

Copy link
Member

nielsdos commented Nov 24, 2024
edited
Loading

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.

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.

Copy link
Member

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

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.

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.

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 reacted with thumbs up emoji
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
Copy link
Member Author

cmb69 commented Nov 25, 2024

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

Didn't think about that yesterday, but if only a single process is involved, there are no ASLR issues (at least preloading could be supported and compilation of internal classes). That is not relevant for CLI, but some SAPIs would benefit; I think FrankenPHP (if ever ported to Windows), maybe Swoole (not sure how it's working), Apache (if we get rid of starting up PHP on the control process; see #7865), and even if users use separate OPcache instances for FCGI (opcache.cache_id). And there could even be something like FPM which uses worker threads instead of forked children). We should probably replace the preprocessor conditionals with something else, so a prebuilt OPcache could adapt to the environment. I don't like using an INI setting, but don't have a better idea for now.

nielsdos reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsdos nielsdos nielsdos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /