Skip to content

Fix external PHP extension ABI exports - #4108

Open
chubes4 wants to merge 13 commits into
WordPress:trunkfrom
chubes4:fix/4107-external-extension-abi
Open

Fix external PHP extension ABI exports#4108
chubes4 wants to merge 13 commits into
WordPress:trunkfrom
chubes4:fix/4107-external-extension-abi

Conversation

@chubes4

@chubes4 chubes4 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • derive the public PHP extension ABI from the matching libphp.a while retaining MAIN_MODULE=2
  • normalize phpize headers so external extensions use stable _emalloc instead of build-specific _emalloc_<size> symbols
  • make the extension compiler resolve PHP patch releases from the same canonical version data as runtime builds
  • add a Docker-backed PHP 8.3 fixture that compiles a real side module and verifies every env and GOT.* import resolves against the matching runtime or module

Closes #4107.

Why

The runtime previously generated its retained export list only from bundled extensions. @php-wasm/compile-extension could therefore produce a valid external side module that imported public PHP APIs removed from the matching main module by dead-code elimination. Startup then failed late with TypeError: resolved is not a function.

The extension compiler also duplicated PHP patch-version constants. That map drifted to PHP 8.4.20 while the runtime used canonical PHP 8.4.23, undermining matched-header ABI proof. Both paths now consume supported-php-versions.mjs; the published bundle embeds that canonical data and does not gain a workspace-relative runtime dependency.

This keeps the MAIN_MODULE=2 optimization while making the generic public PHP extension ABI available to externally compiled modules. Archive member labels emitted by llvm-nm are filtered before generating Emscripten and Wasm linker export lists.

How to test

  1. Check out this branch on a machine with Docker and Buildx.
  2. Run npm ci.
  3. Run npm exec nx -- run php-wasm-compile-extension:lint.
  4. Run npm exec nx -- run php-wasm-compile-extension:test.
  5. Run npm exec nx -- run php-wasm-compile-extension:typecheck.
  6. Run npm exec nx -- run php-wasm-compile-extension:test-external-extension-abi.
  7. Confirm the final target reports success after building both the PHP 8.3 JSPI runtime and the external phpize side module.

Verification

  • Lint passed.
  • Unit tests passed: 7 files, 38 tests.
  • Typecheck passed.
  • Docker-backed external ABI integration passed with ABI_TEST_EXIT=0.
  • PHP 8.3 JSPI Wasm size changed from 22,839,234 bytes on trunk to 23,251,406 bytes on this branch: +412,172 bytes (+1.80%).
  • Homeboy independently adopted the patch, promoted the three changed version-alignment files, and reran the 38 tests plus both TypeScript typechecks successfully.

Compatibility

The supported PHP minor-version list and public package API are unchanged. Extension builds now follow the canonical runtime patch release instead of a stale duplicate, so rebuilding may produce a side module against newer patch-level headers. Published package bundles embed the canonical version data; no extension path or workspace-relative runtime dependency is introduced.

AI assistance

  • AI assistance: Yes
  • Tool(s): OpenCode with OpenAI GPT-5.6 Sol and Homeboy
  • Used for: Diagnosed the external side-module ABI and patch-version drift, drafted the implementation and integration coverage, and iterated on real PHP.wasm and control-plane verification failures. Chris reviewed and owns the change.

AI assistance: OpenAI gpt-5.6-sol via OpenCode was used to review and implement the follow-up fixes and tests.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes the external PHP extension ABI surface for PHP.wasm by deriving exports from the matching libphp.a, normalizing phpize headers to ensure stable allocator symbols, and adding a Docker-backed integration test that compiles a real external extension and verifies its imports resolve.

Changes:

  • Generate retained export lists from libphp.a (in addition to bundled extensions) to preserve the public extension ABI while keeping MAIN_MODULE=2.
  • Patch installed PHP headers in the extension build image to avoid build-specific _emalloc_<size> symbol references.
  • Add a Docker-backed fixture and Nx target to compile and validate an external side module against the matching runtime exports.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
packages/php-wasm/compile/php/Dockerfile Derives ABI export lists from libphp.a via llvm-nm to retain extension ABI under MAIN_MODULE=2.
packages/php-wasm/compile-extension/tests/test-external-extension-abi.sh Adds an integration script that builds a runtime + external module and checks unresolved imports.
packages/php-wasm/compile-extension/tests/fixtures/external-abi/external_abi.c Adds a real external extension that exercises key API/allocator symbols.
packages/php-wasm/compile-extension/tests/fixtures/external-abi/config.m4 Adds phpize config for the external ABI fixture.
packages/php-wasm/compile-extension/project.json Adds an Nx target to run the external ABI integration test.
packages/php-wasm/compile-extension/docker/Dockerfile.ext Patches installed PHP headers to disable constant-size emalloc() specialization.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/php-wasm/compile/php/Dockerfile
Comment thread packages/php-wasm/compile/php/Dockerfile Outdated
Comment thread packages/php-wasm/compile-extension/tests/test-external-extension-abi.sh Outdated
Comment thread packages/php-wasm/compile-extension/docker/Dockerfile.ext Outdated
@chubes4
chubes4 requested a review from Copilot July 18, 2026 01:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comment thread packages/php-wasm/compile/php/Dockerfile
Comment thread packages/php-wasm/compile-extension/docker/Dockerfile.ext Outdated
Comment thread packages/php-wasm/compile-extension/tests/test-external-extension-abi.sh Outdated
Comment thread packages/php-wasm/compile-extension/tests/test-external-extension-abi.sh Outdated
@chubes4

chubes4 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

@mho22 Review follow-up is ready in 202bc37ec: ABI symbols are deterministically sorted/deduplicated, the header patch is idempotent, and the Docker integration uses a unique temporary directory. Lint, typecheck, shell syntax, and diff checks pass; fresh CI is underway. This remains the external-extension ABI dependency for the proven PHP 8.4 Sodium/WPCOM test stack.

@chubes4

chubes4 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Downstream full-stack validation completed against this PR plus #4146 using https://github.com/chubes4/wordpress-playground/tree/proof/bet12-clean-worker-abi.

The combined build loaded the external Sodium extension and ran clean PHP workers against managed MariaDB. Real focused WPCOM PHPUnit execution passed 25 CHATGPT-19 media tests across the MCP media-create integration and image/audio upload transport suites.

Downstream PR: Automattic/wp-codebox#1938

@chubes4

chubes4 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Additional downstream proof: Automattic/static-site-importer#656 now publishes a real PHP 8.5 JSPI zstd side module with valid CORS, and deployed Playground crashes at startup with TypeError: n is not a function. Binary comparison against deployed PHP 8.5.8 identifies nine missing main-module exports: php_register_url_stream_wrapper, strncasecmp, php_check_open_basedir, php_stream_context_get_option, _php_stream_open_wrapper_ex, _php_stream_free, _php_stream_alloc, _php_stream_write, _php_stream_read.

Manifest: https://automattic.github.io/static-site-importer/playground/extensions/v1.3.5/static-site-importer-zstd-php8.5-jspi.manifest.json
Browser run: https://github.com/Automattic/static-site-importer/actions/runs/30385865542
Tracker evidence: #4107 (comment)

This validates that the PR fixes an active external consumer, not only the synthetic/Sodium fixture. After merge and runtime deployment, SSI will rerun extension_loaded("zstd") plus a real .fig import against the immutable manifest.

@brandonpayton
brandonpayton removed the request for review from mho22 August 3, 2026 00:29
@brandonpayton
brandonpayton self-requested a review August 3, 2026 00:29

@brandonpayton brandonpayton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @chubes4, thank you for this PR. These changes make sense to me, and I really like the expanded test coverage.

I left a few questions and a lot of agreeable comments. This looks good to merge, but I'd like to resolve the questions first.

Comment thread .github/workflows/ci.yml
git fetch --no-tags --depth=1 origin "$base_sha"

if git diff --name-only "$base_sha" "$GITHUB_SHA" -- packages/php-wasm/compile-extension/ | grep -q .; then
if git diff --name-only "$base_sha" "$GITHUB_SHA" -- packages/php-wasm/compile-extension/ packages/php-wasm/compile/php/ | grep -q .; then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this makes sense.

Comment on lines +52 to +53
# _emalloc_<size>() symbols. External side modules must use the stable
# _emalloc() entry point exported by every matching PHP.wasm main module.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why must they? It would be good for this comment to say why.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This one is a bit of a mind-bender, but my practical understanding: This normalizes memory allocations onto Zend’s stable entry point, ensuring Playground remains compatible with external extensions built independently from its PHP.wasm runtimes.

Comment thread packages/php-wasm/compile-extension/project.json
Comment thread packages/php-wasm/compile-extension/scripts/build-in-docker.sh
Comment thread packages/php-wasm/compile-extension/src/compile.ts
Comment thread .github/workflows/ci.yml
Comment thread packages/php-wasm/compile-extension/tests/load-built-extension.mjs
Comment thread packages/php-wasm/compile/php/Dockerfile
AI assistance: OpenAI gpt-5.6-sol via OpenCode was used to clarify why external extensions avoid build-specific allocator symbols.
@brandonpayton

