Skip to content

Fix fork instrumentation indirect-call discovery#565

Merged
github-actions[bot] merged 4 commits into
mainfrom
emdash/tuning-fork-instrumentation-vhw9q
May 26, 2026
Merged

Fix fork instrumentation indirect-call discovery#565
github-actions[bot] merged 4 commits into
mainfrom
emdash/tuning-fork-instrumentation-vhw9q

Conversation

@brandonpayton
Copy link
Copy Markdown
Member

Summary

  • Make fork-path discovery table-aware for indirect calls, including active/passive/declared element handling and conservative dynamic table writes.
  • Bound transitive indirect-call closure to two dispatch hops while still propagating direct callers, preventing dynamic-runtime cascades from instrumenting unrelated callbacks.
  • Add WAT regressions for wrong-table, declared/passive element, table.init, and over-cascading indirect-call cases; document the remaining static-analysis limit.

Root Cause

The QuickJS/Node-shaped build uses one large LLVM function table. The previous analyzer treated indirect targets too broadly and let same-table/same-signature indirect closure recurse without a resource boundary, so generic interpreter dispatchers pulled in unrelated QuickJS/OpenSSL callbacks. This was an over-broad call-graph discovery issue, not an npm/demo workaround or a transform correctness change.

Verification

  • scripts/dev-shell.sh bash -lc 'HOST_TARGET=$(rustc -vV | awk "/^host/ {print \\$2}"); cargo test -p fork-instrument --target "$HOST_TARGET"'
  • Fork-capable QuickJS/Node-shaped discover-only: optimized temp binary reduced 5516 -> 876; unoptimized temp binary reduced 7269 -> 978.
  • Unoptimized discover-only no longer pulls OpenSSL-family names: SSL=0 EVP=0 CRYPTO=0 ossl=0; still includes JS_CallInternal, js_call_c_function, and js_os_exec.
  • Rebuilt kernel/rootfs/tooling with bash build.sh and rebuilt QuickJS/Node with the final instrumenter; node.wasm is 5,387,796 bytes.
  • Host runtime checks using NodePlatformIO: node npm-cli.js --version prints 10.9.2; require(.../npm/lib/npm.js) prints {"ctor":"function"}.
  • scripts/dev-shell.sh bash -lc 'cd host && npx vitest run ../tests/package-system/fetch-binaries-allow-stale.test.ts'

Notes

  • scripts/test-allow-stale.sh is stale in this checkout: it expects revision = ... in packages/registry/bzip2/package.toml, but that manifest no longer has the field. It restored the file before exiting; the maintained package-system artifact policy Vitest above passes.
  • The targeted npm Vitest file is not runnable as-is against absolute host-side npm paths under the default rootfs-backed worker harness in this checkout; the same npm/module-loading checks pass via the host filesystem harness.

@brandonpayton
Copy link
Copy Markdown
Member Author

CI triage update:

  • Added two follow-up commits after the fork-instrument fix:
    • 0bddfe51 removes the local fork-instrument test warning while preserving the shape assertion.
    • 4b280fcf avoids running package index-update recording after checkout fails, which was producing misleading scripts/index-update.sh: No such file or directory secondary errors in package matrix jobs.
  • The original failing package jobs split into GitHub transport failures, not fork-instrument failures: jobs that had built successfully failed while state-lock tried to push refs/heads/github-actions/state-lock/pr-565-staging; jobs starting later failed before checkout or while downloading actions.
  • The fresh runs on 0bddfe51 and 4b280fcf fail at change-scope before any repo code runs. actions/checkout fetches Automattic/kandelo with the Actions token and GitHub returns remote: Your account is suspended / HTTP 403.
  • Local/API checks show the user account is not suspended and has admin permission on the repo, so the remaining red CI is an Actions git-token/transport issue that needs GitHub/repo auth intervention or a rerun by a non-affected actor. It is not a fork-instrument regression.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 26, 2026

Phase B-1 matrix build status — pr-565-staging

ABI v12. 62 built, 0 failed, 62 total.

