Skip to content

[PHP] fix: restore boxed process construction helpers#747

Closed
brandonpayton wants to merge 1 commit into
integration/kd-6nz-php-phpt-harness-only-basefrom
gascity/kd-6nz/split-pr717-boxed-process-helpers
Closed

[PHP] fix: restore boxed process construction helpers#747
brandonpayton wants to merge 1 commit into
integration/kd-6nz-php-phpt-harness-only-basefrom
gascity/kd-6nz/split-pr717-boxed-process-helpers

Conversation

@brandonpayton

@brandonpayton brandonpayton commented Jun 20, 2026

Copy link
Copy Markdown
Member

Why

A Process record is large, and constructing one as a value in a caller's stack frame places the whole struct on that frame. On wasm the call stack is small and non-growable, so building a Process inline on deep paths (fork/exec/spawn) risks stack overflow. A heap-allocating constructor keeps large process records off the caller's frame. The PHPT harness and PHP subprocess tests drive exactly those deep process-creation paths, so this is a platform robustness fix, not PHP-specific code.

A companion Default for WasmStat gives the kernel an ergonomic, consistent way to build stat records (used by procfs and other synthetic-stat sites), so callers can spread-construct instead of duplicating every field.

What

  • crates/kernel/src/process.rs: add Process::new_boxed() (marked #[inline(never)], returns Box<Self> from a private new_value) so call sites can heap-allocate the record; Process::new() now delegates to it.
  • crates/shared/src/lib.rs: add impl Default for WasmStat (all-zero fields) enabling ..WasmStat::default() construction.

Provenance

Supersedes fork-headed PR #727; identical head commit 504c802110c0c623152d95b22195e0873f48075d, re-pointed onto an Automattic/kandelo branch. No code changed from #727.

@brandonpayton

Copy link
Copy Markdown
Member Author

Closing #747 after Batch-2 review as superseded/incomplete; it will not be replayed into #871.

Review findings:

  • The old commit conflicts with the current explicit-stdio constructor (Process::new_with_stdio).
  • Process::new_boxed has no production caller. Process::new immediately dereferences the box and the process table still stores Process by value, so the patch adds allocation/move/deallocation without preserving boxed ownership.
  • The earlier complete stack-avoidance design used Box::new_uninit() with direct field initialization and boxed process-table values across create/fork/exec/spawn. This split restores none of that end-to-end ownership.
  • The added WasmStat::Default is unrelated, unused, and omitted from the title. Batch 1's procfs repair deliberately removed the redundant default construction that once motivated it.
  • No current Batch-2 constituent depends on either helper, and there is no focused Wasm stack reproducer or measurement supporting the PR's robustness claim.

The original head does compile, and a throwaway mechanical adaptation to the current constructor also compiled and passed the structural ABI check; the rejection is therefore about correctness, scope, and evidence—not an unresolvable merge conflict.

If kernel Wasm stack depth is reproduced as a current defect, it should get a fresh focused PR that covers every constructor including explicit stdio, retains boxed ownership through the process table and fork/exec/spawn paths, omits unrelated stat convenience APIs, and validates the actual failure on Node and Chromium.

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.

1 participant