test(rccl): add host microtests for src/ras/diagnostics_gpu.cc by aduforest · Pull Request #12436 · ROCm/rocm-systems

aduforest · 2026-09-29T02:35:35Z

JIRA ID: AICOMRCCL-2460

What

Adds host-only microtests for src/ras/diagnostics_gpu.cc in the shared rccl-UnitTestsMicro binary. The suite covers all five GPU diagnostic check pairs: GPU inventory, CUDA driver version, NVIDIA graphics driver version, volatile ECC counters, and NVLink state/speed.

The tests exercise public collection and summary entry points, validate the records written by production, and check argument errors, communicator grouping, incomplete results, mismatches, bounded rank lists, and reporter error propagation. Shared communicator and reporter helpers keep the GPU, common-diagnostics, and RAS fixtures consistent.

The NVIDIA graphics-driver cases use an NVML fake that writes nonzero output before returning an error, so failed-query data must be cleared. Full buffers, unterminated peer versions, unavailable versions, and mismatches have explicit output assertions. All changes are confined to projects/rccl/test/.

Validation

Local validation for commit aeb2e0201:

23/23 GPU diagnostic tests passed.
1,640/1,640 enabled tests passed in the complete shared rccl-UnitTestsMicro binary.
Rebuilt against the current hipified production sources with LLVM 21 coverage instrumentation.
Coverage export confirms execution of both NVIDIA driver-version public entry points and their fill/format helpers.

Coverage for diagnostics_gpu.cc from the focused GPU suite:

Metric	Covered	Percentage
Functions	23/23	100.00%
Lines	552/552	100.00%
Regions	698/824	84.71%
Branches	409/554	73.83%

Required GitHub CI is running separately on the updated commit.

aduforest · 2026-09-29T02:35:38Z

@argus review

therock-pr-bot · 2026-09-29T02:36:21Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

amd-argus

ARGUS

Findings

1 finding(s) that could not be posted as inline comments

projects/rccl/src/ras/diagnostics_gpu.cc:135 (Medium) rasDiagnosticsGpuModelSummarize prints a peer-supplied model[] that has no guaranteed NUL, reading past the records allocation Follow-up for a separate change, not a request on this PR. rasDiagnosticsGpuModelSummarize bounds its comparison with strncmp(..., RAS_DIAG_GPU_MODEL_NAME_LEN) at diagnostics_gpu.cc:120 but passes expectedModel unbounded to %s at diagnostics_gpu.cc:135 and :147, and records is allocated to exactly nData at :92 then fully overwritten with peer bytes at :93. With sizeof(rasDiagnosticsRankHeader) 32 and sizeof(rasDiagnosticsGpuModelData) 65, the 104-byte stride leaves the last record's model[64] plus its 7 padding bytes flush against the end of that allocation, and the framing layer at diagnostics.cc:392-463 validates strides and sizes but never record contents. A trailing single-rank record whose 71 bytes contain no NUL makes %s walk off the heap block, so another rank in the job controls how far this one reads.

Bottom line: Worth settling before approval: the summarize tests never exercise the qsort or read back a production-written record, so several branches they claim to cover are unasserted.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

aduforest · 2026-09-29T18:09:34Z

@argus re-review

amd-argus

ARGUS

Bottom line: Worth settling before approval: the NVML and driver fakes return their error without writing the out-param, so five of the unit's result guards are deletable with the suite green.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

aduforest · 2026-09-30T00:41:12Z

@argus re-review

amd-argus

ARGUS

Bottom line: Worth settling before approval: Contains(state.lines, "unavailable via NVML") at diagnostics-gpu-test.cc:476 is satisfied by two earlier records, so the both-unknown branch it targets is still unasserted.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

amd-argus · 2026-09-30T03:58:33Z

@aduforest the current head (a861c964) already has a review from me above; nothing has changed since then for me to look at again. Push a change and I'll pick it up, or ask again once you have.

aduforest · 2026-09-30T05:39:54Z

@argus re-review

amd-argus

ARGUS

Findings

2 finding(s) that could not be posted as inline comments

projects/rccl/test/host/diagnostics-gpu-test.cc:396 (Medium) both ECC failure cases assert only available, never that the counters were reset Neither failure case asserts that the counters were reset, so the four zeroing statements at diagnostics_gpu.cc:342-345 are deletable with this suite green. data at line 375 still holds 1/3/2/4 from the first call, and the three reads that still succeed rewrite only three of them, so the failed read would surface as a stale count rather than a zero. That is precisely the invariant the comment at diagnostics_gpu.cc:351 claims to hold.
projects/rccl/src/ras/diagnostics_gpu.cc:135 (Medium) pre-existing peer-supplied model[] is printed with %s without ever being re-terminated Worth a separate change, not this one, and flagged only because this PR is the first thing to look closely at the summarizer. rasDiagnosticsGpuModelSummarize prints a peer-supplied model[] with %s at diagnostics_gpu.cc:135, :147 and :173, out of the exactly-nData buffer allocated at diagnostics_gpu.cc:92 and filled verbatim from the wire. Nothing re-terminates a gathered record the way diagnostics_gpu.cc:53 terminates the local one, so a 64-byte name carrying no NUL reads past the allocation. This predates the PR and is not something to fix here.

Bottom line: Worth settling before approval: the ECC device-range guard, three of the four ECC counter reads, and 20 of 21 reporter-failure sites are all still deletable with this suite green.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

aduforest · 2026-09-30T08:25:50Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

amd-argus

ARGUS

This PR changes 1,006 lines across 8 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

amd-argus

ARGUS

This PR changes 1,079 lines across 8 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

amd-argus

ARGUS

This PR changes 1,079 lines across 8 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: Worth settling before approval: the NvidiaDriverVersion check pair in diagnostics_gpu.cc has no test at all, and --gc-sections drops it so the 18/18 function figure excludes it.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

amd-argus

ARGUS

This PR changes 1,189 lines across 8 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

amd-argus

ARGUS

This PR changes 1,189 lines across 8 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

…velop # Conflicts: # projects/rccl/test/host/CMakeLists.txt # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/test/test_categories_micro.yaml

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

…s-diagnostics-gpu-host-microtests # Conflicts: # projects/rccl/test/host/CMakeLists.txt # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/test/test_categories_micro.yaml

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: MICROTEST_README.md:143. Worth clearing before approval.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

systems-assistant · 2026-10-14T00:21:47Z

TheRock Submodule Bump Activity

Newest first

2026-10-14 — Included in TheRock via ROCm/TheRock#8956.

…rs/mkuznet1/aicomrccl-2640-ras-amd-smi Brings in develop through the 2009 branch, including the diagnostics_gpu.cc microtests (#12436) and fabric telemetry in amdsmi_wrap.cc. ras-diagnostics-gpu-test.cc keeps one fixture for the AMD SMI arm: the arm-independent summarize, truncation and reporter tests from develop now expect the RAS_DIAG_* labels, the NVML collect tests are replaced by the AMD SMI ones, and the driver version comes from the ncclCudaDriverVersion seam, since nccl_stubs.cc now owns ncclCudaDriverVersionCache. Co-authored-by: Cursor <cursoragent@cursor.com>

aduforest requested review from a team September 29, 2026 02:35

github-actions Bot added the project: rccl label Sep 29, 2026

systems-assistant Bot added the organization: ROCm label Sep 29, 2026

amd-argus Bot reviewed Sep 29, 2026

View reviewed changes

aduforest force-pushed the users/aduforest/ras-diagnostics-gpu-host-microtests branch from 6e870d9 to 337cb42 Compare September 29, 2026 18:09

amd-argus Bot reviewed Sep 29, 2026

View reviewed changes

aduforest force-pushed the users/aduforest/ras-diagnostics-gpu-host-microtests branch from 337cb42 to a861c96 Compare September 30, 2026 00:40

amd-argus Bot reviewed Sep 30, 2026

View reviewed changes

test(rccl): add host microtests for diagnostics_gpu.cc

a6a2e40

aduforest force-pushed the users/aduforest/ras-diagnostics-gpu-host-microtests branch from a861c96 to a6a2e40 Compare September 30, 2026 05:34

amd-argus Bot reviewed Sep 30, 2026

View reviewed changes

Comment thread projects/rccl/test/host/diagnostics-gpu-test.cc Resolved

Comment thread projects/rccl/test/host/diagnostics-gpu-test.cc Resolved

Comment thread projects/rccl/test/host/diagnostics-gpu-test.cc Resolved

Comment thread projects/rccl/test/host/diagnostics-gpu-test.cc Resolved

test(rccl): cover diagnostics gpu failure paths

94f5168

amd-argus Bot reviewed Sep 30, 2026

View reviewed changes

aduforest marked this pull request as ready for review September 30, 2026 17:53

Merge develop and preserve both RAS and tuning test registration

0489e66

amd-argus Bot reviewed Oct 1, 2026

View reviewed changes

mch approved these changes Oct 2, 2026

View reviewed changes

test(rccl): address RAS diagnostics review feedback

aa4008f

amd-argus Bot reviewed Oct 3, 2026

View reviewed changes

pvallem approved these changes Oct 5, 2026

View reviewed changes

Merge develop and update GPU diagnostics NVML test fakes

84007e7

amd-argus Bot reviewed Oct 6, 2026

View reviewed changes

Merge develop after diagnostics host tests

8355d9c

amd-argus Bot reviewed Oct 6, 2026

View reviewed changes

Comment thread projects/rccl/test/host/ras-diagnostics-gpu-test.cc Resolved

test(rccl): cover NVIDIA graphics driver diagnostics

aeb2e02

amd-argus Bot reviewed Oct 6, 2026

View reviewed changes

aduforest added the Blocked by CI Waiting for CI to pass before reviews or merging label Oct 7, 2026

Merge develop after ras_param host tests

5283ff8

amd-argus Bot reviewed Oct 8, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into fix/12436-merge-de…

c205110

…velop # Conflicts: # projects/rccl/test/host/CMakeLists.txt # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/test/test_categories_micro.yaml

amd-argus Bot reviewed Oct 9, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into users/aduforest/ra…

7d8f57b

…s-diagnostics-gpu-host-microtests # Conflicts: # projects/rccl/test/host/CMakeLists.txt # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/test/test_categories_micro.yaml

amd-argus Bot reviewed Oct 10, 2026

View reviewed changes

aduforest merged commit a7a0377 into develop Oct 12, 2026
76 of 82 checks passed

aduforest deleted the users/aduforest/ras-diagnostics-gpu-host-microtests branch October 12, 2026 19:06

mkuznet1 mentioned this pull request Oct 23, 2026

feat(rccl): read RAS diagnostics GPU checks from AMD SMI #12812

Open

1 task

This was referenced Nov 1, 2026

test(rocrtst): Cover the full SDMA linear-copy packet #14260

Merged

test(rccl): add NCCL_RUN_DIAGNOSTICS tests and docs #12522

Open

Conversation

aduforest commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Validation

Uh oh!

aduforest commented Sep 29, 2026

Uh oh!

therock-pr-bot Bot commented Sep 29, 2026

✅ All Policy Checks Passed

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduforest commented Sep 29, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduforest commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amd-argus Bot commented Sep 30, 2026

Uh oh!

aduforest commented Sep 30, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduforest commented Sep 30, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot left a comment

aduforest commented Sep 29, 2026 •

edited

Loading

aduforest commented Sep 30, 2026 •

edited

Loading