Skip to content

Fix false-positive $SYSCALLS dependency in wrapSyscallFunction. NFC - #28034

Merged
sbc100 merged 1 commit into
emscripten-core:mainfrom
sbc100:fix_syscall_deps
Oct 21, 2026
Merged

sbc100 merged 1 commit into
emscripten-core:mainfrom
sbc100:fix_syscall_deps

Conversation

@sbc100

@sbc100 sbc100 commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

In src/lib/libcore.js, check t.includes('SYSCALLS.') instead of t.includes('SYSCALLS') in wrapSyscallFunction so comments and strings mentioning SYSCALLS_REQUIRE_FILESYSTEM (such as in fd_write) do not pull in $SYSCALLS and $UTF8ToString when SYSCALLS_REQUIRE_FILESYSTEM=0.

Split out from #27999

In `src/lib/libcore.js`, check `t.includes('SYSCALLS.')` instead of
`t.includes('SYSCALLS')` in `wrapSyscallFunction` so comments and
strings mentioning `SYSCALLS_REQUIRE_FILESYSTEM` (such as in
`fd_write`) do not pull in `$SYSCALLS` and `$UTF8ToString` when
`SYSCALLS_REQUIRE_FILESYSTEM=0`.

Split out from emscripten-core#27995
@sbc100
sbc100 requested a review from kripken October 20, 2026 20:41

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the dependency check in src/lib/libcore.js to look for 'SYSCALLS.' instead of 'SYSCALLS' to avoid incorrect matches, alongside updating the codesize test expectations. The reviewer points out that using 't.includes("SYSCALLS.")' is fragile and suggests a more robust regular expression with word boundaries to safely match valid references to 'SYSCALLS' without missing other access patterns.

Comment thread src/lib/libcore.js
@sbc100
sbc100 merged commit d202bda into emscripten-core:main Oct 21, 2026
42 checks passed
@sbc100
sbc100 deleted the fix_syscall_deps branch October 21, 2026 20:42
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.

2 participants