test(#2293): screen every generated tool log for sanitizer findings - #2299
Merged
Conversation
The dual-PCS regression from #2292 passed only the two iccApplyToLink logs through sanitizer_check(). The two iccFromXml logs and the six iccDumpProfile logs were never screened, and the iccDumpProfile calls had no status check at all, so a finding in the tools that build this test's fixtures and produce the output its assertions read went unreported. All twelve invocations now go through one choke point rather than five more sanitizer_check() calls, since an omitted call at a new call site is exactly how the gap arrived. The sibling harness already uses this idiom (iccdev-issue-1781-applytolink-qa-matrix.sh:118). The log is screened BEFORE the status is judged. halt_on_error=0 only keeps a tool alive where the build can recover, and nothing here passes -fsanitize-recover=address: Build/Cmake/CMakeLists.txt:977 adds -fno-sanitize-recover= for UBSan/integer/float only and SANITIZER_RECOVER defaults OFF (:584). So an ASan finding always aborts and arrives WITH a non-zero status, and only UBSan can report while still exiting zero, in the lane that sets SANITIZER_RECOVER=ON (ci-iccdev-tool-tests.yml:434). Judging the status first leaves the run red but reports every ASan abort as a plain tool failure. The stale comment that described the opposite ordering is rewritten rather than left to contradict this. iccDumpProfile gets its own variant, because with -v its exit code is a VALIDATION VERDICT and not a success flag: it ends "return nValid". The device links this test generates already validate as noncompliant -- the last severity that still maps to 0 -- so a plain status gate would turn a future validation tightening into a phantom #1982 regression while the D2B/B2D selection this test guards was intact. Same asymmetry documented in iccdev-qa-profile-manifest.sh:197-207 and Build/Cmake/Testing/CMakeLists.txt:6538. That tolerance is an ALLOW-list (0, 254, 255), not a blacklist, because the tool has more non-verdict exits than verdicts: 1 from the QA-flag argument paths (iccDumpProfile.cpp:582, :604), 253 from a failed dump write (:1010, :1053), and any signal. 253 truncates the log, which matters most in Part 2 where the assertion is that the two normalized chain dumps DIFFER: a truncation hitting one arm only would make them differ for a reason unrelated to tag selection. 255 needs two log pins on top of that, because three different things produce it: icValidateCriticalError, an unparseable profile (:703-706 forces the same verdict), and a bad invocation (:617, :632, :664). A bad invocation prints "Usage:" and returns before the version banner at :703, so requiring the banner catches it; an unparseable profile prints the banner and then "Unable to parse". Part 1 also gains the link-exists guard Part 2 already had. All three keep a tool-output failure from surfacing as fixture drift. The "-v 100" on the dumps is deliberate and is what makes that tolerance necessary: no grep pin reads the validation report, so dropping -v would let every dump use run_tool() and delete the variant entirely. It is kept because this test carries the "asan" label and CIccProfile::Validate() walks every tag -- measured, that is the only difference between the 165-line -v dump and the 160-line plain one -- so dropping it would shrink the code executed under the sanitizer, which is the coverage this change exists to protect. sanitizer_check's alternation gains LeakSanitizer, MemorySanitizer and DEADLYSIGNAL: dump_tool has to tolerate 254 and 255, so for those logs the regex is the only thing that can catch a finding which still exits with one of those codes. ICCDEV_TEST_ENV sets detect_leaks=0, but this script is runnable standalone against an ASan build where LSan is on by default. Measured with tool shims modelling each case, clang 21.1.3 ASan+UBSan: case before after iccFromXml finding, exit 0 PASS FAIL iccDumpProfile finding, exit 0 PASS FAIL iccDumpProfile UBSan runtime error PASS FAIL iccDumpProfile LeakSanitizer, exit 23 PASS FAIL iccDumpProfile SUMMARY: line only PASS FAIL iccDumpProfile exit 1 (arg path) PASS FAIL iccDumpProfile exit 253 (write fail) PASS FAIL iccDumpProfile exit 139 (SIGSEGV) PASS FAIL iccDumpProfile exit 255 (bad invocation) PASS FAIL iccApplyToLink exits 0, writes nothing PASS FAIL iccApplyToLink writes unparseable link PASS FAIL ASan report + abort (real ASan) FAIL* FAIL (*named a tool failure) iccDumpProfile exit 255 (verdict) PASS PASS iccDumpProfile exit 254 (unknown) PASS PASS iccApplyToLink finding FAIL FAIL Anti-regression: with the two CIccXform::Create conditions reverted to their pre-fix form the test still fails (exit 2) at the first opt-out chain, so the #1982 assertion is unweakened. shellcheck and bash -n clean; CTest #149 passes. A sanitizer report also survives normalize() into the two files Part 2 compares, carrying a per-process PID that differs between arms; screening before the comparison removes that interaction. It is a latent ordering hazard only, not a demonstrated false pass: on both regression shapes tried -- both conditions reverted, and the destination condition alone -- iccApplyToLink fails closed and Part 2 is never reached.
colourbill-ctrl
requested review from
ChrisCoxArt,
dwtza and
xsscx
as code owners
August 24, 2026 20:20
github-actions
Bot
added
Scripts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2293.
The regression added in #2292 screened only the two
iccApplyToLinklogs. The twoiccFromXmllogs and the sixiccDumpProfilelogs were never passed tosanitizer_check, and theiccDumpProfilecalls had no exit-status check at all.ICCDEV_TEST_ENVsetshalt_on_error=0for both sanitizers, so a UBSan report is printed while the tool still exits zero — an exit status cannot see it. All twelve invocations now go through one choke point rather than five moresanitizer_checkcalls, since an omitted call at a new call site is how the gap arrived.Red/green
Measured with tool shims modelling each case (clang 21.1.3, ASan+UBSan):
iccFromXmlfinding, exit 0iccDumpProfilefinding, exit 0iccDumpProfileUBSanruntime error:iccDumpProfileLeakSanitizer, exit 23iccDumpProfileSUMMARY:line onlyiccDumpProfileexit 1 (arg path)iccDumpProfileexit 253 (write failure)iccDumpProfileexit 139 (SIGSEGV)iccDumpProfileexit 255 (bad invocation)iccApplyToLinkexits 0, writes nothingiccApplyToLinkwrites an unparseable linkiccDumpProfileexit 255 (verdict)iccDumpProfileexit 254 (unknown)iccApplyToLinkfindingThe last three are the point of the
dump_toolsplit: with-v,iccDumpProfilereturns the validation verdict, not a success flag. The device links this test generates already validate as noncompliant — the last severity mapping to exit 0 — so a plain|| failwould turn a future validation tightening into a phantom #1982 regression. Same asymmetry documented aticcdev-qa-profile-manifest.sh:197-207.Anti-regression: with the two
CIccXform::Createconditions reverted to their pre-fix form the test still fails (exit 2), so the #1982 assertion is unweakened.shellcheckandbash -nclean; CTest #149 passes.On CI coverage — please read before trusting the checks
I dispatched
ci-pr-actionbefore opening this PR per the playbook (run 32772831218,ci_scope=fast-lane,ctest_recent_limit=0). It concludedsuccess, but every build and CTest lane wasskipped, so that green is not evidence for this change and I am not citing it as such.The reason is structural, not a misdispatch: a
.github/scripts/-only diff setsci_changedand nothing else,native_changedis set only by C/C++ extensions (ci-pr-action.yml:473), andtool-testsgates onnative_changed == true(:866). Noci_scopevalue changes that —run_full=truefromfast-lanedoes not reach this gate. Noting it as a fact rather than filing it: a change to a CTest harness script currently gets no CTest coverage, so the evidence here is the local matrix above.Scope
The same unscreened-log pattern exists in
iccdev-issue-1781-applytolink-qa-matrix.sh(plus fourexit 0skip paths with noSKIP_RETURN_CODE). Filed separately as #2298 rather than folded in — different harness, different CTest.