Skip to content

fix(hipthreads): shorten Windows lit exec root to stay under MAX_PATH - #11783

Merged
marsavicc merged 2 commits into
developfrom
users/marsavic/hipthreads-issue-11488
Sep 23, 2026
Merged

marsavicc merged 2 commits into
developfrom
users/marsavic/hipthreads-issue-11488

Conversation

@marsavicc

@marsavicc marsavicc commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Closes #11488

Windows test binaries were dying with unexplained exit codes on deep test paths. test_exec_root pointed at the build tree, and this suite's deep libc++-style layout plus a long CI workspace prefix pushed %t/%t.d past MAX_PATH. Point it at a short %TEMP%-based path on Windows instead, nothing outside lit reads from it.

Note: with the exec root now under %TEMP%, prepare_execdir()'s short-scratch-dir fallback (#10939) no longer triggers in practice because every %t.d is short enough that it returns early, so the deep tests run in their real %t.d again instead of %TEMP%\hipthreads-test\<hash>. Nothing under test/std touches files relative to the cwd, so tests are unaffected, but it isn't visible from the diff. The fallback stays load-bearing regardless: gettempdir() is environment-driven, and it's the guard that engages first if the root grows (at a 75-char exec root, vs. 85 for the executable= path)

Also added a config-time warning (test/lit.cfg) that fires if test_exec_root or the test tree ever grow deep enough to approach MAX_PATH again, computed from the actual tree rather than a hardcoded depth.

@therock-pr-bot

therock-pr-bot Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🚫 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?

@therock-pr-bot

therock-pr-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@marsavicc

Copy link
Copy Markdown
Contributor Author

These 5 runs confirm that the fix should be working fine: https://github.com/ROCm/rocm-libraries/actions/runs/35224798168/job/105264360629

Run 3 has failed because of this issue that's not on us to fix: #11489

@aleksandrasavicc aleksandrasavicc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I'd add a warning so a longer exec root doesn't come back silently.

Comment thread projects/hipthreads/test/lit.cfg Resolved
Comment thread projects/hipthreads/test/lit.cfg Resolved
@marsavicc

Copy link
Copy Markdown
Contributor Author

@DanielMcIntosh @aleksandrasavicc, actually, since this PR makes prepare_execdir()'s TEMP-fallback branch in run.py (from #10939) basically unreachable on Windows, should we just remove it here instead of leaving dead code around? The \?-form directory creation and the executable=/lpApplicationName handling for long exe paths still earn their keep (exe paths can still exceed MAX_PATH even under a short root), but the "run in a short TEMP scratch dir" fallback and its hashing logic would have no callers left. Maybe we do it in this PR or a quick follow-up PR...

@aleksandrasavicc

Copy link
Copy Markdown
Contributor

@marsavicc On removing the fallback: both are driven by the same thing, the exec root length, and the fallback triggers first: cwd 248 vs exe 260, and the exe path is always 2 chars longer. So if the root ever grows, the fallback is the half that comes back first. Dropping it while keeping executable= keeps the wrong one.
And the root isn't ours, gettempdir() reads TEMP from the environment. Either way I'd keep this out of the PR.

@marsavicc

Copy link
Copy Markdown
Contributor Author

@marsavicc On removing the fallback: both are driven by the same thing, the exec root length, and the fallback triggers first: cwd 248 vs exe 260, and the exe path is always 2 chars longer. So if the root ever grows, the fallback is the half that comes back first. Dropping it while keeping executable= keeps the wrong one. And the root isn't ours, gettempdir() reads TEMP from the environment. Either way I'd keep this out of the PR.

Ooh you are right. Keeping both, and out of this PR.

@marsavicc
marsavicc merged commit 4795398 into develop Sep 23, 2026
105 of 106 checks passed
@marsavicc
marsavicc deleted the users/marsavic/hipthreads-issue-11488 branch September 23, 2026 10:12
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Issue] Windows : hipthreads — thread.mutex.class ctor.default/try_lock tests crash (exit status 0xffffffff)

2 participants