ext/opcache JIT: attempt to fix EPERM on unprotecting dasm buffer.

[devnexen]

despite disabling protection before mprotect call, we still get an EPERM with ZTS build we set all PROT*flags. removing typo in mapped segments (flag in the wrong place even tough the os most likely discard it).

[dstogov]

I don't see how this affects MACOS/ZTS builds...
Please add MACOS_ARM64_DEBUG_ZTS and MACOS_X64_DEBUG_ZTS test jobs.

[devnexen]

cc @iluuu1994 when you have a moment, to double check if I ve done anything wrong in the CI's side. Thx.

[iluuu1994]

@devnexen You can see the error here: https://github.com/php/php-src/actions/runs/7826796920

The workflow is not valid. .github/workflows/push.yml (Line: 131, Col: 16): Unrecognized named-value: 'max'. Located at position 1 within expression: max.debug && 'DEBUG' || 'RELEASE'

max should be matrix

[devnexen]

@devnexen You can see the error here: https://github.com/php/php-src/actions/runs/7826796920

The workflow is not valid. .github/workflows/push.yml (Line: 131, Col: 16): Unrecognized named-value: 'max'. Located at position 1 within expression: max.debug && 'DEBUG' || 'RELEASE'

max should be matrix

Now, it seems arm64 picks up x86 setting (bison path). No idea yet maybe the job hash ? I ll look later ..

[iluuu1994]

@devnexen See https://github.com/php/php-src/pull/13299/files#diff-5634435a3b24e8a6d21e34340762047e16266acbedc19ecf829b9754de588d89, you didn't copy all changes from GH-13299.

[devnexen]

@devnexen See https://github.com/php/php-src/pull/13299/files#diff-5634435a3b24e8a6d21e34340762047e16266acbedc19ecf829b9754de588d89, you didn't copy all changes from GH-13299.
Thanks. Let me know if I did a mess in your department :)

[iluuu1994]

@devnexen If I may ask, do we need to backport all of these changes? I was under the impression you merely wanted to test this change for 8.2. For warnings, you could have temporarily disabled --enable-werror. As this shows the build to be correct, I'd prefer just merging the functional changes for macOS. If we want to backport CI, we should do so in a separate PR anyway.

[devnexen]

@devnexen If I may ask, do we need to backport all of these changes? I was under the impression you merely wanted to test this change for 8.2. For warnings, you could have temporarily disabled --enable-werror. As this shows the build to be correct, I'd prefer just merging the functional changes for macOS. If we want to backport CI, we should do so in a separate PR anyway.

Not necessarily I kept as separated commits on purpose to see which worths what. Ok for the CI change, at least we see the original change fix the issue on macos arm64.

[iluuu1994]

@devnexen Great! I don't mind back-porting CI. nightly.yml can be dropped, it has no effect on older branches.

[dstogov]

I understood, this PR attempts to fix MACOS-ARM64-ZTS builds.
According to the source changes in ext/opcache/jit/zend_jit.c and ext/opcache/shared_alloc_mmap.c, this also affects MACOS-X64-ZTS builds, that worked fine.
I afraid, this patch completely removes WRITE protection of the JIT memory segment on MACOS-X64-ZTS.
Please confirm (I may be wrong).
I think, removing this protection in the old PHP releases is a completely unacceptable solution.

Also the PR needs to be cleaned up. It touches many unrelated files. I suspect. this is because of change of the base branch.

[dstogov]

Do PHP-8.2/3-MACOS-ARM64-ZTS builds also fail? (they are not covered by "nightly" workflow).

[devnexen]

I understood, this PR attempts to fix MACOS-ARM64-ZTS builds. According to the source changes in ext/opcache/jit/zend_jit.c and ext/opcache/shared_alloc_mmap.c, this also affects MACOS-X64-ZTS builds, that worked fine. I afraid, this patch completely removes WRITE protection of the JIT memory segment on MACOS-X64-ZTS. Please confirm (I may be wrong). I think, removing this protection in the old PHP releases is a completely unacceptable solution.

The change done in ext/opcache/shared_alloc_mmap.c is a correct one though, MAP_JIT not only is. no-op on x86 but also should not be in the flags variable in the first place (since it contains PROT_* flags).

However the other change should had been done only for arm64. I ll go back to it later.

Also the PR needs to be cleaned up. It touches many unrelated files. I suspect. this is because of change of the base branch.

I did not plan to push other changes, was just to display more data on CI.

[dstogov]

The change done in ext/opcache/shared_alloc_mmap.c is a correct one though, MAP_JIT not only is. no-op on x86 but also should not be in the flags variable in the first place (since it contains PROT_* flags).

It seems like this is the real mistake. MAP_JIT should be added to mmap() flags instead of prot argument.
I'll try to check this.

[dstogov]

This disables EXEC permissions for all threads. Therefore other threads are going to crash while this one won't return the permissions back.

According to https://developer.apple.com/documentation/apple-silicon/porting-just-in-time-compilers-to-apple-silicon we should use pthread_jit_write_protect_np() without mprotect(), but I wasn't able to make it work.
In my opinion the code should be changed in this way https://github.com/php/php-src/pull/13375/files

It seems pthread_jit_write_protect_supported_np() somehow returns false (on github actions).
May be we should set com.apple.security.cs.allow-jit entitlement...
I don't have access to Mac and I have no idea how to do this.

[devnexen]

I think it needs codesign (and probably a proper apple license).

[dstogov]

I think it needs codesign (and probably a proper apple license).

Can you try this?

[dstogov]

using self-signed identity.

[devnexen]

will give it a try sometime today definitively.