Package Arch Status Sha
libcurl wasm32 built d938b440
libcxx wasm32 built 09267a20
libcxx wasm64 built e92ca659
libpng wasm32 built adf10992
libxml2 wasm32 built 7e63465e
libxml2 wasm64 built 36c4a646
ncurses wasm32 built 7de98ade
openssl wasm32 built 12e0cc76
openssl wasm64 built 4bd30ffd
sqlite wasm32 built 9074b3dd
sqlite wasm64 built 4b0fb7fa
zlib wasm32 built f401d53b
zlib wasm64 built 3d2ee1be
bash wasm32 built 55589f31
bc wasm32 built 4632724c
bzip2 wasm32 built 5626a651
coreutils wasm32 built 98e4fbc4
curl wasm32 built ee1fb867
dash wasm32 built 0485d2a9
diffutils wasm32 built cad0d2a5
dinit wasm32 built ad1648be
fbdoom wasm32 built 4bfc408e
file wasm32 built fd6a158e
findutils wasm32 built b6767bf7
gawk wasm32 built 6e046997
git wasm32 built 9568234c
grep wasm32 built 03dc8eb0
gzip wasm32 built d4031699
kandelo-sdk wasm32 built 7d2fa7e1
kernel wasm64 built e0016223
lamp wasm32 built d487238d
less wasm32 built f19d21ea
lsof wasm32 built 21b8be60
m4 wasm32 built 8b1fd20d
make wasm32 built 2552cbb9
mariadb-test wasm32 built 75d18a2d
mariadb-vfs wasm32 built 50558d5b
mariadb-vfs wasm64 built 7afac29e
mariadb wasm32 built ee22cf42
mariadb wasm64 built 9d3156b9
nano wasm32 built 952c5287
nethack-browser-bundle wasm32 built dcea28a1
nethack wasm32 built 392c7b17
nginx wasm32 built 1997ae34
node-vfs wasm32 built 7a7394e2
node wasm32 built b503ab52
php wasm32 built 59c874c8
quickjs wasm32 built b96781e3
sed wasm32 built a869d67c
shell wasm32 built fa7b5b24
spidermonkey wasm32 built 3e0f31c7
tar wasm32 built 5b55cd57
tcl wasm32 built 6fcb0efc
unzip wasm32 built 2b849f34
userspace wasm64 built 57c55bfb
vim-browser-bundle wasm32 built 93109b9a
vim wasm32 built 38b2475e
wget wasm32 built e17bb180
wordpress wasm32 built ccd5b0f2
xz wasm32 built d22b8c62
zip wasm32 built 88ad8d26
zstd wasm32 built ae63dda2

Auto-generated; replaced on each push. Raw data in the publish-status workflow artifact.

@brandonpayton brandonpayton added skip-staging-tests Opt into skipping the staging tests. This does not skip tests before merge. ready-to-ship labels May 26, 2026
@github-actions github-actions Bot merged commit c4897cb into main May 26, 2026
52 of 61 checks passed
@github-actions
Copy link
Copy Markdown

prepare-merge: test-gate passed against the synthetic PR merge and binaries-abi-v12. Missing entries were built or promoted from PR staging only when their merged-tree cache key matched; merge-gate=success posted on PR HEAD and auto-merge enabled.

@github-actions github-actions Bot deleted the emdash/tuning-fork-instrumentation-vhw9q branch May 27, 2026 09:02
brandonpayton added a commit that referenced this pull request May 29, 2026
## Summary
- Make fork-path discovery table-aware for indirect calls, including
active/passive/declared element handling and conservative dynamic table
writes.
- Bound transitive indirect-call closure to two dispatch hops while
still propagating direct callers, preventing dynamic-runtime cascades
from instrumenting unrelated callbacks.
- Add WAT regressions for wrong-table, declared/passive element,
table.init, and over-cascading indirect-call cases; document the
remaining static-analysis limit.

## Root Cause
The QuickJS/Node-shaped build uses one large LLVM function table. The
previous analyzer treated indirect targets too broadly and let
same-table/same-signature indirect closure recurse without a resource
boundary, so generic interpreter dispatchers pulled in unrelated
QuickJS/OpenSSL callbacks. This was an over-broad call-graph discovery
issue, not an npm/demo workaround or a transform correctness change.

## Verification
- `scripts/dev-shell.sh bash -lc 'HOST_TARGET=$(rustc -vV | awk "/^host/
{print \\$2}"); cargo test -p fork-instrument --target "$HOST_TARGET"'`
- Fork-capable QuickJS/Node-shaped discover-only: optimized temp binary
reduced `5516 -> 876`; unoptimized temp binary reduced `7269 -> 978`.
- Unoptimized discover-only no longer pulls OpenSSL-family names: `SSL=0
EVP=0 CRYPTO=0 ossl=0`; still includes `JS_CallInternal`,
`js_call_c_function`, and `js_os_exec`.
- Rebuilt kernel/rootfs/tooling with `bash build.sh` and rebuilt
QuickJS/Node with the final instrumenter; `node.wasm` is `5,387,796`
bytes.
- Host runtime checks using `NodePlatformIO`: `node npm-cli.js
--version` prints `10.9.2`; `require(.../npm/lib/npm.js)` prints
`{"ctor":"function"}`.
- `scripts/dev-shell.sh bash -lc 'cd host && npx vitest run
../tests/package-system/fetch-binaries-allow-stale.test.ts'`

## Notes
- `scripts/test-allow-stale.sh` is stale in this checkout: it expects
`revision = ...` in `packages/registry/bzip2/package.toml`, but that
manifest no longer has the field. It restored the file before exiting;
the maintained package-system artifact policy Vitest above passes.
- The targeted npm Vitest file is not runnable as-is against absolute
host-side npm paths under the default rootfs-backed worker harness in
this checkout; the same npm/module-loading checks pass via the host
filesystem harness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-ship skip-staging-tests Opt into skipping the staging tests. This does not skip tests before merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant