Enable host-UT coverage measurement and improve branch coverage - #10062
Enable host-UT coverage measurement and improve branch coverage#10062i-kosarev wants to merge 11 commits into
Conversation
…path find_package(GTest) via the RCCL-vendored path (build/gtest) only exports GTest::gtest, not the legacy GTest::GTest alias that target_link_libraries() below expects. Add the alias once after GTest resolution so it works regardless of which resolution path (system/vendored/FetchContent) was taken.
…st redefinition mem_manager.h defines its own hipMemFabricHandle_st fallback whenever HIP_FABRIC_API is undefined. test/host/CMakeLists.txt never replicated the top-level CMakeLists.txt's detection of this macro, so on ROCm 7.14.0 (whose hip_runtime_api.h already provides hipMemFabricHandle_t) the host-UT build hit a hard struct redefinition error. Mirror the top-level check_symbol_exists/check_cxx_source_compiles detection and wire HIP_FABRIC_API into both rccl-source-wrappers and rccl-HostUnitTests compile definitions.
…st.a RCCL-vendored build/gtest/lib/libgtest.a is compiled without -fPIC. Linking it into the default PIE executable produced by hipcc fails with "relocation R_X86_64_32 cannot be used against local symbol; recompile with -fPIC" against gtest-all.cc.o. Link rccl-HostUnitTests as non-PIE instead of rebuilding the vendored archive.
…t flush ProcessIsolatedTestRunner.cpp forks each RUN_ISOLATED_TEST into a child that re-execs itself and calls _exit() directly, bypassing libc atexit handlers that would normally flush LLVM's profiling runtime. The runner already has a fix for this gated behind RCCL_TEST_CODE_COVERAGE (explicit __llvm_profile_write_file() call before _exit()), matching test/CMakeLists.txt's ENABLE_CODE_COVERAGE pattern, but test/host/CMakeLists.txt never defined it. Without this, only 1 of ~103 expected .profraw files were generated per run; with it, all isolated test children flush their coverage data correctly.
Whitebox-clear manager->initialized after a successful Init, then probe every guarded entry point: Track/Untrack/MarkExportToPeer return ncclInternalError and leave the entry list untouched, while Destroy treats the manager as already torn down, detaches it from comm and returns ncclSuccess. Closes the !initialized guards at mem_manager.cc lines 71/143/243/344 (BRDA: True:0 -> True:1 on all four). Destroy's detach path deliberately does not free, so the test re-arms and reattaches the manager afterwards to let the fixture run the real teardown (~mutex() + free); verified leak-free against Init_Success with a malloc/free interposer.
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
Pull request overview
Enables LLVM source-based coverage instrumentation for the CPU-only rccl-HostUnitTests build and adds targeted host unit tests to improve branch coverage in several host-testable code paths.
Changes:
- Fixes host-UT CMake plumbing for coverage and broader build compatibility (GTest aliasing, HIP fabric API detection, link options, coverage macro propagation).
- Adds new host-only unit tests to exercise previously uncovered branches in Rome topology consensus, kernel config parsing, mem manager guards, and alt RSMI init paths.
- Improves determinism safeguards around coverage-driven tests (notably for unordered-map iteration assumptions).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rccl/test/RomeTopoConsensusTests.cpp | Adds a coverage-targeted test for plurality/tie-break branches in Rome topo consensus. |
| projects/rccl/test/mem_manager/MemManagerTests.cpp | Adds a test to validate not-initialized guards across mem manager entry points. |
| projects/rccl/test/IommuPassthrough_test.cpp | Adds tests for null/empty option handling and read-file failure paths. |
| projects/rccl/test/host/CMakeLists.txt | Adds coverage option/macro propagation, GTest target aliasing, HIP fabric API detection, and link option adjustments for host-UT. |
| projects/rccl/test/AltRsmiTests.cpp | Adds isolated-process tests covering alt RSMI “already initialized” and bad KFD path branches. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| std::unordered_map<int, std::pair<int, int>> probe; | ||
| probe.reserve(7); | ||
| probe.emplace(1, std::make_pair(0, 0)); | ||
| probe.emplace(2, std::make_pair(0, 0)); | ||
| probe.emplace(8, std::make_pair(0, 0)); | ||
| std::vector<int> order; | ||
| for (const auto& e : probe) order.push_back(e.first); | ||
| auto pos = [&order](int key) { | ||
| return std::find(order.begin(), order.end(), key) - order.begin(); | ||
| }; | ||
| ASSERT_EQ(probe.bucket_count(), 7u) << "reserve(7) no longer yields 7 buckets"; | ||
| ASSERT_EQ(probe.bucket(1), probe.bucket(8)) << "keys 1 and 8 no longer collide"; | ||
| ASSERT_LT(pos(8), pos(1)) | ||
| << "key 8 is no longer visited before key 1: this test would still pass " | ||
| "but would stop covering the False arms on rome_topo_consensus.cc:36"; | ||
| } |
| include(CheckSymbolExists) | ||
| check_symbol_exists("hipMemImportFromShareableHandle" "hip/hip_runtime_api.h" HIP_FABRIC_API_FUNC) | ||
| check_cxx_source_compiles(" | ||
| #include <hip/hip_runtime_api.h> | ||
| int main() { | ||
| hipMemFabricHandle_t handle; | ||
| (void)handle; | ||
| return 0; | ||
| } | ||
| " HIP_FABRIC_HANDLE_TYPE) |
Three three-group votes exercising the remaining arms of the plurality tie-break on rome_topo_consensus.cc:36. Part 1 uses vote counts 4/2/1 so a group is visited with cnt < refVotes, closing the `cnt == refVotes` False arm. Part 2 ties all three groups at 2 votes so a group with a higher firstRank loses the tie-break, closing the `firstRank < refFirstRank` False arm (BRDA: both False arms 0 -> non-zero). Each part also asserts the emitted "voted refIdx N from K of M". The return value alone cannot distinguish a correct tie-break from a broken one -- the function reports ncclInvalidUsage for any disagreement, which with three groups holds no matter which index wins -- so the WARN is the only channel that observes the decision. Verified by mutation: inverting `firstRank < refFirstRank` leaves the return value untouched and is caught only by these assertions. Part 3 pins the strictness of the vote comparison itself, which parts 1 and 2 cannot: in both, the group that should win is visited last, so weakening `cnt > refVotes` to `cnt >= refVotes` elects the same index. It uses nranks 5 and keys 6/2/1, whose visit order is 2, 1, 6. Model-index keys are load-bearing: reserve(nranks) sizes the tallies map and std::hash<int> is the identity, but the visit order also depends on insertion order, so it is established per case rather than derived from key % bucket_count alone. Each part asserts its layout up front so it fails loudly rather than silently covering nothing if it ever changes.
Call ncclKernelConfigContentHasOption with a nullptr and an empty option string. Closes the option == NULL / option[0] == '\0' guard at kernel_config.cc:42 (BRDA: True:0 -> True:1 on both sub-branches).
Call ncclKernelConfigReadFile with a nullptr path and with a nullptr content pointer. Closes the path == NULL / content == NULL guard at kernel_config.cc:47 (BRDA: True:0 -> True:1 on both sub-branches).
Call ncclKernelConfigReadFile with a path that is asserted not to exist, closing the !in.is_open() guard at kernel_config.cc:50 (BRDA: True:0 -> True:1). The path is PID-scoped and built from temp_directory_path(), matching the convention already used for kTestDrmBasePath, so a stale path left by a parallel CI job on the same node cannot make the test silently take the success path instead.
Call ARSMI_init() twice inside one isolated test and assert the device count is unchanged. The second call must hit the "already initialized" early return (ARSMI_num_devices > 0) at alt_rsmi.cc:83 instead of re-scanning the KFD nodes directory (BRDA: True:0 -> True:1). A third KFD node is created between the two calls, because without it the test cannot fail: ARSMI_allSystemNodes is a local rebuilt on every call, so re-scanning an unchanged directory writes the same ARSMI_num_devices and is indistinguishable from the early return. Mutating the guard away is caught only with the node present, since a re-scan then reports 3 devices. The extra node is removed before the assertions run. kTestKFDPath is shared rather than PID-scoped, and a failed ASSERT returns without reaching cleanupTestEnvironment, so leaving it behind would make every later test that expects 2 devices fail for an unrelated reason.
Point kKFDNodesPathRoot at a directory asserted not to exist and call ARSMI_init() without the sandbox setup, closing the opendir() == nullptr failure arm at alt_rsmi.cc:89 (BRDA: True:0 -> True:1). The path is PID-scoped and built from temp_directory_path(), matching the convention documented for kTestDrmBasePath, so it stays absent when several CI jobs share a node.
fec926c to
98dec2f
Compare
|
FYI there is another pending PR related to this: #9817 |
| @@ -172,16 +204,20 @@ add_executable(rccl-HostUnitTests | |||
|
|
|||
| target_include_directories(rccl-HostUnitTests PRIVATE ${RCCL_INCLUDE_DIRS}) | |||
|
|
|||
| option(ENABLE_CODE_COVERAGE "Enable LLVM source-based coverage instrumentation" OFF) | |||
There was a problem hiding this comment.
One of the acceptance criteria we defined for AICOMRCCL-1661 was to just always generate coverage for this binary. I'm not seeing that here right now though.... does this influence the compiler flags to add coverage somehow I'm not seeing? In another PR (#9817) we have this:
target_compile_options(rccl-UnitTestsMicro PRIVATE
-fprofile-instr-generate -fcoverage-mapping
"SHELL:-Xarch_device -fno-profile-instr-generate"
"SHELL:-Xarch_device -fno-coverage-mapping")
target_link_options(rccl-UnitTestsMicro PRIVATE
-fprofile-instr-generate -fcoverage-mapping)
| @@ -63,6 +63,15 @@ if(NOT GTest_FOUND) | |||
| endif() | |||
| endif() | |||
|
|
|||
| # Both find_package(GTest) code paths above (system and RCCL-vendored) export | |||
There was a problem hiding this comment.
Why do we have three ways to get gtest? Could we just use FETCH_CONTENT and call it a day?
| // That detach path deliberately does not free: nothing owns `m` now, so | ||
| // leaving it here would leak the manager (TearDown skips a null | ||
| // comm->memManager). Re-arm and reattach it so the fixture runs the real | ||
| // teardown, which destroys the placement-new mutex and frees the struct. | ||
| m->initialized = 1; | ||
| comm->memManager = m; |
There was a problem hiding this comment.
Any concerns about clean up and cross-test interactions in the event this test fails or throws an exception? It seems like a failure/exception could cause a leak. Maybe ScopedGuard?
| std::array<const char*, n> names{{"a", "a", "a", "a", "a", "a", "a"}}; | ||
| std::array<uint64_t, n> hosts{{1, 1, 1, 1, 1, 1, 1}}; |
There was a problem hiding this comment.
Are these intended to be representative of all indices being on the same host?
| EXPECT_NE(testing::internal::GetCapturedStderr().find("voted refIdx 1 from 4 of 7"), | ||
| std::string::npos); |
There was a problem hiding this comment.
I'm a little uncomfortable with using an internal gtest API for this, but it's probably fine for now. Perhaps we can add a helper to work with debug.{h,cc} to make it easier to capture log outputs in tests. An expectation like EXPECT_LOG_TO_CONTAIN("voted refIdx 1 from 4 of 7"); would be nice and readable.
| { | ||
| std::unordered_map<int, std::pair<int, int>> probe; | ||
| probe.reserve(7); | ||
| probe.emplace(1, std::make_pair(0, 0)); | ||
| probe.emplace(2, std::make_pair(0, 0)); | ||
| probe.emplace(8, std::make_pair(0, 0)); | ||
| std::vector<int> order; | ||
| for (const auto& e : probe) order.push_back(e.first); | ||
| auto pos = [&order](int key) { | ||
| return std::find(order.begin(), order.end(), key) - order.begin(); | ||
| }; | ||
| ASSERT_EQ(probe.bucket_count(), 7u) << "reserve(7) no longer yields 7 buckets"; | ||
| ASSERT_EQ(probe.bucket(1), probe.bucket(8)) << "keys 1 and 8 no longer collide"; | ||
| ASSERT_LT(pos(8), pos(1)) | ||
| << "key 8 is no longer visited before key 1: this test would still pass " | ||
| "but would stop covering the False arms on rome_topo_consensus.cc:36"; | ||
| } |
There was a problem hiding this comment.
I don't think this is useful, it seems like a partial reimplementation of rcclCheckRomeTopoModelIdxConsensus, and is not testing any production code.
| { | ||
| std::unordered_map<int, std::pair<int, int>> probe; | ||
| probe.reserve(5); | ||
| probe.emplace(6, std::make_pair(0, 0)); | ||
| probe.emplace(2, std::make_pair(0, 0)); | ||
| probe.emplace(1, std::make_pair(0, 0)); | ||
| std::vector<int> order; | ||
| for (const auto& e : probe) order.push_back(e.first); | ||
| auto pos = [&order](int key) { | ||
| return std::find(order.begin(), order.end(), key) - order.begin(); | ||
| }; | ||
| ASSERT_LT(pos(2), pos(1)) | ||
| << "key 2 is no longer visited before key 1: this part would still pass " | ||
| "but would stop pinning the strictness of the vote comparison"; | ||
| } |
There was a problem hiding this comment.
This doesn't exercise any production code either.
| { | ||
| constexpr int n = 5; | ||
| std::array<int, n> idx{{6, 2, 2, 1, 1}}; | ||
| std::array<const char*, n> names{{"a", "a", "a", "a", "a"}}; | ||
| std::array<uint64_t, n> hosts{{1, 1, 1, 1, 1}}; | ||
| testing::internal::CaptureStderr(); | ||
| EXPECT_EQ(rcclCheckRomeTopoModelIdxConsensus( | ||
| n, | ||
| [&](int r) { return idx[r]; }, | ||
| [&](int r) { return names[r]; }, | ||
| [&](int r) { return hosts[r]; }), | ||
| ncclInvalidUsage); | ||
| EXPECT_NE(testing::internal::GetCapturedStderr().find("voted refIdx 2 from 2 of 5"), | ||
| std::string::npos); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you can extract a helper function to reduce duplication in these tests. The only inputs that change are the idx arrays, and the outputs we're asserting are the stderr.
RCCL Perf-Regression Gate:
|
| group | keys | regressions | inconclusive |
|---|---|---|---|
| all_gather_perf-d=bfloat16-default | 0 | 0 | 0 |
| all_gather_perf-d=float-default | 0 | 0 | 0 |
| all_reduce_perf-d=bfloat16-default | 0 | 0 | 0 |
| all_reduce_perf-d=float-default | 0 | 0 | 0 |
| broadcast_perf-d=bfloat16-default | 0 | 0 | 0 |
| broadcast_perf-d=float-default | 0 | 0 | 0 |
| reduce_scatter_perf-d=bfloat16-default | 0 | 0 | 0 |
| reduce_scatter_perf-d=float-default | 0 | 0 | 0 |
Motivation
rccl-HostUnitTests(added in #9320) gives us a CPU-only test binary, but building it withcoverage instrumentation did not work, so branch coverage for host-testable code had never
actually been measured. Without a working measurement the test suite has no objective
acceptance signal — gaps are guessed at rather than found.
This PR makes that build work end to end, then closes gaps the resulting report exposed in
four host-testable files. Everything here is CPU-only: no GPU, no HIP allocation, no device
required to build or run.
Technical Details
Part 1 — make the instrumented host-UT build work (4 commits). Four independent blockers
in
test/host/CMakeLists.txt:GTest::GTestalias is missing on the RCCL-vendored GTest path, so linking fails when thebuild does not fall back to a system GTest.
hipMemFabricHandle_stis redefined depending on the HIP version; added aHIP_FABRIC_APIdetection guard mirroring the existingAMDSMI_FABRIC_APIhandling.libgtest.ais non-PIC, so the binary must link-no-pie.RCCL_TEST_CODE_COVERAGEwas defined only in the GPU test tree (test/CMakeLists.txt),never in the host tree, even though
test/common/ProcessIsolatedTestRunner.cppkeys itsprofraw flush off that macro. Process-isolated host tests therefore silently discarded
their coverage data. Added an
ENABLE_CODE_COVERAGEoption (defaultOFF) that propagatesthe define.
Part 2 — close the exposed gaps (7 commits, one test per commit). Each uncovered branch
was confirmed as
BRDA == 0in the merged profile before a test was written, then re-measuredafter:
NotInitialized_GuardsAllEntryPointsmem_manager.ccthreeWayVoteStrictLoserAndTieBreakLoserrome_topo_consensus.ccContentHasOptionNullOrEmptyOptionkernel_config.ccReadFileNullArgskernel_config.ccReadFileNonexistentPathkernel_config.ccARSMIInitCalledTwiceIsShortCircuitedalt_rsmi.ccARSMIInitBadKfdPathFailsToOpenDiralt_rsmi.ccThree test-design points worth a reviewer's attention:
Why the rome-topo test asserts log output.
rcclCheckRomeTopoModelIdxConsensusreturnsncclInvalidUsagefor any disagreement, so with three vote groups the return value isidentical no matter which index wins — checking it alone cannot tell a correct tie-break from
a broken one. The emitted
voted refIdx N from K of Mis the only channel through which thefunction reports its decision, so each part pins that string. Part 3 additionally pins the
strictness of the comparison itself: in parts 1 and 2 the group that should win is visited
last, so weakening
cnt > refVotestocnt >= refVoteselects the same index and goesunnoticed.
Why the double-init test mutates the filesystem.
ARSMI_allSystemNodesis a local rebuilton every call, so re-scanning an unchanged directory writes the same
ARSMI_num_devicesandis indistinguishable from the early return. The test adds a third KFD node between the two
calls — a re-scan would then report 3 devices. It removes that node before asserting:
kTestKFDPathis shared rather than PID-scoped, and a failedASSERTreturns withoutreaching
cleanupTestEnvironment, so leaving it behind would make every later test thatexpects 2 devices fail for an unrelated reason.
Teardown in the mem-manager test.
ncclMemManagerDestroy's not-initialized path detachesthe manager from
command returns success without freeing, to avoid a double free ifDestroy already ran. The fixture's
TearDownonly frees whencomm->memManageris non-null,so probing that path leaks the manager. The test re-arms
initializedand reattaches themanager so the fixture runs the real teardown, which also destroys the placement-new mutex.
Structurally unreachable branches — documented, deliberately not tested:
rome_topo_consensus.cc:47, False arm ofif (nDisagree > 0). Line 42 already returnedwhen
tallies.size() == 1, so at least two distinct model indices exist and at least onerank necessarily differs from
refIdx.nDisagreeis mathematically always> 0here.mem_manager.cc:206and:214, False arms ofelse if (memType == ncclMemOffload).ncclMemType_thas three values andncclMemPersistreturns early at line 149, so bythese lines the type is either Scratch or Offload — the trailing else-if can never fall
through.
Known remaining gaps (out of scope, follow-up material).
kernel_config.ccline 129(
ncclIommuPassthroughOkwith a null or non-matching cmdline) is still open, andncclKernelConfigReadGzip,ncclKernelConfigReadFirstAvailableandncclKernelHasConfigOptionare never called by any test in this binary — they own the mostcomplex logic in the file (gzip via
popen, multi-path fallback search) and deserve their owntest plan.
alt_rsmi.ccandmem_manager.ccretain reachable, non-hardware-gated branchesbeyond this PR's scope.
Nothing outside
test/is touched, so no CHANGELOG entry is needed: no NCCL API versionchange, no impact on library users or other ROCm libraries.
Issue Tracking
JIRA ID: AICOMNET-379
Builds on #9320, which introduced the host-only test binary. The
ENABLE_CODE_COVERAGEplumbing here is a prerequisite for any further host-only coverage work in this binary.
Test Plan
rccl-HostUnitTestswith-DENABLE_CODE_COVERAGE=ONand run the full suite.llvm-profdata merge -sparse, thenllvm-cov export -format=lcov. Measure the base commit and HEAD with the identicalpipeline in one session.
and require the test to fail. A test that survives removal of its own guard buys coverage
but no guarantee.
Test Result
Full suite: 282 tests from 37 test suites, 0 failures (baseline 276). Each of the 11
commits was built and run on its own; the test count increases monotonically
276 → 277 → 278 → 279 → 280 → 281 → 282 with no failures at any step.
Mutation results — 8/8 mutations detected. Each row removes or inverts the named branch in
the generated source and re-runs only the test that claims it:
mem_manager: drop the!initializedguard inncclMemTrackrome_topo: invert the tie-break (firstRank <→>)rome_topo: weaken the plurality test (cnt >→>=)kernel_config: drop the null/empty option checkkernel_config: drop the null-argument check inReadFilekernel_config: drop the!is_open()checkalt_rsmi: drop the already-initialized early returnalt_rsmi: drop the opendir-failure returnBranch coverage, base → HEAD, same pipeline for both. Two denominators are reported because
llvm-cov's lcov export is not self-consistent: itsBRF/BRHsummary counts branches thatits own
BRDAlines do not enumerate (1150 vs 656 formem_manager.cc).BRF/BRHis whatgenhtmlshows and is the conservative figure; the second column excludes branches in blocksthat were never entered at all, which is the more useful signal for "of the code these tests
reach, how much is half-tested".
BRF/BRHBRDA, entered blocks onlykernel_config.ccmem_manager.ccrome_topo_consensus.ccalt_rsmi.ccNote the two metrics disagree sharply for
kernel_config.cc(23% vs 86%) because three of itsfunctions are never called by any test in this binary, so most of its branches sit in blocks
that are never entered. The line figure staying flat at 18.92% is the honest signal there.
The
mem_managerteardown fix was verified empirically rather than by reading the source: anLD_PRELOADmalloc/free interposer with a negative control showed one outstanding allocationwithout it and zero with it, against an
Init_Successbaseline.(An earlier revision of this description reported branch figures using only the second
denominator, labelled as if they were the lcov-standard ones, and overstated the
mem_manager.ccdelta. The table above supersedes it.)Submission Checklist
🤖 Generated with Claude Code