Copy link
Copy Markdown
Member

Feedback from Codex, which @brandonpayton asked me to post for @chubes4:

After filtering my earlier review through the actual scope of #4107 and comparing this PR with current trunk, I want to revise the severity of that feedback.

This PR is a meaningful, bounded improvement over trunk. It addresses the original PHP/Zend-symbol failure by retaining the externally visible symbols from the matching libphp.a, normalizes allocator calls to the stable _emalloc entry point, aligns the compiler headers with the runtime patch releases, and adds a real independently compiled extension test. The reported approximately 412 KB / 1.8% size increase is a reasonable cost for that PHP-owned symbol surface.

I no longer think this PR should be blocked on providing every possible libc or Emscripten system symbol that an arbitrary future side module might import. That is not realistic within the current MAIN_MODULE=2 architecture without retaining a substantially larger runtime.

My revised findings are:

  1. The general libc-completeness concern is not a blocker. The remaining issue is a scope/claim mismatch. The later browser zstd reproduction cited in compile-extension artifacts can require symbols omitted from MAIN_MODULE=2 runtimes #4107 has nine missing imports on trunk. This PR should provide the eight PHP-owned imports, but strncasecmp is an Emscripten libc symbol and would remain missing. Therefore:

    • If this PR is intended to unblock that concrete zstd artifact, add the targeted _strncasecmp export and corresponding browser validation. That would fix a known consumer without claiming to provide a complete libc ABI.
    • Otherwise, describe this as retaining the PHP-owned/libphp.a symbol surface, remove the claim that zstd will work after deployment, and track arbitrary system imports separately.
  2. The absent rebuilt runtime artifacts remain a delivery concern, not necessarily a reason to reject the source change. There are no changes under node-builds or web-builds, so the committed runtimes and packages would not yet contain these exports. The build recipe can merge as a staged first step, but compile-extension artifacts can require symbols omitted from MAIN_MODULE=2 runtimes #4107 should not be considered delivered until regenerated runtimes are committed and deployed through this PR or a guaranteed follow-up.

  3. The Nx/CI dependency gap is valid but nonblocking. A change only to supported-php-versions.mjs does not trigger the custom Docker job. That should be hardened for future metadata-only updates, but it does not invalidate the current successful integration run.

  4. The exact PHP 8.4 patch-version assertion is a nonblocking cleanup. It duplicates the canonical-metadata test and will become stale on a routine patch update.

The PR description should also be brought in line with the implementation: it currently says PHP 8.3 while the test uses PHP 8.5, mentions an ABI_TEST_EXIT=0 marker that is no longer emitted, omits the new get_module validation, and describes the result more broadly than the implementation supports. A more precise title would be something like “Retain libphp symbols for external PHP-WASM extensions.”

The longer-term general solution should be a documented PHP-owned ABI plus required-import metadata/preflight validation. That would satisfy #4107's alternative acceptance criterion—load successfully or fail early with a precise unsupported-import diagnostic—without requiring PHP.wasm to contain every possible libc function.

With that framing, my revised verdict is: mergeable as a valuable incremental improvement once its scope and runtime-artifact delivery plan are stated accurately; universal libc completeness should not block it.

Retain the libc symbol required by the published zstd extension and exercise it in the external ABI fixture.

AI assistance: OpenAI gpt-5.6-sol via OpenCode identified the missing browser-runtime export and added focused fixture coverage; Chris Huber remains responsible for the change.
@chubes4

chubes4 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@brandonpayton The targeted follow-up is complete at f0c4a9d86:

  • added _strncasecmp to the retained ABI and exercised it through a runtime-allocated external extension call
  • passed the Docker external ABI gate
  • passed PHP 8.4 Sodium load and authenticated secretbox round trip
  • passed a full Chromium import of a real Fisiostetic.fig using SSI v1.4.0, WordPress 6.9, candidate PHP 8.5.9, and the immutable published zstd side module (success: true, pages 1 -> 2)
  • passed the focused WPCOM gate on PHP 8.4.24: OK (6 tests, 207 assertions)

This provides the targeted _strncasecmp and browser validation requested in the review. Runtime publication remains a separate delivery step after the build-recipe change lands.

AI assistance: OpenAI GPT-5.6 Sol via OpenCode implemented the targeted ABI follow-up, ran the browser/runtime/WPCOM validation, and prepared this evidence with Chris Huber.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compile-extension artifacts can require symbols omitted from MAIN_MODULE=2 runtimes

3 participants