Repository navigation
test(libc): run cpu_topology on Windows - #9584
DavidSamir wants to merge 2 commits into
Conversation
CpuTopology::Get and Pin have Windows paths (GetProcessAffinityMask, GetLogicalProcessorInformationEx efficiency classes, SetThreadAffinityMask) that no test ran. The test now builds on Windows too: the sysfs cpu-list parser checks stay Linux-only, the pinned mask is read back with SetThreadAffinityMask, and pinning by thread handle is checked.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
There was a problem hiding this comment.
The Windows assertions exercise process-mask filtering, current-thread pinning and the thread-handle path; the implementation changes shown in the PR leave the Linux assertions under their original platform guard. The latest conventions check passes, superseding the earlier template-check failure. Hosted relinker, register-context and write-watch checks pass; product build jobs are skipped. The Windows test result is author-reported; I did not run it locally.
The unresolved manual overlap with #6044 is specifically the moved CpuTopology.cpp, not just the shared CMake insertion. #6044 renames the Linux test to core/libs/tests/process/CpuTopology.cpp without changing its contents; this PR adds the Windows branch to the original path. Please coordinate with #6044's author so the Windows additions follow the file to its renamed path if #6044 lands first, and record the agreed order/dependency if one must land first. The PR's statement that merge-tree followed the rename should be reconciled with the overlap bot's explicit inability to compare the test path.
Please also update the test path/registration if the reorganization lands first; keeping the new Windows-only coverage on the unregistered old filename would drop it from CI.
|
The Conventions check fails on the head of this PR (job 119621777041: Everything else in the matrix is green (relinker on all three OSes, relinker-asan, register-context, write-watch), and |
There was a problem hiding this comment.
The test changes keep Linux behavior under the Linux guards and add Windows checks for process affinity, class layout, current-thread pinning and handle-based pinning. I also compiled the exact core/libs/tests/CpuTopology.cpp with GCC 12.2 (-std=c++20 -Wall -Wextra -Werror) and ran it standalone on Linux; it passed. This checks the shared/Linux path, not the new Windows-only assertions. The Windows 11 run and negative control in the PR description remain author-reported. One repository gate remains: 1. #6044 coordination: thank you for proposing on #6044 that the Windows cases follow CpuTopology.cpp to tests/process/ if its rename lands first. I do not see an agreement from that PR's author yet. Please wait for agreement and record the agreed merge order in the checklist (Depends on if this PR must wait), then carry the Windows cases and registration to the renamed path as needed. The overlap workflow cannot compare the shared files while #6044 conflicts with main. This head is 107 commits behind current upstream/main; please update it after the #6044 order is settled and rerun the checks. Checks on exact head 46c665015223ad582a26e90768b2c0f9dfd7b0e0: - git diff --check upstream/main...HEAD: passed. - python tools/check_conventions.py --base upstream/main --head HEAD: passed. - Standalone Linux test-source compile/run: passed (GCC 12.2; this does not test the Windows branch). - Hosted eligibility, conventions, labels, report, relinker matrix, ASan, register-context and write-watch checks pass; Linux/Windows product jobs are skipped. I am requesting changes for unresolved overlap agreement and base freshness only, not a queue-based intake hold or a defect in the added test logic. It is fine to keep submitting PRs; maintainers will handle them in due course.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
|
Independent Windows run, as data only. Machine: Windows 11 Enterprise 10.0.22621, AMD Ryzen 9 5950X (16 cores, 32 logical processors), WinLibs GCC 15.2.0 UCRT r7, Release. Head 46c6650 was merged with #9583 onto AI-assisted (Claude Code). |
|
The remaining review items are #6044 coordination and current-base freshness. I still do not see a reply from #6044’s author to your proposed ordering for moving the Windows test registration. This head is 150 commits behind current |
|
I’ve noticed that asking you to hold this PR was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. |
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 46c665015223ad582a26e90768b2c0f9dfd7b0e0 against 6ef5a1c4c92272598b18adeb9b5414112f8d15ed. No actionable findings in this focused review.
Source review: Inspected the complete registration/test change and CpuTopology's header-only implementation. Linux list parsing stays Linux-only. Windows checks compare SetThreadAffinityMask's previous-mask return value after Pin, reject masks outside the process and verify handle-based pinning. The CMake branch adds the Windows runtime/unwind setup and PATH consistently with neighboring native tests.
Validation: Windows / MinGW GCC 15.2: built the exact head CpuTopology.cpp against its pinned headers and ran it successfully (exit 0). Its runtime changes are confined to that test child. Baseline Windows compilation fails at the former unconditional Linux affinity APIs, demonstrating the portability change. Conventions, whitespace and script registration passed. Current hosted relinker/ASan and metadata checks succeeded; product Windows/Linux jobs were skipped.
Scope: Actual header-only CPU topology test on this Windows host; the full libc-linked CMake target and Linux runtime were not rebuilt/run locally. No game or GPU test.
|
Static review plus an independent Linux run (CachyOS, kernel 7.2.10, GCC 16.2.1, Ninja, Release, What I checked (static)
Focused test run
Not an AGC/shader/VideoOut change — no paired Vulkan-validation run required. Not covered The new Windows-only asserts ( |
What
cpu_topologywas registered only on Linux (libc/CMakeLists.txt:912).CpuTopology::GetandPinalso have Windows paths, which no test ran:GetProcessAffinityMask,LayoutFromClassesoverGetLogicalProcessorInformationExefficiency classes, andSetThreadAffinityMask.How:
libc/CMakeLists.txt: the guard becomesLinux OR WIN32. macOS stays excluded becauseGet/Pinhave no macOS path. The libc link,configure_windows_unwindand thePATHprepend apply on WIN32 only, so the Linux build is unchanged.CpuTopology.cpp: the sysfsMaskFromCpuListchecks move under#ifdef __linux__, since that function is Linux-only. On Windows the pinned mask is read back withSetThreadAffinityMask, which returns the mask it replaces. Pinning by thread handle is also checked, because the WindowsPinhonours handles. The Linux checks are unchanged.Tested
ctest -R ^cpu_topology$passes.GetProcessAffinityMaskinGet()disabled, the test fails with "process affinity read". Restored, it passes.tools/check_conventions.pypasses.Overlap (
tools/pr_overlap.py --branchplusgit merge-tree):tests/process/CpuTopology.cpp.git merge-treefollows the rename and reports no conflict there, and the libc CMakeLists conflict-hunk count against test: organize library tests and label CTest suites #6044 is unchanged (17).Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)