Repository navigation
fix(rccl): [ppc64le] Fix test build break - #11460
Conversation
✅ 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.
ARGUS
Findings
1 finding(s) that fall outside the diff and could not be anchored to a line
projects/rccl/test/host/CMakeLists.txt:271(Low) Comment at CMakeLists.txt:267-276 now misdescribes netTransport
Bottom line: LGTM once #include "transport.h" moves up to the file's existing include block. That's mechanical, so it doesn't need another pass from me.
Comment @argus review to re-run, or @argus re-review after pushing.
|
Hi @pvallem we need 1 more review in order to merge this. Thanks! |
On ppc64le, the TOC (Table of Contents) entry for netTransport survives --gc-sections even though the code section referencing it is dead. This causes an undefined symbol error at link time for the MicroEnqueue host test. Add a zero-initialized stub definition so the symbol resolves. The x86 build is unaffected because its relocation model allows --gc-sections to eliminate the reference entirely.
23a0c62 to
079a1e9
Compare
There was a problem hiding this comment.
ARGUS
Findings
1 finding(s) that could not be posted as inline comments
projects/rccl/test/host/CMakeLists.txt:384(Low) Defining netTransport makes the undefined-symbol inventory comment in test/host/CMakeLists.txt stale, including three hard counts and the recount recipe below them DefiningnetTransportmakes the undefined-symbol inventory inprojects/rccl/test/host/CMakeLists.txtstale, and the recount recipe at lines 392-393 of that file will now report a number the comment contradicts: ''' line 382 "leaves 34 undefined" -> 33 line 383 "33 ordinary externals" -> 32 line 384 drop netTransport from the named symbols line 388 "All 34 that remain" -> 33 ''' The standalone block needs no edit, since line 966 of that file only cross-references this one.
Bottom line: LGTM once the symbol inventory in test/host/CMakeLists.txt drops netTransport and its three counts come down by one. That one is mechanical, so it does not need another pass from me.
Comment @argus review to re-run, or @argus re-review after pushing.
TheRock Submodule Bump ActivityNewest first
|
Motivation
Part of ROCm/TheRock#5518
Tests were failing to build on ppc64le architecture because of how dead code that is pruned on x86, is not pruned on ppc64le.
Technical Details
The
MicroEnqueuehost test (#10861) compilesenqueue.ccagainst stub implementations.enqueue.ccreferences the globalnetTransport(defined insrc/transport/net.cc) but the test does not link againstnet.cc. The code path that usesnetTransportis unreachable, so on x86--gc-sectionseliminates the reference and linking succeeds.On ppc64le, the ELFv2 ABI uses a TOC (Table of Contents) for global variable access. The compiler emits a TOC entry for
netTransportat compile time, and this TOC entry creates a hard symbol reference that survives--gc-sections— the linker can discard the code section but not the TOC relocation. This causes:The fix adds a zero-initialized definition of
netTransportintransport_stubs.cc. It is never called — it only satisfies the TOC reference.Issue Tracking
Test plan
netTransportwas already undefined and GC'd)Submission Checklist