Repository navigation
fix(mirage): warn when the session's ROCm cannot enumerate the profile's target - #11367
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
🟢 Approval recommended
No correctness issues found in the new ROCr target check logic or its integration points, and the change is covered by targeted unit tests across core/ctl/rocjitsu.
Pull request overview
Adds a pre-flight diagnostic to mirage run that warns when the ROCm runtime the session will load cannot enumerate the emulated GPU ISA (e.g., gfx1250 on older ROCm), addressing the “session appears successful but has no GPU” failure mode described in issue #11361.
Changes:
- Introduces
mirage_core::rocrto locatelibhsa-runtime64.so.1, extract supported gfx targets from the binary image, and reportSupported / Unsupported / Unknown. - Extends the emulator backend trait with an
emulated_isa()hook (defaultNone) and implements it forrocjitsuby readinggfx_target_versionfrom the effectiveSimulationConfig(including--configoverrides). - Wires a best-effort warning into
mirage run(skipping containerized runs and runs that redirect the loader via--env), and documents the symptom/remedy indocs/building.md.
File summaries
| File | Description |
|---|---|
| emulation/mirage/core/src/rocr.rs | New module to locate ROCr, scan supported target names, and produce a best-effort support verdict + user-facing explanation string. |
| emulation/mirage/core/src/lib.rs | Exposes the new rocr module from mirage_core. |
| emulation/mirage/core/src/emulator.rs | Adds EmulatorBackend::emulated_isa() default method so backends can report the ISA actually presented to ROCr. |
| emulation/mirage/rocjitsu/src/lib.rs | Implements emulated_isa() for Rocjitsu by parsing the resolved simulation config and extracting gfx_target_version. |
| emulation/mirage/ctl/src/run.rs | Calls the new check before session startup and prints a warning only on the “confidently unsupported” path. Adds unit tests for the policy. |
| emulation/mirage/docs/building.md | Documents the “no GPU but exit 0” troubleshooting case and references the new warning. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Heads-up: rebasing this PR onto Force-push incoming. This is a history-only rewrite — no code changes from me, no scope expansion.
Why: the branch is one commit behind A follow-up comment will land on this thread once the push is done with the new HEAD SHA, the diff-stat parity check, and the list of commits dropped (if any). If you have a local checkout of |
65d76b5 to
97b4bdf
Compare
|
Rebase complete — force-push landed History-only rewrite. Diff against the new base is byte-equivalent to the diff against the old base (parity-checked locally before pushing).
Diff-stat vs new base (parity-checked against pre-rebase)
A separate commit follows for the If you had a local checkout of |
|
🎉 All checks passed! This PR is ready for review. |
|
The bot's log ends with The
The fix adds Result on Two notes for the reviewer:
|
6f7e9b4 to
43ac55e
Compare
lialan
left a comment
There was a problem hiding this comment.
Five inline findings from the review of 43ac55e.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Three moderate findings remain unresolved regarding environment ambiguity, target scanning, and async-runtime blocking.
Review details
Suppressed comments (3)
emulation/mirage/core/src/rocr.rs:207
- An empty
LD_LIBRARY_PATHis not necessarily harmless: glibc treats a zero-length search-list entry as the current working directory, so the workload can load a differentlibhsa-runtime64.so.1than this probe resolves. In the--clear-env-varscase,resolved_envcan pass exactly this empty value and this early return lets the code emit a false unsupported-target warning. Treat an explicitly emptyLD_LIBRARY_PATHas ambiguous as well.
if value.is_empty() {
return false;
emulation/mirage/core/src/rocr.rs:510
- The scan stops only at non-alphanumeric bytes, so an identifier such as
gfx942_helperis counted as the targetgfx942. Enough such identifiers can clearMIN_PLAUSIBLE_TARGET_REFERENCESand cause an arbitrary or stripped image to produce a falseUnsupportedverdict. Consume the whole word (including_) before applyingplausible_target.
while end < image.len() && image[end].is_ascii_alphanumeric() {
emulation/mirage/supervisor/src/spec.rs:468
build_specsis also called directly from the asyncmirage execpath (ctl/src/run.rs:526), but this new call performs synchronous executable/runtime reads and ELF parsing (including files up to 256 MiB/512 MiB). On a slow filesystem this blocks the Tokio runtime before signal/lease handling is polled, so an exec can become unresponsive for the duration of the probe. Please run the probe viaspawn_blocking(or otherwise keep the synchronous scan off the async runtime) while retaining the shared warning behavior.
warn_if_the_runtime_cannot_see_the_device(desc, &specs);
- Files reviewed: 15/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
b769a41 to
71bdfca
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@lialan can you please review the PR again? If you are good with current code then can you please merge it? |
@amd-arosa is right that `emulated_isa` should not be a bare field on `SessionDescription`, and that the agent is where it belongs. The agent already writes the GPU down as `vm.gpu.device.gfx_target_version`; the field was a second copy of it, and two copies can disagree. So nothing stores the ISA now. It is derived, from the one document that owns it: - `KfdDeviceInfo::gfx_target` and `AgentDef::gfx_target` answer it, and return `Option<GfxTarget>` — a newtype over the packed version, because zero is not a target. A CPU-only KFD node and a device a document left unfilled both report 0, and both mean "no GPU here"; a `NonZeroU32` makes that a case a caller handles rather than a sentinel it can forget to compare against. `Display` renders `gfx1250`, so the encoding is spelled out in one place. - `EmulatorDef::agent` reaches the agent, and `EmulatorDef::resolve_refs` follows the topology and agent references that make it reachable. - `ProfileDef::emulated_gfx_target` is the whole rule: the agent has to name a GPU *and* the backend has to be one that stands a synthetic GPU up. `EmulatorBackend::presents_emulated_device` defaults to false, so `hotswap` and `rocjitsu-dbt` — which retarget onto the host's real card — are silent without knowing the preflight exists, and so is any backend added later. Their two hand-written `emulated_isa: None` lines are gone. - `SessionDescription` carries the resolved profile instead of the ISA. `mirage exec` cannot resolve one for itself: it takes no `--profile`, and re-reading the stored one would answer with whatever has been edited into it since. Optional on the wire, as the field it replaces was, so an older `mirage exec` still attaches; the field doc records the one asymmetry that buys, since `ProfileDef` denies unknown fields. - `InjectionDef::emulated_isa` and `isa_of_config` are deleted. The one case where the agent was *not* the device that would exist is a drop-in `--config`, which is why the field was introduced. That is now fixed at the source instead of reported around: the new `EmulatorBackend::reconcile_profile` hook lets a backend correct the profile before a session freezes it, and rocjitsu reads the config's `vm.gpu.device` into the agent. A config mirage cannot read clears the target rather than leaving the agent's own — rocjitsu parses these with FlatBuffers, which accepts JSON `serde_json` does not, so failing to parse says nothing about whether the run works, only that mirage cannot describe it. Naming the device the config replaced would be a confident wrong answer where silence is available. `Session::new` does both once — resolve, then reconcile — so the profile a session holds describes the machine it is about to stand up, and keeps describing it. That is now where the "fixed at bring-up" guarantee comes from, rather than from a separately captured string, and it makes `SessionContext::profile`'s "every `MaybeRef` followed" true; it was not before, since `resolve_profile` followed only the profile's own reference. A dangling *agent* reference is consequently refused at session creation, as a dangling topology already was, instead of failing every exec later. De-duplication, continuing @amd-arosa's earlier pass: `rocjitsu-dbt` had its own copy of the walk from emulator definition to agent, which is now `resolve_refs` — that alone dropped three imports from `dbt.rs`. Test Plan: the derivation is pinned from both sides — the agent's target is the reported one for gfx1250 and gfx942, a `--config` becomes the agent's device, a config that cannot be read leaves no target at all, and a retargeting backend (`rocjitsu-dbt`) reports none however specific its agent is. `GfxTarget` has its own test for the zero case and the hex step (`gfx90a`). The session test that asserted the ISA came from the injection now asserts the invariant that replaced it: editing the agent store under a live session does not change the device it reports. Stub profiles in the supervisor's own tests name their agent inline, because bring-up follows that reference now. Test Result: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` and `cargo doc --no-deps` clean. `MIRAGE_E2E_ALLOW_SKIP=1 cargo test --locked --workspace --no-fail-fast`: 635 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails on any host that has a compatible GPU and does so on the parent commit too. (`mirage_ctl`'s `a_grid_that_already_fits_changes_nothing` is flaky under the parallel workspace run on the parent commit as well — 2 passes and 1 failure in 3 runs there — and is unrelated to this change.) Co-authored-by: Cursor <cursoragent@cursor.com>
Two findings from @kuhar's review of e81e1b2, one of them also open from Copilot, and the other three open Copilot findings on the same code. - `device_of_config` decoded a supplied `--config`'s device into `KfdDeviceInfo`, which denies unknown fields. Both shipped gfx1250 configs carry `revision_id` and `num_sdma_queues_per_engine`, so the decode failed, the agent was left with no target, and a gfx1250 session on a ROCm that cannot see gfx1250 -- the case this PR exists for -- said nothing. The supplied device is now decoded after dropping fields the type does not model, with the allow-list taken from the type's own serialisation so it cannot drift. Stored agents stay strict. - `reconcile_profile` read the user's file at session creation and `kmd_config` copied it again at injection, so a rewrite in between left the session reporting gfx942 while injecting gfx1250. The snapshot is taken once now, in `reconcile_profile`, which reads the device from those bytes and points the profile at the copy; `kmd_config` hands that copy to the interposer unchanged. The trait method gains the session directory, which `Session::new` has already created. - The container check compared the spelling of a path with the session directory, so `<session>/../host.json` and a symlink out of the session both counted as mounted. A path now has to be inside the session both as spelled, since the mount is made at the session's spelling, and once symlinks are followed. - The container check refused any materialised config `serde_json` cannot parse, including self-contained FlatBuffers-JSON that `kmd_config` copies in only because it names no `simulator_config`. Such a config has no dependency to be out of reach, and is accepted. - A config with an enabled DBT guest presents the host's devices and the guest together, so it names no target, rather than the host VM device it reported before. Taking the guest device instead would make "the workload will see no GPU at all" false. Test Plan: seven new regressions (eight cases), each failing with its fix removed -- the two shipped gfx1250 configs plus gfx942 through `reconcile_profile`; Jakub's sequence at the backend and through a real `Session::new` in the supervisor; a DBT guest config; `..` and symlink escapes; and a commented self-contained config in a container. Test Result: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` and `cargo doc --no-deps` clean. `cargo test -p mirage_rocjitsu --lib` 50 passed; `cargo test -p mirage_supervisor session::tests::` 17 passed. `MIRAGE_E2E_ALLOW_SKIP=1 cargo test --locked --workspace --no-fail-fast`: 641 passed, 1 failed -- `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails on any host with a compatible GPU. `mirage_ctl`'s `a_grid_that_already_fits_changes_nothing`, flaky under the parallel run as noted in e81e1b2, failed in one of two workspace runs and passes 3/3 on its own. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Heads-up: rebasing this PR onto Force-push incoming. This is a history-only rewrite — no code changes from me, no scope expansion.
Why: GitHub reports the merge state as DIRTY — A follow-up comment will land on this thread once the push is done with the new HEAD SHA, the diff-stat parity check, and the list of commits dropped (if any). If you have a local checkout of |
d15d86f to
60b9225
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
…Cm#11367) Four findings from @kuhar's review of df3fb66, three of them also raised by Copilot's second pass. - `loader_state_is_ambiguous` returned early for any empty value, so a search-list variable set to nothing looked like one that was not set. glibc gives an empty *element* of a search list the meaning "the current directory", so what an emptied whole value means is a question about one loader's parsing rather than about the workload — and this module answers only what it is sure of. A search-list variable is now ambiguous as soon as it is present; `LD_PRELOAD`, `LD_AUDIT` and the HSA overrides name something to load or a version to report, so an empty one still asks for nothing and stays harmless. - `target_names` ended a token at the first non-alphanumeric byte, so `gfx942_helper` was counted as a `gfx942` reference. Ten identifiers shaped like that clear `MIN_PLAUSIBLE_TARGET_REFERENCES`, which is the one thing standing between an arbitrary image and a confident "your ROCm does not support this GPU". The scan now consumes the whole word, `_` included, before asking whether it is a target name. The looser boundary in `mentions` is left alone and now says why: every direction it errs in is silence. - `mirage exec` called `build_specs` on a Tokio worker, and that call now reads and parses the workload's executable and the runtime it resolves. It moves to `spawn_blocking`, matching `Session::start_exec`, so slow storage cannot stall the task holding the lease and the signal relays. - `emulated_isa(&EmulatorDef)` had no production caller left: the ISA is captured from the materialised config in `injection_def`, and the public resolver survived only as a second lookup the tests used. It is gone, and those tests now assert on `InjectionDef::emulated_isa` from the real injection path. Reaching it hermetically needs the interposer as a parameter — whether rocjitsu is installed is a fact about the host, and this workspace forbids the `unsafe` that setting an environment variable would need — so `Rocjitsu::injection_def_with` joins `Hotswap::injection_def_for` and `RocjitsuDbt::injection_def_with` in taking the host's answer from its caller. `daemon_capability` and `start_daemon` move unchanged to keep the trait impl contiguous. Swept for the same classes: the two `build_specs` call sites (the other was already on a blocking thread), the other word-boundary scans (`expand_origin` was already correct), and the remaining public entry points of `mirage_core::rocr` — `check_target_at` was public with one in-module caller and no size cap of its own, so it is private now. Test Plan: three new regressions — an emptied search list against an empty preload, suffixed identifiers failing to clear the confidence floor, and a missing interposer refusing the injection. Test Result: `cargo fmt --all -- --check` and `cargo clippy --workspace --all-targets --all-features -- -D warnings` clean. `cargo test --locked --workspace --no-fail-fast`: 613 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails identically on the base commit because this host has a compatible GPU. (The two `the_suite_can_actually_run` guards fail on any checkout without a built sibling rocjitsu; `MIRAGE_E2E_ALLOW_SKIP=1` is the documented way through and is what the count above was taken with.) Co-authored-by: Cursor <cursoragent@cursor.com>
… about the loader (ROCm#11367) Five findings from @kuhar and Copilot on 7202291. Two are loader state the probe did not model. - `/etc/ld.so.preload` is the host's half of the preload question, and no workload environment mentions it. A library listed there loads ahead of the workload's own dependencies and can interpose the very HSA entry points the target table is evidence about. Present and non-blank now forces `Unknown`; absent is the ordinary case and the only harmless one, and unreadable is not rounded into it. - `hwcap_runtime_exists` knew only `<dir>/glibc-hwcaps/<level>`. Before 2.37, glibc also searched hardware-capability directories placed directly under the RUNPATH entry — `tls`, `x86_64`, `haswell`, the platform's own name — so on the glibc most hosts still run, a runtime in one of those is what loads while the probe reads the base copy. It is now `alternative_runtime_exists`, and it looks for a ROCr in any immediate subdirectory rather than for names: the legacy set is per-architecture and per-CPU, and a list of it would be a second thing to keep in step with glibc. Still not recursive — that is what buried the real runtime under `hipblaslt/library` — so the legacy *combinations* nested a level deeper stay a documented false verdict. On this host's ROCm 7.2.4 the added scan is 189 entries, 14 subdirectories, no alternatives, about 1 ms. Two are the session's own configuration changing after bring-up read it. - `start_daemon` called `kmd_config` again, re-resolving the topology and agent stores minutes after `injection_def` captured the ISA from them. An edit in that window left the daemon emulating one device while `SessionDescription` reported another. It reads the materialised file now, through a new `session_config`, which is the only way anything after bring-up is allowed to ask. - A drop-in `--config` was handed to the interposer as the user's own path, and the interposer reopens it — in `--in-process` mode, on every later workload. Rewriting that file retargeted a live session while the captured ISA, and the warning drawn from it, still described the old device. `kmd_config` now copies it into the session directory, so the document a session emulates is one the session owns. Nothing is written beside the user's file, as before. One is the contract this all sits under. - `spec.rs` still promised a mapping "very nearly a function of its inputs" while `build_specs` had grown a probe that reads the process environment, the workload's executable and the runtime it resolves, and prints to stderr. The module doc now states that exception, why it is there rather than in the callers (an earlier version in `run` checked the run and skipped every exec), and how the `CallerStreams` lesson is paid: the policy is `unsupported_target_notices`, a pure function the tests drive with a stub. Ambient here is the answer, never the decision. Swept for the same classes. The other host-wide loader inputs already decline by construction — cache-only resolution never reaches a verdict because only `DT_NEEDED` through `DT_RUNPATH` does, and a `$LIB` or `$PLATFORM` token leaves a `$` the existing check refuses. `describe()` was the earlier half of the re-resolution class and already reads the retained injection; `check_config` resolves before a session exists and writes nothing; `session_config` has one caller. The rocjitsu ISA tests shared one session directory across independent cases, which materialise -once turns into one session brought up twice; each has its own now. Test Plan: four new regressions — a system-wide preload (absent, blank, listed, unreadable), a `RUNPATH/tls` runtime, a supplied `--config` rewritten under a live session, and reading a session that has no materialised configuration. Test Result: `cargo fmt --all -- --check` and `cargo clippy --workspace --all-targets --all-features -- -D warnings` clean. `cargo test --locked --workspace --no-fail-fast`: 616 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails identically on the base commit because this host has a compatible GPU. Co-authored-by: Cursor <cursoragent@cursor.com>
…eference (ROCm#11367) Three findings from Copilot's pass on 47f39ed. Two of them are mine from that commit. - `alternative_runtime_exists` reads the `RUNPATH` directory now, and `read_dir(...).ok()?` made a directory that is not there indistinguish- able from one this cannot read: the walk aborted instead of trying the next entry, so one stale entry in a multi-entry `RUNPATH` silenced the diagnostic for the whole list. glibc skips an entry it finds nothing in and carries on, and so does the probe now — `NotFound` continues, any other error still declines the verdict, and a directory that exists but cannot be vouched for still declines, because that one the loader *will* search. - Copying a drop-in `--config` into the session moved the anchor for the one reference in the schema that resolves beside the config file. `resolve_dbt_host_config_path` joins a relative `dbt_guest.simulator_config` onto the parent of whatever the `config_path` handoff names, so after the copy the shipped `guest_gfx950_on_simulated_gfx942.json` would look for `gfx942_cdna3_kmd.json` in the session scratch directory rather than beside itself. `kmd_config` now makes that reference absolute against the original's directory before writing the copy — the same path rocjitsu would have resolved. Absolute references, configs without one, and JSON this does not recognise travel byte for byte, which is what keeps the ordinary copy a copy. `ProgramConfig::binary_path` is the only other path-valued field in the schema and nothing loads it. - The warning said "the GPU this profile emulates" while the ISA it reports is read from the configuration the session was brought up on, which for a drop-in `--config` is not the profile's agent. It says "this session" now, and the way out is "emulate a GPU this ROCm supports" rather than an instruction to go and edit the one input that is not deciding it. `docs/building.md` follows. Swept the same classes. The `PATH` walk in `resolve_command` already had the semantics the `RUNPATH` walk was missing — a candidate that is not there is skipped and the search continues — and the remaining `.ok()?` reads are all on files a verdict cannot be produced without, where declining is the answer. Nothing else in mirage relocates a file whose contents name other files; containerised drop-in `--config` remains unsupported for the reason it already was, and the pin does not change that. Test Plan: two new regressions — a `RUNPATH` whose first entry does not exist still resolves through its second (with the fixture asserting the linker really kept both entries), and a relative `simulator_config` still naming the file beside the original after the copy, with absolute, absent and unparseable configs going through untouched. The report test now also pins the attribution. Test Result: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` and `cargo doc --no-deps` clean. `cargo test --locked --workspace --no-fail-fast`: 618 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails identically on the base commit because this host has a compatible GPU. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Rebase complete — force-push landed History-only rewrite, parity-checked locally before pushing. The diff against the new base matches the diff against the old base for every file except
The conflict. #12245 ( /// A run that overrides its CPU thread budget gets the same session copy,
/// carrying the override — see [`supplied_config_with_budget`].
Verified: relative to the old PR tip, the new tip of Diff-stat vs new base (parity-checked against pre-rebase)Conflicts resolved during rebaseIf you had a local checkout of |
|
Follow-up on three findings from Copilot's Sep 22 review. They were listed under "Previously missed" in its summary and never opened as threads, so they were easy to lose. All three are fixed in ccc1dd7:
Each fix has a regression test that fails with the fix removed. The workspace run gives 645 passed; the only two failures are the host-dependent ones noted in earlier commits. |
…e's target ## Motivation A profile whose target the host's ROCm was not built for produces a session that starts, a daemon that logs its usual lines, a workload that runs, and an exit status of 0 — with no GPU and no diagnostic anywhere. `libhsa-runtime64.so` enumerates agents by looking their ISA up in the table it was compiled with and skips one it does not recognise silently, so every visible signal is the signal a working run gives. On a ROCm 7.0.2.2 host six of the seven builtin profiles enumerate a GPU and only `mi450x` (gfx1250, the one target outside that ROCm's list) does not. The absence of the diagnostic, not the emulator, is the defect: it cost about a day and was nearly filed against rocJITsu. ## Technical Details `mirage run` now asks, before the session starts, whether the ROCm runtime the workload will load knows the ISA the profile emulates, and prints a warning naming the target, the runtime file, its ROCm version and the symptom when it does not. - `mirage_core::rocr` locates `libhsa-runtime64.so.1` through the shared discovery policy and reads the target table out of the library's own image. There is no API to ask: `hsa_isa_from_name` needs `hsa_init()`, which needs the KFD device that does not exist until the session does. So the verdict has three outcomes, not two — named, absent from a table that plainly is one, or no verdict at all — and only the middle one says anything. A scan that finds fewer than ten plausible names is taken to have missed the table rather than found a small one. - `EmulatorBackend::emulated_isa` defaults to `None`; only `rocjitsu` answers, out of the very `SimulationConfig` the interposer will load, so a drop-in `--config` is judged by the device it actually describes. `hotswap` and `rocjitsu-dbt` retarget onto the host's real GPU, where the profile's target says nothing about whether the session will work. - A warning, never a refusal, and silent whenever mirage is not the one who knows which runtime will be loaded: a containerised session runs the image's ROCm, and a `--env` that sets one of the variables the search consults has already redirected the workload's loader. The variable list comes from `LibSearch::env_hints`, so it cannot drift from the search it guards. ## Issue Tracking GitHub issue: ROCm#11361 ## Test Plan 18 unit tests over the detection logic, split so that the parts which are facts about the host — which ROCm is installed, which backends the binary was linked with — are parameters a test can choose. Verified by hand on the ROCm 7.0.2.2 host from the issue: `mi450x` warns, `mi300x` is silent, `--image` is silent, `--env LD_LIBRARY_PATH=…` is silent. ## Test Result `cargo test --locked --workspace --no-fail-fast`: 605 passed, 1 failed. The failure is `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails identically on the base commit because this host has a compatible GPU. Clippy (`--all-targets --all-features -D warnings`) and rustfmt clean. Co-authored-by: Cursor <cursoragent@cursor.com>
`therock-pr-bot` failed on this PR after waiting out its full 900s budget: `❌ Timed out waiting for required checks to complete.` The policy itself passed. `checks.required_check_runs` in `tools/systems_pr_bot/policy.yml` has exactly one entry, `pre-commit`, and `summarize_required_checks` treats a check run that is *absent* the same as one still running, so the bot polls until it times out and returns 1. No workflow creates a check run named `pre-commit` for a change confined to `emulation/mirage/**`. The job lives in `rocjitsu-formatting.yml`, whose `paths` filter covers `emulation/rocjitsu/**` but not its sibling subtree, so the event never starts the workflow and the check run is never created. ROCm#11242, which changes only `emulation/mirage/CMakeLists.txt`, fails the bot the same way at 15m32s; ROCm#11312 touches `.github/workflows/rocjitsu-*.yml` as well, matches the filter, and passes the bot in 46s. So add `emulation/mirage/**` to the filter and `emulation/mirage` to the sparse checkout, which is what the comment above each of them asks for. Extending the existing job rather than adding a second workflow keeps one check run named `pre-commit`; two would collide in the bot's name-keyed lookup and let an arbitrary one of them decide the verdict. Verified with `pre-commit run --files` over this PR's changed files plus the workflow: the hooks that select them (trailing-whitespace, end-of-file-fixer, check-added-large-files, check-yaml) pass, and no hook rewrites a file. Out of scope and left alone: `emulation/mirage/cmake/RocjitsuCorpus.cmake` and the two files under `emulation/mirage/hotswap/cmake/` are not gersemi-clean today. The job only checks files a PR touches, so this change does not surface them here, but the next PR that edits one of them will need them formatted first. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…Cm#11367) Four findings from @kuhar's review of df3fb66, three of them also raised by Copilot's second pass. - `loader_state_is_ambiguous` returned early for any empty value, so a search-list variable set to nothing looked like one that was not set. glibc gives an empty *element* of a search list the meaning "the current directory", so what an emptied whole value means is a question about one loader's parsing rather than about the workload — and this module answers only what it is sure of. A search-list variable is now ambiguous as soon as it is present; `LD_PRELOAD`, `LD_AUDIT` and the HSA overrides name something to load or a version to report, so an empty one still asks for nothing and stays harmless. - `target_names` ended a token at the first non-alphanumeric byte, so `gfx942_helper` was counted as a `gfx942` reference. Ten identifiers shaped like that clear `MIN_PLAUSIBLE_TARGET_REFERENCES`, which is the one thing standing between an arbitrary image and a confident "your ROCm does not support this GPU". The scan now consumes the whole word, `_` included, before asking whether it is a target name. The looser boundary in `mentions` is left alone and now says why: every direction it errs in is silence. - `mirage exec` called `build_specs` on a Tokio worker, and that call now reads and parses the workload's executable and the runtime it resolves. It moves to `spawn_blocking`, matching `Session::start_exec`, so slow storage cannot stall the task holding the lease and the signal relays. - `emulated_isa(&EmulatorDef)` had no production caller left: the ISA is captured from the materialised config in `injection_def`, and the public resolver survived only as a second lookup the tests used. It is gone, and those tests now assert on `InjectionDef::emulated_isa` from the real injection path. Reaching it hermetically needs the interposer as a parameter — whether rocjitsu is installed is a fact about the host, and this workspace forbids the `unsafe` that setting an environment variable would need — so `Rocjitsu::injection_def_with` joins `Hotswap::injection_def_for` and `RocjitsuDbt::injection_def_with` in taking the host's answer from its caller. `daemon_capability` and `start_daemon` move unchanged to keep the trait impl contiguous. Swept for the same classes: the two `build_specs` call sites (the other was already on a blocking thread), the other word-boundary scans (`expand_origin` was already correct), and the remaining public entry points of `mirage_core::rocr` — `check_target_at` was public with one in-module caller and no size cap of its own, so it is private now. Test Plan: three new regressions — an emptied search list against an empty preload, suffixed identifiers failing to clear the confidence floor, and a missing interposer refusing the injection. Test Result: `cargo fmt --all -- --check` and `cargo clippy --workspace --all-targets --all-features -- -D warnings` clean. `cargo test --locked --workspace --no-fail-fast`: 613 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails identically on the base commit because this host has a compatible GPU. (The two `the_suite_can_actually_run` guards fail on any checkout without a built sibling rocjitsu; `MIRAGE_E2E_ALLOW_SKIP=1` is the documented way through and is what the count above was taken with.) Co-authored-by: Cursor <cursoragent@cursor.com>
… about the loader (ROCm#11367) Five findings from @kuhar and Copilot on 7202291. Two are loader state the probe did not model. - `/etc/ld.so.preload` is the host's half of the preload question, and no workload environment mentions it. A library listed there loads ahead of the workload's own dependencies and can interpose the very HSA entry points the target table is evidence about. Present and non-blank now forces `Unknown`; absent is the ordinary case and the only harmless one, and unreadable is not rounded into it. - `hwcap_runtime_exists` knew only `<dir>/glibc-hwcaps/<level>`. Before 2.37, glibc also searched hardware-capability directories placed directly under the RUNPATH entry — `tls`, `x86_64`, `haswell`, the platform's own name — so on the glibc most hosts still run, a runtime in one of those is what loads while the probe reads the base copy. It is now `alternative_runtime_exists`, and it looks for a ROCr in any immediate subdirectory rather than for names: the legacy set is per-architecture and per-CPU, and a list of it would be a second thing to keep in step with glibc. Still not recursive — that is what buried the real runtime under `hipblaslt/library` — so the legacy *combinations* nested a level deeper stay a documented false verdict. On this host's ROCm 7.2.4 the added scan is 189 entries, 14 subdirectories, no alternatives, about 1 ms. Two are the session's own configuration changing after bring-up read it. - `start_daemon` called `kmd_config` again, re-resolving the topology and agent stores minutes after `injection_def` captured the ISA from them. An edit in that window left the daemon emulating one device while `SessionDescription` reported another. It reads the materialised file now, through a new `session_config`, which is the only way anything after bring-up is allowed to ask. - A drop-in `--config` was handed to the interposer as the user's own path, and the interposer reopens it — in `--in-process` mode, on every later workload. Rewriting that file retargeted a live session while the captured ISA, and the warning drawn from it, still described the old device. `kmd_config` now copies it into the session directory, so the document a session emulates is one the session owns. Nothing is written beside the user's file, as before. One is the contract this all sits under. - `spec.rs` still promised a mapping "very nearly a function of its inputs" while `build_specs` had grown a probe that reads the process environment, the workload's executable and the runtime it resolves, and prints to stderr. The module doc now states that exception, why it is there rather than in the callers (an earlier version in `run` checked the run and skipped every exec), and how the `CallerStreams` lesson is paid: the policy is `unsupported_target_notices`, a pure function the tests drive with a stub. Ambient here is the answer, never the decision. Swept for the same classes. The other host-wide loader inputs already decline by construction — cache-only resolution never reaches a verdict because only `DT_NEEDED` through `DT_RUNPATH` does, and a `$LIB` or `$PLATFORM` token leaves a `$` the existing check refuses. `describe()` was the earlier half of the re-resolution class and already reads the retained injection; `check_config` resolves before a session exists and writes nothing; `session_config` has one caller. The rocjitsu ISA tests shared one session directory across independent cases, which materialise -once turns into one session brought up twice; each has its own now. Test Plan: four new regressions — a system-wide preload (absent, blank, listed, unreadable), a `RUNPATH/tls` runtime, a supplied `--config` rewritten under a live session, and reading a session that has no materialised configuration. Test Result: `cargo fmt --all -- --check` and `cargo clippy --workspace --all-targets --all-features -- -D warnings` clean. `cargo test --locked --workspace --no-fail-fast`: 616 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails identically on the base commit because this host has a compatible GPU. Co-authored-by: Cursor <cursoragent@cursor.com>
…eference (ROCm#11367) Three findings from Copilot's pass on 47f39ed. Two of them are mine from that commit. - `alternative_runtime_exists` reads the `RUNPATH` directory now, and `read_dir(...).ok()?` made a directory that is not there indistinguish- able from one this cannot read: the walk aborted instead of trying the next entry, so one stale entry in a multi-entry `RUNPATH` silenced the diagnostic for the whole list. glibc skips an entry it finds nothing in and carries on, and so does the probe now — `NotFound` continues, any other error still declines the verdict, and a directory that exists but cannot be vouched for still declines, because that one the loader *will* search. - Copying a drop-in `--config` into the session moved the anchor for the one reference in the schema that resolves beside the config file. `resolve_dbt_host_config_path` joins a relative `dbt_guest.simulator_config` onto the parent of whatever the `config_path` handoff names, so after the copy the shipped `guest_gfx950_on_simulated_gfx942.json` would look for `gfx942_cdna3_kmd.json` in the session scratch directory rather than beside itself. `kmd_config` now makes that reference absolute against the original's directory before writing the copy — the same path rocjitsu would have resolved. Absolute references, configs without one, and JSON this does not recognise travel byte for byte, which is what keeps the ordinary copy a copy. `ProgramConfig::binary_path` is the only other path-valued field in the schema and nothing loads it. - The warning said "the GPU this profile emulates" while the ISA it reports is read from the configuration the session was brought up on, which for a drop-in `--config` is not the profile's agent. It says "this session" now, and the way out is "emulate a GPU this ROCm supports" rather than an instruction to go and edit the one input that is not deciding it. `docs/building.md` follows. Swept the same classes. The `PATH` walk in `resolve_command` already had the semantics the `RUNPATH` walk was missing — a candidate that is not there is skipped and the search continues — and the remaining `.ok()?` reads are all on files a verdict cannot be produced without, where declining is the answer. Nothing else in mirage relocates a file whose contents name other files; containerised drop-in `--config` remains unsupported for the reason it already was, and the pin does not change that. Test Plan: two new regressions — a `RUNPATH` whose first entry does not exist still resolves through its second (with the fixture asserting the linker really kept both entries), and a relative `simulator_config` still naming the file beside the original after the copy, with absolute, absent and unparseable configs going through untouched. The report test now also pins the attribution. Test Result: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` and `cargo doc --no-deps` clean. `cargo test --locked --workspace --no-fail-fast`: 618 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails identically on the base commit because this host has a compatible GPU. Co-authored-by: Cursor <cursoragent@cursor.com>
Address @kuhar's follow-up on the legacy glibc loader gap. glibc 2.36 and earlier do not only search one hardware-capability component below a RUNPATH entry: they form a bounded power set from synthetic `tls`, the active `AT_PLATFORM`, and HWCAP_IMPORTANT names. A runtime can therefore live at a nested path such as `tls/haswell` while the probe reads the base copy and emits a false Unsupported verdict. Follow the final legacy component names for x86-64, x86 and AArch64 and walk every existing permutation without traversing any other directory. The ordering deliberately over-approximates glibc: an extra Unknown is safe, while missing an order can produce a false warning. Architectures whose final legacy ABI is not listed decline the verdict entirely. This preserves both boundaries from the earlier review: an immediate legacy runtime remains ambiguous, and `hipblaslt/library` remains an unrelated asset tree rather than a loader candidate. Add regressions for the loader's `tls/haswell` ordering and the requested `haswell/tls` ordering. Measured against this host's ROCm 7.2.4 `/opt/rocm/lib`: five candidate paths checked, no alternatives, about 0.13 ms, and the gfx1250 verdict is unchanged. Test Result: - cargo fmt --all -- --check - cargo clippy --workspace --all-targets --all-features -- -D warnings - cargo doc -p mirage_core --no-deps - cargo test -p mirage_core rocr::tests -- --test-threads=1 (19 passed) - MIRAGE_E2E_ALLOW_SKIP=1 cargo test --locked --workspace --no-fail-fast (618 passed; one pre-existing host-dependent mirage_hotswap failure) Co-authored-by: Cursor <cursoragent@cursor.com>
…OCm#11367) @kuhar is right that the pin added in 9fffabd only works when the supplied path is already absolute, and that the CLI is the only thing making it so. `check_config_file` canonicalizes `--config` before storing it, but that is the CLI's doing. `resolve_sim_config` took the profile option with a bare `PathBuf::from`, so a profile whose `config` was written any other way — by hand, by another tool, by an older mirage — reached the backend exactly as spelled. With `configs/guest.json`, `pin_external_references` then anchored the sibling to `configs`, still relative, and wrote `configs/gfx942_cdna3_kmd.json` into the session snapshot. rocjitsu joins that onto the parent of the config it is handed, which after bring-up is the session directory, so it resolved to `<session>/configs/gfx942_cdna3_kmd.json` and found nothing. So the path is made absolute where the profile option becomes a path this backend acts on, which is the last moment its spelling still means what its author meant. Absolute rather than canonical on purpose: `resolve_dbt_host_config_path` anchors siblings to the parent of the path it is handed without following symlinks, so canonicalizing here would move the directory a symlinked config's siblings hang from. `SimConfig::Supplied` now documents the invariant the pin relies on. Swept the class: `config` is the only path-valued emulator option in the tree. The DBT backend's `target_isa` and `source_isa` are gfx names, and hotswap declares no options at all, so nothing else reaches a backend as an unanchored path. Test Plan: the requested regression drives a relative `emulator.options.config` through `injection_def_with` and asserts the property rocjitsu enforces — the reference is absolute, and joining it the way `resolve_dbt_host_config_path` does finds the host config. It fails on the parent commit with exactly the reported spelling, `configs/gfx942_cdna3_kmd.json`. Test Result: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` and `cargo doc --no-deps` clean. `cargo test --locked --workspace --no-fail-fast`: 619 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails identically on the base commit because this host has a compatible GPU. Co-authored-by: Cursor <cursoragent@cursor.com>
…11367) @kuhar re-tested the pin added in 9fffabd and found it still breaks the case it was added for. One `//` comment in the shipped `guest_gfx950_on_simulated_gfx942.json` is enough: `serde_json` refuses it, `pin_external_references` fell through to copying the bytes unchanged, and the relative `dbt_guest.simulator_config` then resolved under the session directory, where the host config has never been. The dialect gap is wider than comments. rocjitsu parses its configs with `flatbuffers::Parser` (v24.3.25, `config_common.h:54-57`), whose lexer skips `//` and `/* */`, and which with the default `strict_json = false` also accepts trailing commas (`idl_parser.cpp:1465,1674`) and unquoted field names (`:1477`). Matching that would mean carrying a second JSON parser in mirage, so this takes the other option in the review: preserve the original reference base on the fallback. `pin_external_references` returns `None` when it could not read the document *and* the document mentions the one field whose meaning a move would change, and `kmd_config` then hands the interposer the user's own file instead of a copy. Every config rocjitsu accepts keeps working; what is given up is the narrow guarantee the copy buys, on configs mirage cannot read. The literal-substring test is deliberately the shape that errs safely: a field is set by naming it, every dialect spells the name the same way, so its absence is enough to know there is nothing to pin. `session_config` now reads the `config_path` discovery file rather than asserting `rj_config.json` exists, because the session's config is one of two files and a second rule for which would be a second thing to keep in step. That file is the byte the interposer itself opens, so the daemon and the workload cannot disagree about it. `CONFIG_PATH_NAME` replaces the four spellings of its name. A node container mounts the session's scratch directory and nothing else, so a config left where it lies is one nothing in the container can open. `injection_def_with` refuses that combination by name instead. Containerised drop-in `--config` was already unsupported — an external reference is not mounted either, pinned or not — but an interposer that cannot read its own configuration, inside a container, with nothing saying why is the exact failure mode this PR exists to stop shipping. Also the terminology half of Copilot's pass: `rocr.rs` still said "the GPU a profile describes" and "the ISA the profile emulates" where its own `explain()` says "this session". The module doc states the distinction once now. `docs/building.md` already named both sources as of f943543. De-duplication pass, at @amd-arosa's request. Prose collapsed onto one canonical statement with cross-references: absence-not-presence (twice in the module doc, again in `verdict`), session-not-profile (module doc + `explain`), the drop-in `--config` essay (`injection_def_with` and `start_daemon` each re-derived what `kmd_config` documents), and `docs/building.md`, which restated the module's internal rationale at a user. Code: `hwcap_runtime_exists` and `legacy_combination_runtime_exists` shared a verbatim trust-check-and-probe block, now `searchable_directory`; the two read-whole size caps were bare literals, now `MAX_EXECUTABLE_BYTES` and `MAX_RUNTIME_BYTES`; the loader env-var lists were transcribed three times in the tests, now `SEARCH_LIST_VARS` and `NAMED_OBJECT_VARS`; `ROCR_SONAME` was `pub` with no caller outside the module. Test Plan: one new regression — a commented drop-in `--config` naming a relative sibling is not relocated, the interposer and the daemon are both pointed at the original, the sibling still resolves beside it, and the containerised form is refused. It fails on the parent commit, where the snapshot is written and the reference resolves into the session directory. `pin_external_references` also now covers the trailing-comma and unquoted-key spellings, and `session_config` the empty discovery file. Test Result: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` and `cargo doc --no-deps` clean. `cargo test -p mirage_core rocr::tests` 19 passed; `cargo test -p mirage_rocjitsu --lib` 41 passed. `MIRAGE_E2E_ALLOW_SKIP=1 cargo test --locked --workspace --no-fail-fast`: 630 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails identically on the unmodified parent commit because this host has a compatible GPU. Co-authored-by: Cursor <cursoragent@cursor.com>
…m#11367) Copilot is right that 1ce44e9 only checked whether the top-level config itself was inside the session. A strict supplied config is copied there, so that check passes, but an active `dbt_guest.simulator_config` is still either pinned to or already names an absolute host path outside the session. Container bring-up mounts the session scratch at the same spelling, not arbitrary files the config references, so the interposer cannot open the simulator config. Inspect the materialised config on the container path. Reject when either the config itself or an active simulator-backend dependency lies outside the session; empty still means the config itself, matching rocjitsu. Disabled and hardware-backed DBT blocks ignore `simulator_config` and are left alone. Correct the previous error's advice too: making the reference absolute does not make it visible in a container. The regressions cover both paths Copilot identified: relative references made absolute by the pin and references already absolute. Both fail on the parent commit because the copied top-level config passes its `starts_with` guard. A second regression keeps unused references from being over-rejected. Test Result: - cargo fmt --all -- --check - cargo clippy --workspace --all-targets --all-features -- -D warnings - cargo doc --no-deps - cargo test -p mirage_rocjitsu --lib -- --test-threads=1 (43 passed) - MIRAGE_E2E_ALLOW_SKIP=1 cargo test --locked --workspace --no-fail-fast (582 passed; one pre-existing host-dependent mirage_hotswap failure) Co-authored-by: Cursor <cursoragent@cursor.com>
@amd-arosa is right that `emulated_isa` should not be a bare field on `SessionDescription`, and that the agent is where it belongs. The agent already writes the GPU down as `vm.gpu.device.gfx_target_version`; the field was a second copy of it, and two copies can disagree. So nothing stores the ISA now. It is derived, from the one document that owns it: - `KfdDeviceInfo::gfx_target` and `AgentDef::gfx_target` answer it, and return `Option<GfxTarget>` — a newtype over the packed version, because zero is not a target. A CPU-only KFD node and a device a document left unfilled both report 0, and both mean "no GPU here"; a `NonZeroU32` makes that a case a caller handles rather than a sentinel it can forget to compare against. `Display` renders `gfx1250`, so the encoding is spelled out in one place. - `EmulatorDef::agent` reaches the agent, and `EmulatorDef::resolve_refs` follows the topology and agent references that make it reachable. - `ProfileDef::emulated_gfx_target` is the whole rule: the agent has to name a GPU *and* the backend has to be one that stands a synthetic GPU up. `EmulatorBackend::presents_emulated_device` defaults to false, so `hotswap` and `rocjitsu-dbt` — which retarget onto the host's real card — are silent without knowing the preflight exists, and so is any backend added later. Their two hand-written `emulated_isa: None` lines are gone. - `SessionDescription` carries the resolved profile instead of the ISA. `mirage exec` cannot resolve one for itself: it takes no `--profile`, and re-reading the stored one would answer with whatever has been edited into it since. Optional on the wire, as the field it replaces was, so an older `mirage exec` still attaches; the field doc records the one asymmetry that buys, since `ProfileDef` denies unknown fields. - `InjectionDef::emulated_isa` and `isa_of_config` are deleted. The one case where the agent was *not* the device that would exist is a drop-in `--config`, which is why the field was introduced. That is now fixed at the source instead of reported around: the new `EmulatorBackend::reconcile_profile` hook lets a backend correct the profile before a session freezes it, and rocjitsu reads the config's `vm.gpu.device` into the agent. A config mirage cannot read clears the target rather than leaving the agent's own — rocjitsu parses these with FlatBuffers, which accepts JSON `serde_json` does not, so failing to parse says nothing about whether the run works, only that mirage cannot describe it. Naming the device the config replaced would be a confident wrong answer where silence is available. `Session::new` does both once — resolve, then reconcile — so the profile a session holds describes the machine it is about to stand up, and keeps describing it. That is now where the "fixed at bring-up" guarantee comes from, rather than from a separately captured string, and it makes `SessionContext::profile`'s "every `MaybeRef` followed" true; it was not before, since `resolve_profile` followed only the profile's own reference. A dangling *agent* reference is consequently refused at session creation, as a dangling topology already was, instead of failing every exec later. De-duplication, continuing @amd-arosa's earlier pass: `rocjitsu-dbt` had its own copy of the walk from emulator definition to agent, which is now `resolve_refs` — that alone dropped three imports from `dbt.rs`. Test Plan: the derivation is pinned from both sides — the agent's target is the reported one for gfx1250 and gfx942, a `--config` becomes the agent's device, a config that cannot be read leaves no target at all, and a retargeting backend (`rocjitsu-dbt`) reports none however specific its agent is. `GfxTarget` has its own test for the zero case and the hex step (`gfx90a`). The session test that asserted the ISA came from the injection now asserts the invariant that replaced it: editing the agent store under a live session does not change the device it reports. Stub profiles in the supervisor's own tests name their agent inline, because bring-up follows that reference now. Test Result: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` and `cargo doc --no-deps` clean. `MIRAGE_E2E_ALLOW_SKIP=1 cargo test --locked --workspace --no-fail-fast`: 635 passed, 1 failed — `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails on any host that has a compatible GPU and does so on the parent commit too. (`mirage_ctl`'s `a_grid_that_already_fits_changes_nothing` is flaky under the parallel workspace run on the parent commit as well — 2 passes and 1 failure in 3 runs there — and is unrelated to this change.) Co-authored-by: Cursor <cursoragent@cursor.com>
Two findings from @kuhar's review of e81e1b2, one of them also open from Copilot, and the other three open Copilot findings on the same code. - `device_of_config` decoded a supplied `--config`'s device into `KfdDeviceInfo`, which denies unknown fields. Both shipped gfx1250 configs carry `revision_id` and `num_sdma_queues_per_engine`, so the decode failed, the agent was left with no target, and a gfx1250 session on a ROCm that cannot see gfx1250 -- the case this PR exists for -- said nothing. The supplied device is now decoded after dropping fields the type does not model, with the allow-list taken from the type's own serialisation so it cannot drift. Stored agents stay strict. - `reconcile_profile` read the user's file at session creation and `kmd_config` copied it again at injection, so a rewrite in between left the session reporting gfx942 while injecting gfx1250. The snapshot is taken once now, in `reconcile_profile`, which reads the device from those bytes and points the profile at the copy; `kmd_config` hands that copy to the interposer unchanged. The trait method gains the session directory, which `Session::new` has already created. - The container check compared the spelling of a path with the session directory, so `<session>/../host.json` and a symlink out of the session both counted as mounted. A path now has to be inside the session both as spelled, since the mount is made at the session's spelling, and once symlinks are followed. - The container check refused any materialised config `serde_json` cannot parse, including self-contained FlatBuffers-JSON that `kmd_config` copies in only because it names no `simulator_config`. Such a config has no dependency to be out of reach, and is accepted. - A config with an enabled DBT guest presents the host's devices and the guest together, so it names no target, rather than the host VM device it reported before. Taking the guest device instead would make "the workload will see no GPU at all" false. Test Plan: seven new regressions (eight cases), each failing with its fix removed -- the two shipped gfx1250 configs plus gfx942 through `reconcile_profile`; Jakub's sequence at the backend and through a real `Session::new` in the supervisor; a DBT guest config; `..` and symlink escapes; and a commented self-contained config in a container. Test Result: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` and `cargo doc --no-deps` clean. `cargo test -p mirage_rocjitsu --lib` 50 passed; `cargo test -p mirage_supervisor session::tests::` 17 passed. `MIRAGE_E2E_ALLOW_SKIP=1 cargo test --locked --workspace --no-fail-fast`: 641 passed, 1 failed -- `mirage_hotswap`'s `no_compatible_gpu_is_refused_rather_than_fabricated`, which fails on any host with a compatible GPU. `mirage_ctl`'s `a_grid_that_already_fits_changes_nothing`, flaky under the parallel run as noted in e81e1b2, failed in one of two workspace runs and passes 3/3 on its own. Co-authored-by: Cursor <cursoragent@cursor.com>
…#11367) Three findings from Copilot's review of 5c70e6f, listed under "Previously missed" in its summary and never opened as threads. - `session_config` trimmed every surrounding space from the path in the `config_path` discovery file. rocjitsu's own reader, `parse_dbt_runtime_config_handoff`, takes the first line and removes one trailing `\r`, nothing else, so a config whose path begins or ends with a space named one file to mirage and another to the interposer. It reads the line the same way now. - A `RUNPATH` entry that exists but is not a directory reached `trusted_system_directory`, which declines a regular file, so one such entry silenced the diagnostic for the whole list. The loader's open through it fails and it moves on; so does the probe now, matching the missing-entry case and the legacy capability walk. The entry is checked with `metadata` rather than `symlink_metadata`, so a dangling symlink is the missing entry it is to the loader too. - The selected runtime, up to `MAX_RUNTIME_BYTES`, was read once to check its ELF class and again for the verdict. The resolver now returns the image it checked and the verdict judges those bytes, so each workload reads it once. `check_target_at`, which existed only for the second read, is gone. Test Plan: two new regressions, each failing with its fix removed -- a discovery path with leading and trailing spaces, alone and on a CRLF first line followed by a gpu_id line; and the stale-first-entry workload with that entry made a regular file, which also asserts the returned image is the runtime's own bytes. Test Result: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` and `cargo doc --no-deps` clean. `cargo test -p mirage_core rocr::tests` 19 passed; `cargo test -p mirage_rocjitsu --lib` 53 passed. `MIRAGE_E2E_ALLOW_SKIP=1 cargo test --locked --workspace --no-fail-fast`: 645 passed, 2 failed -- the host-dependent `mirage_hotswap` `no_compatible_gpu_is_refused_rather_than_fabricated` and the flaky `mirage_ctl` `a_grid_that_already_fits_changes_nothing`, both noted in earlier commits on this branch. Co-authored-by: Cursor <cursoragent@cursor.com>
|
merging it as per Vivek's request |
TheRock Submodule Bump ActivityNewest first
|
Motivation
A profile whose target the host's ROCm was not built for produces a
session that starts, a daemon that logs its usual lines, a workload that
runs, and an exit status of 0 — with no GPU and no diagnostic anywhere.
libhsa-runtime64.soenumerates agents by looking their ISA up in thetable it was compiled with and skips one it does not recognise silently,
so every visible signal is the signal a working run gives.
On a ROCm 7.0.2.2 host, six of the seven builtin profiles enumerate a
GPU. The only one that does not is the only one whose target is outside
that ROCm's list:
The same binary, the same rocJITsu build and the same
mi450xprofileenumerate the device correctly inside a ROCm 10.0 container. rocJITsu is
not at fault — it presents the emulated device correctly and its daemon
output is byte-identical between a working target and an unsupported
one. The absence of the diagnostic is the defect: it cost about a day of
investigation and was nearly filed against the emulator.
Technical Details
Before each host workload starts — both the owning
mirage runcommand and latermirage execcommands — mirage asks whether the ROCm runtime that concrete process will load knows the ISA the session emulates, and prints a warning when it does not:Why the pre-flight check and not the post-condition check. The issue
suggests two designs, and the post-condition one — count GPU agents once
the session is up and warn if none appeared — is the more general of the
two, but mirage cannot take it without becoming an HSA client. Agent
enumeration is
hsa_init()plushsa_iterate_agents, run inside thesession against the interposed KFD; from outside, mirage sees a daemon
that came up healthy and a workload it does not interpret. Doing it
properly means linking the ROCm runtime into mirage or shipping a probe
binary into every session, container and rank — a large, always-on cost
paid on every run to detect one thing. The pre-flight check is a file
read of a library mirage already knows how to locate, and it names the
cause rather than the symptom.
Resolving the runtime and reading the target list. There is no side-effect-free API to ask:
hsa_isa_from_nameneedshsa_init(), which would initialize ROCr in mirage rather than inspect the workload. The probe therefore resolves the concrete command through its effectivePATH, parses owned ELF bytes, and follows only a directDT_NEEDEDforlibhsa-runtime64.so.1through an unambiguous modernDT_RUNPATH. It never invokes the dynamic loader or executes workload code.The target scan is evidence, not an interface, so presence is inconclusive: ROCr also embeds names used only for legacy code-object conversion. Only absence from a recognizably complete runtime image produces
Unsupported; target mentions and every uncertain case produceUnknownand stay silent.Who gets asked. The rocjitsu backend records the emulated ISA while it materializes the exact
SimulationConfigand injection used for session bring-up. That retained value travels inSessionDescription, so editing a named profile, topology, or agent cannot change what a live session reports.hotswapandrocjitsu-dbtleave it unset because they retarget code onto a physical host GPU.A warning, never a refusal, and silent whenever the static answer is not trustworthy. Containerized sessions use the image's ROCm. Host probes return
Unknownfor loader overrides, legacy RPATH, transitive ordlopendependencies, cache-only resolution, mutable or privileged executables, executable audit tags, unresolved loader tokens, and glibc hardware-capability alternatives. Those deliberate false negatives are documented indocs/building.md; broader coverage can be added later with a bounded loader-equivalent static resolver.Issue Tracking
GitHub issue: #11361
Test Plan
Regression coverage includes the original ROCm 7 target-table case; raw legacy gfx mentions; concrete command and effective-environment resolution;
--clear-env-vars; hostmirage exec; loader overrides; direct RUNPATH and$ORIGINtoken boundaries; executableDT_AUDIT/DT_DEPAUDIT; glibc-hwcaps versus unrelated ROCm asset trees; root execute-permission semantics; and retaining the materialized session ISA.Verified by hand on the ROCm 7.0.2.2 host from the issue:
mi450xwarns,mi300xis silent, and containerized runs remain silent.Test Result
Review-fix validation on
b769a41313:cargo fmt --all -- --checkcargo clippy -p mirage_core -p mirage_rocjitsu -p mirage_hotswap -p mirage_supervisor --all-targets -- -D warningscargo test -p mirage_core rocr::tests -- --test-threads=1(16 passed)cargo test -p mirage_rocjitsu --lib -- --test-threads=1(26 passed)cargo test -p mirage_supervisor 'session::tests::' -- --test-threads=1(16 passed)cargo test -p mirage_supervisor 'spec::tests::' -- --test-threads=1(40 passed)The earlier full workspace run remains 605 passed and one pre-existing host-dependent
mirage_hotswapfailure, reproduced on the base commit.Note for the reviewer of #11312: this touches
core/src/emulator.rs,rocjitsu/src/lib.rsanddocs/building.md, which that PR also touches,but at different points and with no semantic overlap —
emulated_isareads the config
resolve_sim_configreturns, whatever that functiongrows. Whoever lands second gets a small textual rebase in
rocjitsu/src/lib.rs.Submission Checklist