Repository navigation
Fix deadlock in rocm-run - #560
Conversation
There was a problem hiding this comment.
Verdict: APPROVE -- submitted as COMMENT (automated reviews are advisory) · Findings: 0 (0 Critical, 0 Major, 0 Minor)
Scope
Single-file change to mlir/utils/widgets/rocm-run, a developer convenience wrapper around mlir-runner. The PR removes every fork() that happened before the final exec, because rocprofv3 profiles the script by LD_PRELOADing a library whose constructor spawns threads, and forking from that state can deadlock the child on an inherited lock. Three fork sites are replaced: $(cd … && pwd) command substitutions become parameter expansion, the ( cd … && pwd ) subshell-echo return convention becomes global assignment (BASEDIR / LLVM_BUILD) with a documented contract comment on each function, and the awk pipeline over CMakeCache.txt becomes a while IFS= read -r loop with an input redirect.
Findings
No blocking issues found.
Notes
- I walked the post-change file end to end for residual fork points and found none:
case,[[ ]],read,local, andechoare all bash builtins, anddone < "$cache"is a plain redirect (bash does not subshell a redirectedwhile). The stated goal is fully achieved. - Behavior-preservation spot-checks:
${line#*=}strips the shortest…=prefix, matching the oldsub(/^[^=]*=/, ""); theLLVM_DIR:*=*pattern still excludesLLVM_DIR-ADVANCED:INTERNAL=1as the surrounding comment claims; andbreakinsidecaseinsidewhilecorrectly exits the loop. Therepo_rootfallback is slightly more robust than before, since the oldcd … 2>/dev/null && pwdcould silently yield an empty string and probe/build. - Tradeoff worth being aware of (not a defect): the old
cd+pwdcollapsed..logically, whereas the now-unnormalized strings are resolved physically by the kernel. If abin/directory or$PWDcomponent is a symlink into another tree,$SCRIPT_DIR/..can land somewhere the old code would not. Unlikely for this script's install locations, and the file comment already flags the unnormalized paths — just noting the semantic delta is slightly wider than "cosmetic..segments". - rocMLIR back-port:
mlir/utils/widgets/is not in the "shared with rocMLIR" path list indocs/PR_REVIEW_CHECKLIST.md, so no back-port note is strictly required. That said, the script'sCMakeCache.txt/LLVM_DIRfallback targets the external-LLVM layout, which suggests a counterpart may exist upstream — worth a quick confirmation rather than a blocking finding. - The PR description leaves Test Plan and Test Result as empty placeholders. Since the motivation is a reproducible hang in
parameterSweeps-conv.py, a one-line confirmation that the sweep now completes underrocprofv3would make this self-documenting.
CI status
No checks in fail or cancel. py-checks, detect, Set System Property, and Kill old PR builds pass; Jenkins, Build and Test, MIGraphX, and Code coverage are still pending at the reviewed SHA. The review check is this auto-review pipeline itself.
Motivation
The paremeterSweeps-conv.py tests hangs.rocprofv3 profiles by LD_PRELOADing a library whose constructor spawns threads, and fork() in that state can deadlock the child on an inherited lock.
Technical Details
This rocm-run script must not fork before the final exec (no command substitutions, subshells, or pipes) due to rocprofv3. Paths are therefore built by parameter expansion and left unnormalized (they may contain ".." segments).
Test Plan
Test Result
Submission Checklist