Add std_hello e2e test: println! on vibix (#852)#870
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds out-of-tree userspace ChangesRust std Userspace Enablement
Sequence DiagramsequenceDiagram
participant Dev as Developer (xtask)
participant Cargo as Cargo (-Z build-std)
participant Host as xtask (image assembly)
participant QEMU as QEMU VM
participant Kernel as vibix Kernel
participant Loader as ELF Loader
participant PML4 as Page Tables (_pml4)
participant Userspace as std_hello _start
participant Main as main()
participant Serial as QEMU Serial
Dev->>Cargo: build std_hello with -Z build-std --target x86_64-unknown-vibix
Cargo-->>Dev: std_hello ELF (image-base set)
Dev->>Host: inject ELF into ext2 rootfs, create ISO
Dev->>QEMU: boot ISO
QEMU->>Kernel: boot
Kernel->>Loader: load_user_elf_with_vmas(std_hello)
Loader->>Loader: register PT_LOAD VMAs
Loader->>PML4: map_user_segment() for each PT_LOAD (eager)
PML4-->>Loader: mappings installed
Kernel->>Userspace: jump to _start (PID 1)
Userspace->>Main: call main(argc=0, argv=null)
Main->>Serial: println!("hello from std")
Serial-->>Dev: capture "hello from std"
Main->>Kernel: exit_group syscall
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Phase 1 capstone: a standard Rust binary using println! runs on vibix
under QEMU and emits "hello from std" on serial.
- userspace/std_hello: standalone binary with `println!("hello from std")`
- xtask std-hello: builds with -Z build-std, installs into ext2, boots QEMU
- kernel: eagerly map PT_LOAD pages in load_user_elf_with_vmas to work
around a demand-paging bug where FileObject::fault serves zero-filled
pages for certain file offsets
- library/std PAL: remove diagnostic prints from _start
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dburkart
force-pushed
the
m852-std-hello-e2e
branch
from
e63fcc7 to
c552942
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
kernel/src/mem/loader.rs (1)
363-373: ⚡ Quick winUpdate the function contract comment above this workaround.
The surrounding docs still say
load_user_elf_with_vmasleavespml4untouched and eagerly maps nothing, but this loop now installs everyPT_LOADpage up front. Please document the temporary eager-map behavior and its scope so callers are not reading a stale contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kernel/src/mem/loader.rs` around lines 363 - 373, Update the function contract comment for load_user_elf_with_vmas to state that, as a temporary workaround, the function now eagerly maps all PT_LOAD pages into the provided pml4 (via map_user_segment over parsed.load_segments()) instead of leaving pml4 untouched; clearly document the scope (this is a workaround for the init-binary/demand-paging bug and affects only PT_LOAD segments installed into the PML4 up front), note that callers must not assume pml4 remains unchanged nor rely on lazy/demand paging for those pages, and mark the behavior as temporary with a reference to the bug (`#852`) and where to revert (the eager mapping loop).xtask/src/main.rs (1)
623-653: ⚡ Quick winAdd preflight validation for
__CARGO_TESTS_ONLY_SRC_ROOTenvironment variable usage.
__CARGO_TESTS_ONLY_SRC_ROOTis an internal test-only Cargo environment variable, not a supported public interface. This build flow couples thestd-hellotarget to undocumented Cargo internals. Add explicit detection and validation (e.g., checking for expected library structure or Cargo version constraints) so a toolchain update that removes this escape hatch fails visibly rather than silently breaking the build.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtask/src/main.rs` around lines 623 - 653, In build_userspace_std_hello, add a preflight validation before setting the __CARGO_TESTS_ONLY_SRC_ROOT env: ensure the library_root has the expected in-repo std layout (e.g., library_root.join("Cargo.toml") and library_root.join("src").join("lib.rs") or at least library_root.join("src").exists()) and return a clear error if missing, and also validate the toolchain supports the unstable build flags (check cargo --version contains "nightly" or run a quick cargo -Z help probe and fail with a descriptive message if unsupported); this makes the use of the internal __CARGO_TESTS_ONLY_SRC_ROOT explicit and fails visibly if the escape hatch or layout is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xtask/src/main.rs`:
- Line 258: The new "std-hello" subcommand is implemented in xtask (see the
match arm "std-hello" => std_hello(&opts)? and the std_hello function) but it's
not exercised in CI; add it to the CI smoke pipeline and to the aggregate
runner: update test_all() in xtask/src/main.rs to call std_hello(&opts)? (so
local runners exercise it), and update the relevant GitHub Actions workflow
under .github/workflows (the smoke/test job) to run `cargo xtask std-hello` as
part of the smoke tests step (or add it to the existing cargo xtask test_all
invocation) so the subcommand is executed during CI.
---
Nitpick comments:
In `@kernel/src/mem/loader.rs`:
- Around line 363-373: Update the function contract comment for
load_user_elf_with_vmas to state that, as a temporary workaround, the function
now eagerly maps all PT_LOAD pages into the provided pml4 (via map_user_segment
over parsed.load_segments()) instead of leaving pml4 untouched; clearly document
the scope (this is a workaround for the init-binary/demand-paging bug and
affects only PT_LOAD segments installed into the PML4 up front), note that
callers must not assume pml4 remains unchanged nor rely on lazy/demand paging
for those pages, and mark the behavior as temporary with a reference to the bug
(`#852`) and where to revert (the eager mapping loop).
In `@xtask/src/main.rs`:
- Around line 623-653: In build_userspace_std_hello, add a preflight validation
before setting the __CARGO_TESTS_ONLY_SRC_ROOT env: ensure the library_root has
the expected in-repo std layout (e.g., library_root.join("Cargo.toml") and
library_root.join("src").join("lib.rs") or at least
library_root.join("src").exists()) and return a clear error if missing, and also
validate the toolchain supports the unstable build flags (check cargo --version
contains "nightly" or run a quick cargo -Z help probe and fail with a
descriptive message if unsupported); this makes the use of the internal
__CARGO_TESTS_ONLY_SRC_ROOT explicit and fails visibly if the escape hatch or
layout is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1440e39-58cf-48d0-90ad-b29a65e0f92d
📒 Files selected for processing (7)
.cargo/config.tomlCargo.tomlkernel/src/mem/loader.rslibrary/std/src/sys/pal/vibix/mod.rsuserspace/std_hello/Cargo.tomluserspace/std_hello/src/main.rsxtask/src/main.rs
| repro_fork_build(&opts)?; | ||
| } | ||
| "shell-pipeline" => shell_pipeline(&opts)?, | ||
| "std-hello" => std_hello(&opts)?, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 --hidden -g '.github/**' '\bstd-hello\b|cargo xtask (test|smoke)' .Repository: dburkart/vibix
Length of output: 4230
The std-hello subcommand is not wired into any CI workflow.
The grep across .github/workflows/ found no invocations of cargo xtask std-hello, despite the subcommand existing in xtask/src/main.rs. The PR objective explicitly requires this to be part of the CI smoke test pipeline, but test_all() still only runs unit and integration tests. The new subcommand will regress silently in CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xtask/src/main.rs` at line 258, The new "std-hello" subcommand is implemented
in xtask (see the match arm "std-hello" => std_hello(&opts)? and the std_hello
function) but it's not exercised in CI; add it to the CI smoke pipeline and to
the aggregate runner: update test_all() in xtask/src/main.rs to call
std_hello(&opts)? (so local runners exercise it), and update the relevant GitHub
Actions workflow under .github/workflows (the smoke/test job) to run `cargo
xtask std-hello` as part of the smoke tests step (or add it to the existing
cargo xtask test_all invocation) so the subcommand is executed during CI.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cargo xtask std-helloend-to-end smoke test that builds a standard Rust binary (println!("hello from std")), installs it as/initin the ext2 rootfs, boots under QEMU, and asserts the marker string appears on serial output.load_user_elf_with_vmasby eagerly mapping PT_LOAD segment pages into the PML4 (the VMA tree is still registered for fork compatibility)._startto remove debugging diagnostics.Test plan
cargo xtask std-hellopasses (~750ms)cargo test --lib -p vibixpasses (714 tests)cargo xtask buildsucceedscargo xtask ext2-imagehash check passesCloses #852
🤖 Generated with Claude Code