Skip to content

fix(tools): reject unmodeled save-temps modes in syntax checks - #11868

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
rooklz:fix/check-compile-save-temps-modes-v2
Oct 27, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
rooklz:fix/check-compile-save-temps-modes-v2

Conversation

@rooklz

@rooklz rooklz commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

What

A recorded compile command can carry -save-temps=<value> with a value other than the stripped cwd and obj modes. The syntax check passes such a joined form to the compiler, assuming an unknown mode is rejected there (6229cd07eb made the test skip whenever a compiler accepts unknown modes). Newer Clang builds do accept them: any value is taken as an output prefix, so on Apple clang 21.0.0 the command

$ clang -fsyntax-only -save-temps=invalid t.cpp ; echo rc=$? ; ls
rc=0
t.cpp  t.ii

exits 0 and writes t.ii even under -fsyntax-only (with -c it also writes t.bc, t.s, t.o). GCC instead fails on the same option, so the outcome of the check is toolchain-dependent: a silent file write on one compiler, a failure on another.

Reject -save-temps= and --save-temps= values other than the four already-stripped forms before the compiler runs, like the forwarded-action and -Wp rejections: the rewritten test no longer probes the compiler and no longer skips, and asserts the mode is reported, the compiler is never invoked and no output appears. The new scope is recorded in docs/dev/TechnicalDebt.md.

The rejection is placed after the -Wp, handling with an explicit not in DROP guard, so it does not touch the option-stripping block that #11522 extends; both changes merge in either order.

Tested

macOS 26.2, Apple M5 (arm64), Apple clang 21.0.0 (clang-2100.1.1.101), CMake/Ninja, ANYPS5_RELINKER_ONLY=ON, BUILD_TESTING=ON (Python 3.13). Base: 801d5b9bc1 (upstream/main), tested head: fe80227fce.

Host-only change to the Python tool; no title, GPU or console involved. No performance claim.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@MarcZX10 MarcZX10 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.

I reviewed exact head fe80227fce02056fefbcbacaaa9aaec4cf823517. The new guard rejects unsupported joined -save-temps= / --save-temps= values before invoking the compiler, while the exact supported cwd and obj forms remain handled by DROP. The regression checks both spellings and several invalid values with a compiler marker to prove no compiler invocation or output file occurs.

I ran python3 -m unittest tools.tests.test_check_compile tools.tests.test_check_compile_modes from this head: 77 tests passed, 14 skipped. The new regression ran and passed. The open #11522 change also edits syntax_arguments; a three-way merge preview against its current head is clean, and the edits apply at separate locations.

Hosted policy checks and ASan pass. Linux, Windows and both macOS relinker jobs are still pending; the full 361-case hosted suite has not completed yet. I found no actionable code issue in this review, but I am leaving this as a comment until the platform jobs finish.

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 907 docs/dev/TechnicalDebt.md
PR 1368 docs/dev/TechnicalDebt.md
PR 1384 docs/dev/TechnicalDebt.md
PR 1395 docs/dev/TechnicalDebt.md
PR 1601 docs/dev/TechnicalDebt.md
PR 1835 docs/dev/TechnicalDebt.md
PR 3986 docs/dev/TechnicalDebt.md
PR 5021 docs/dev/TechnicalDebt.md

@MarcZX10 MarcZX10 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.

The previously pending Ubuntu, Windows, both macOS relinker jobs, and ASan checks have now completed successfully. The 77 focused Python tests (14 skipped) passed in the reviewed head, including the new regression; the merge preview against #11522 was clean. I found no actionable issue in the exact reviewed diff. Product-level Linux/Windows jobs are skipped by repository policy, so this approval covers the tooling change and checks listed above.

@babyhuey

Copy link
Copy Markdown
Contributor

Windows run of this head fe80227fce0, merged onto main 8ff4baf5348 (clean merge, local only), with the selection CI's skipped Windows product job uses: Windows 11, Ryzen 7 5800X, RTX 3080, WinLibs GCC 15.2, Release, BUILD_TESTING=ON. The tree was configured fresh after the merge, then select_tests.py select / ninja -f select.ninja selected / --target libs / select_tests.py ctest --parallel 4 --timeout 120.

  • Selected 435 tests: 429 pass, 0 fail, 6 skip. No failures; the new check_compile_test_unmodeled_saved_temporary_modes_are_rejected_before_the_compiler_runs passes.

AI-assisted: yes (Claude Code)

@oneandonlydean oneandonlydean self-assigned this Oct 27, 2026
@github-actions github-actions Bot added the docs label Oct 27, 2026
@oneandonlydean
oneandonlydean merged commit 200f408 into boykopovar:main Oct 27, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants