testing/ostest: Split the fork test into vfork and fork. - #3685
Conversation
d3b7722 to
bbc21fe
Compare
casaroli
changed the title
bbc21fe to
45b4ba7
Compare
1f61625 to
ae340d5
Compare
casaroli
changed the title
🔗 Cross-repo PR dependenciesThe read-only Build run reported the following dependent PR(s) and fetched head SHA(s): CI run: https://github.com/apache/nuttx-apps/actions/runs/31246994111 |
ae340d5 to
2a43c6d
Compare
🔗 Cross-repo PR dependenciesThe read-only Build run reported the following dependent PR(s) and fetched head SHA(s): CI run: https://github.com/apache/nuttx-apps/actions/runs/31384974151 |
|
@casaroli the change run out of flash space: we need optimize either the implementation or link script/defconfig |
|
@xiaoxiang781216 I made a PR that will temporarily fix the issue by disabling some unused drivers in that config, apache/nuttx#19837, however this will bite us again, and the proper fix in my opinion is to stop building protected build for this board as it does not have enough flash for it and find another armv7m build with enough flash and qemu support (spoiler: apache/nuttx#19765 has 1MB of flash) and test protected mode there instead. In fact, I think we should model real powerful boards in Renode, and test what we can there, instead of qemu. Say the word and I can add some renode configs to our CI. Thank you for your time and patience in my latest PRs. 🙏 ❤️ |
🔗 Cross-repo PR dependenciesThe read-only Build run reported the following dependent PR(s) and fetched head SHA(s): CI run: https://github.com/apache/nuttx-apps/actions/runs/31723719540 |
ostest's "vfork" test was never testing vfork(). It has the child write a global and the parent observe the write -- the defining property of *sharing*, not of vfork(), whose defining property is that the parent is suspended and whose contract forbids the child to write anything at all. It passed because NuttX implemented fork() and vfork() as the same sharing primitive, which apache/nuttx#19562 separates. vfork.c is rewritten to test what vfork() promises. The child does only what POSIX permits -- it calls _exit(42) and nothing else, not even exit(), which would run atexit handlers and flush stdio in the parent's address space. Since the child may not write memory and the parent cannot run while the child lives, the observable is the child's exit status: had the parent not been suspended, it would have reached waitpid() while the child was still alive. Where child status is not retained -- ostest_main() sets SA_NOCLDWAIT for the whole run, deliberately -- ECHILD is accepted as equally good evidence, since it says the child was already gone when the parent asked. fork.c is new and tests POSIX fork(): the child's writes to .data, .bss and the heap are invisible to the parent and vice versa, a pointer to a stack local taken before the fork names the same object in both, and the child does everything a vfork() child may not -- calls malloc() and printf(), and returns from the function that called fork(). Both run at the top of user_main(). They exercise the lowest-level machinery in the suite -- address environments, stack setup, the architecture's register context -- so a fault in one takes the process down instead of reporting a failure. Learning that in seconds rather than after everything else has passed matters when a port is being brought up. Each test gates on the one primitive it tests, ARCH_HAVE_VFORK and ARCH_HAVE_FORK respectively. There is no compatibility layer and no mapping between symbols. vfork.c no longer requires SCHED_WAITPID: the suspension is in the kernel primitive now, so the test's core assertion holds without it and only the status check is conditional. The simulator is the one exception. It selects ARCH_HAVE_VFORK, but ostest takes the sim down as soon as the test runs there, so the call keeps the !ARCH_SIM guard that apps ee76427 put on the old test in 2024. The old gate hid this: ARCH_HAVE_FORK is not set on the sim, so the test was not built there at all. The other in-tree callers are audited for which primitive they actually meant: * interpreters/python's _posixsubprocess and netutils/libwebsockets' LWS_HAVE_WORKING_VFORK want the fork-then-exec path -- ARCH_HAVE_VFORK. * python's os.fork() and libwebsockets' LWS_HAVE_FORK mean real fork() and stay on ARCH_HAVE_FORK, so they become *absent* rather than silently wrong. * testing/fs/fdsantest's vfork case follows ARCH_HAVE_VFORK. interpreters/bas is deliberately left alone. Its SHELL and EDIT statements reach for vfork() under an ARCH_HAVE_FORK guard and want the same treatment, but checkpatch.sh checks the whole of any file a patch touches and bas_statement.c produces 1681 pre-existing findings against master, so a one-line change there fails CI on its own. The consequence is small: EXAMPLES_BAS_SHELL is EXPERIMENTAL and already depends on ARCH_HAVE_FORK, so it becomes unselectable rather than misbehaving. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Marco Casaroli <marco.casaroli@gmail.com>
2a43c6d to
9911d39
Compare
That sounds nice @casaroli but out CI is already overloaded and frequently over assigned quotas so we cannot stretch it anymore with yet another framework. But if you want prepare something like that on a separate account/repo so it could be replicated and also launched on local workstations that could be interesting :-) |
I believe the best option is to partition CI into more granular checks. We already split by arch, but I think we can go further and split by config: compute the blast radius of a change and rebuild/test only the configs it actually affects. It is a lot of engineering, but I believe it is worth it, since it could cut GitHub Actions consumption considerably. I am not sure what the ultimate benefit would be, but it is probably worth measuring. I am still not sure whether we can use ccache: that could be a short-term improvement. The alternative is fixing the build system so it can reuse artifacts from previous builds instead of requiring a distclean every time. I think all of this is possible, and all of it is worth a measurement. IMHO what we cannot afford is buying back a few KB every time this platform goes over the limit - for a platform nobody uses, kept only because it is our single config that we can test under QEMU. So either we trim it properly (i.e. remove networking and telnet), or we drop it entirely: protected build is not feasible on that tiva board. WDYT? |
|
Yeah, we just need to reduce complexity, and usage. Thus before we change anything we need to have measurements and approvals from @lupyuen and @simbit18 not otherwise :-) Also what would be nice to have is to trigger next steps even if checkpatch/codespell fails, sometimes this comes from existing code camel cases or nomenclature and we cannot change that :-P |
🔗 Cross-repo PR dependenciesThe read-only Build run reported the following dependent PR(s) and fetched head SHA(s): CI run: https://github.com/apache/nuttx-apps/actions/runs/31747547407 |
4 participants
Depends-On: apache/nuttx#19837
Summary
apache/nuttx#19562 separates
fork()andvfork(), which NuttX implements as the same function. This gives each one a test of its own.ostest's "vfork" test was never testingvfork(). It has the child write a global and the parent observe the write — the defining property of sharing, not ofvfork(), whose defining property is that the parent is suspended and whose contract forbids the child to write anything at all. It passed because both names resolved to the same sharing primitive.vfork.cis rewritten to test whatvfork()promises. The child does only what POSIX permits — it calls_exit(42)and nothing else, not evenexit(), which would runatexithandlers and flush stdio in the parent's address space. Since the child may not write memory and the parent cannot run while the child lives, the observable is the child's exit status: had the parent not been suspended, it would have reachedwaitpid()while the child was still alive. Where child status is not retained —ostest_main()setsSA_NOCLDWAITfor the whole run, deliberately —ECHILDis accepted as equally good evidence, since it says the child was already gone when the parent asked.fork.cis new and tests POSIXfork(): the child's writes to.data,.bssand the heap are invisible to the parent and vice versa, a pointer to a stack local taken before the fork names the same object in both, and the child does everything avfork()child may not — callsmalloc()andprintf(), and returns from the function that calledfork().Both run at the top of
user_main(). They exercise the lowest-level machinery in the suite — address environments, stack setup, the architecture's register context — so a fault in one takes the process down instead of reporting a failure. That matters more than usual right now, becauseostestdoes not currently run to completion on any target: it aborts later intimedmutex_timeout_regression_test()attimedmutex.c:185, added by master eea8384 and unrelated to this PR. Running first is the only reason the fork tests run at all.Each test gates on the one primitive it tests,
ARCH_HAVE_VFORKandARCH_HAVE_FORKrespectively. There is no compatibility layer and no mapping between symbols.vfork.cno longer requiresSCHED_WAITPID: the suspension lives in the kernel primitive now, so the test's core assertion holds without it and only the status check is conditional.The other in-tree callers are audited for which primitive they actually meant:
interpreters/python's_posixsubprocessandnetutils/libwebsockets'LWS_HAVE_WORKING_VFORKwant the fork-then-exec path —ARCH_HAVE_VFORK.python'sos.fork()andlibwebsockets'LWS_HAVE_FORKmean realfork()and stay onARCH_HAVE_FORK, so they become absent rather than silently wrong.testing/fs/fdsantest'svforkcase followsARCH_HAVE_VFORK.interpreters/basis deliberately left alone. ItsSHELLandEDITstatements reach forvfork()under anARCH_HAVE_FORKguard and want the same treatment, butcheckpatch.shchecks the whole of any file a patch touches andbas_statement.cproduces 1681 pre-existing findings against master, so a one-line change there fails CI on its own. The consequence is small:EXAMPLES_BAS_SHELLisEXPERIMENTALand alreadydepends on ARCH_HAVE_FORK, so it becomes unselectable rather than misbehaving.Ordering
apache/nuttx#19562 merged on 2026-08-10, so the symbols each test keys on now exist.
This PR must merge after apache/nuttx#19837.
vfork.ckeys onARCH_HAVE_VFORKnow, so it is built again onlm3s6965-ek:qemu-protected, and that configuration has no room left: CI fails there withregion uflash overflowed by 128 bytes. #19837 makes the configuration fit the part again. Its kernel and user images already overlapped by 1384 bytes, unseen, because the kernel was linked against a script that declares the whole flash instead of its own half.fork_test()costs nothing on size-constrained configurations, because it is not built on them. It keys onARCH_HAVE_FORK, which no architecture sets untilup_addrenv_fork()lands for it. It is exercised by the per-architecture PRs that follow, which are what turnARCH_HAVE_FORKback on.Until this PR merges,
ostesthas no fork test. That is the deliberate cost of carrying no compatibility layer.Testing
Host: macOS 15 (Darwin 25.5.0) on Apple Silicon. QEMU 11.0.3, xPack
riscv-none-elf-gcc14.2.0-3, Arm GNUarm-none-eabi-gcc/aarch64-none-elf-gcc14.2.rel1,xtensa-esp32s3-elf-gcc12.2.0.../nuttx/tools/checkpatch.sh -c -u -m -g <base>..HEAD, the exact command.github/workflows/check.ymlruns — ✔️ All checks pass, withcodespell,cvt2utf,cmake-formatandnxstyleinstalled.vfork_test()passes onraspberrypi-pico-2:nsh(real RP2350),qemu-armv7a:nsh,qemu-armv8a:nsh,rv-virt:nsh64andsim:nsh, built against apache/nuttx#19562:esp32s3-devkit:osteston real ESP32-S3 hardware builds and boots clean with neither test compiled in — Xtensa selects neitherARCH_HAVE_VFORKnorARCH_HAVE_FORKand has no fork entry point, so it is the "architecture this does not touch" case.fork_test()is not exercised yet by construction: no architecture selectsARCH_HAVE_FORKuntil a per-architecture PR implementsup_addrenv_fork().