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

JIT_G(enabled) not set correctly on other threads #16851

Closed
dktapps opened this issue Nov 18, 2024 · 3 comments
Closed

JIT_G(enabled) not set correctly on other threads #16851

dktapps opened this issue Nov 18, 2024 · 3 comments

Comments

@dktapps
Copy link
Contributor

dktapps commented Nov 18, 2024

Description

Disclaimer: I encountered this issue using a threading extension. Well aware this may not be the best source of truth on issues like this, especially due to the difficulty of reproducing.

With the following INI:

opcache.jit=tracing
opcache.jit_buffer_size=0

the following happens:

  • INI is loaded on the main thread and sets JIT_G(enabled) = 1 and JIT_G(on) = 1
  • accel_post_startup() is called during process init, and overrides thread-locals here to force-disable JIT:
    JIT_G(enabled) = false;
    JIT_G(on) = false;
  • dasm_buf is therefore null because zend_jit_startup() wasn't called due to jit_buffer_size=0:
    if (JIT_G(enabled)) {
    if (JIT_G(buffer_size) == 0) {
    JIT_G(enabled) = false;
    JIT_G(on) = false;
    } else if (!ZSMMG(reserved)) {
    zend_accel_error_noreturn(ACCEL_LOG_FATAL, "Could not enable JIT: could not use reserved buffer!");
    } else {
    zend_jit_startup(ZSMMG(reserved), jit_size, reattached);
    }
    }
    &
    dasm_buf = buf;
  • when a second thread is started, INI is loaded again, setting JIT_G(enabled) = 1 and JIT_G(on) = 1, but this time accel_post_startup() does not override these settings
  • finally, the following calls fail on Windows with errors like VirtualProtect() failed [87] The parameter is incorrect during zend_accel_persist_script() because dasm_buf is NULL - this is because JIT_G(on) = 1 although it was supposed to be forcibly disabled:
    if (JIT_G(on) && for_shm) {
    zend_jit_unprotect();
    }
    &
    if (JIT_G(on) && for_shm) {
    if (JIT_G(opt_level) >= ZEND_JIT_LEVEL_OPT_SCRIPT) {
    zend_jit_script(&script->script);
    }
    zend_jit_protect();
    }

Proposed solution

  • JIT_G(enabled) needs to become a true global
  • zend_jit_config() needs to respect the existing value of JIT_G(enabled) (or its replacement true global) instead of overwriting it like it currently does
  • JIT_G(enabled) (or its replacement true global) should be set to true in accel_post_startup() after zend_jit_startup() returns a success code

PHP Version

8.1, 8.2, 8.3, 8.4, master

Operating System

Windows

@dktapps dktapps changed the title JIT_G(enabled) not inherited by other threads if overridden (leads to memory protection errors) (ZTS) JIT_G(enabled) not set correctly on other threads (leads to memory protection errors) (ZTS) Nov 18, 2024
@cmb69
Copy link
Member

cmb69 commented Nov 18, 2024

See also krakjoe/parallel#266 (comment). It seems to me the real problem is that you can enable JIT (opcache.jit) without enabling it (opcache.jit_buffer_size=0).

@dktapps
Copy link
Contributor Author

dktapps commented Nov 18, 2024

Yeah, that looks like the same issue. Unfortunately these are the default values for opcache.jit and opcache.jit_buffer_size so you can run into this just by loading opcache and not even touching JIT.

@cmb69
Copy link
Member

cmb69 commented Nov 19, 2024

Unfortunately these are the default values for opcache.jit and opcache.jit_buffer_size

Note that as of PHP 8.4.0, the defaults are opcache.jit=disable and opcache.jit_buffer_size=64M (see https://wiki.php.net/rfc/jit_config_defaults), though.

@cmb69 cmb69 changed the title JIT_G(enabled) not set correctly on other threads (leads to memory protection errors) (ZTS) JIT_G(enabled) not set correctly on other threads Nov 20, 2024
cmb69 added a commit that referenced this issue Nov 20, 2024
* PHP-8.2:
  Fix GH-16851: JIT_G(enabled) not set correctly on other threads
cmb69 added a commit that referenced this issue Nov 20, 2024
* PHP-8.3:
  Fix GH-16851: JIT_G(enabled) not set correctly on other threads
@cmb69 cmb69 closed this as completed in ff3b4ec Nov 20, 2024
cmb69 added a commit that referenced this issue Nov 20, 2024
* PHP-8.4:
  Fix GH-16851: JIT_G(enabled) not set correctly on other threads
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants