fix(#1948): read float buffers as bytes in the CIccIO writers, and record the corpus verdicts (#1946) - #1947
Merged
Merged
Conversation
…very corpus profile The Testing corpus mixes three kinds of profile that a "did iccDumpProfile exit non-zero" sweep cannot tell apart: 107 conformant examples, 28 carrying a known baselined warning, and 78 that are malformed on purpose. A sweep that fails on a non-zero exit reports the 78 deliberate negatives as unexplained failures; one that tolerates them cannot see a conformant profile that has newly turned critical, because "some profiles are critical" is already the normal state. Testing/qa-profile-manifest.tsv records the expected verdict per profile and a CTest enforces it. Suite is derived from the verdict rather than hand-assigned, so the manifest stays regenerable and nobody adjudicates 213 rows by hand. Per the resolutions agreed on #1809: sha256 is populated only for the 80 git-tracked profiles, because 178 of the 191 tracked Testing XML fixtures set <CreationDateTime>now</CreationDateTime>, which resolves through localtime_r at conversion time and feeds the recomputed profile ID - so the generated corpus is neither clock- nor timezone-stable. expected_status and expected_exit are recorded as independent fields because iccDumpProfile maps noncompliant to exit 0.
colourbill-ctrl
requested review from
ChrisCoxArt,
dwtza,
maxderhak and
xsscx
as code owners
August 1, 2026 23:54
xsscx
enabled auto-merge (squash)
August 1, 2026 23:57
Member
CI Error Log2026-08-02 00:14:21 UTC |
This was referenced Aug 2, 2026
…iters
CIccIO::Write16/Write32/Write64 loaded the caller's buffer through the wide
integer type before byte-swapping it:
icUInt32Number *ptr = (icUInt32Number*)pBuf32;
tmp = *ptr;
WriteFloat32Float forwards straight into Write32 whenever icFloatNumber and
icFloat32Number are the same width, which is the normal build, so the object
being read is a float. A float and an icUInt32Number are not similar types, so
that load is undefined: the compiler may assume a store through one cannot be
observed by a load through the other. ENABLE_LTO defaults to ON, which puts the
store in CIccSegmentedCurve::Write and this load in one optimization scope, and
clang at -O3 drops the store as dead. Every breakpoint then reached the file as
the stack slot's initial 0.0.
The read path was never affected because icSwab32Array walks the buffer as
icUInt8Number, which may alias any object. That asymmetry is why readers agreed
across toolchains while writers did not. The writers now walk the buffer the
same way, reordering bytes into a small scratch buffer.
The bytes are shuffled directly rather than copied out with memcpy. Both forms
are well defined, but memcpy is intercepted under ASAN, and a call per element
cost about 20% on iccdev.issue-1781-applytolink-qa-matrix - enough to push it
past its timeout under parallel load, since these writers carry every CLUT a
profile contains. The byte-shuffle form measures slightly faster than the
original undefined code (24.7s against 26.3s on that test).
The damage is not cosmetic. A segmented curve whose trailing breakpoint is
flattened to 0.0 gives its sampled segment an end point equal to its start
point, and CIccSampledCurveSegment::Begin refuses a zero range, so the curve set
cannot be applied at all. Any profile written by an optimized clang build -
including the default macOS toolchain - carries the damage.
gcc is unaffected today but is not immune: nothing in the standard obliges it to
keep honouring a load it is entitled to reorder.
Testing/qa-profile-manifest.tsv records Calc/RGBWProjector.icc as valid rather
than carrying a baselined zero-range warning. That row was measured on a
clang+LTO host, where the defect bites; the corpus is generated from XML at test
time, so the manifest had captured this miscompilation as expected behaviour.
With the writers fixed, all 191 tracked Testing XML fixtures convert to
byte-identical profiles under clang and gcc, modulo the creation date and
profile ID that the header records per conversion.
.github/ci/regression/iccio-write-float-aliasing.cpp drives
CIccSegmentedCurve::Write and asserts the emitted breakpoint bytes, then reads
them back and confirms the curve still begins. Both the store and the load live
inside IccProfLib, so the test reproduces wherever the library itself is
miscompiled, independently of how the test translation unit is built. It fails
on the unfixed tree with the exact 0x00000000-for-0x3F800000 substitution.
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
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.
Fixes #1946.
Fixes #1948.
#1809 carried two halves: the empty-processing-element policy, which landed as #1887, and
the QA manifest, which @xsscx authorized on 2026-07-28 in
#1809 (comment)
("Go with PoLR paths based on your preference & resolution"), taking both resolutions I
proposed:
sha256scoped to what git tracks, andnoncompliantrecorded together withexpected_exit: 0rather than assuming the two correlate. #1809 closed when #1887 merged,so the manifest half is filed separately as #1946 and this PR delivers it.
The problem
The corpus mixes three kinds of profile that a "did
iccDumpProfileexit non-zero" sweepcannot tell apart: conformant examples, profiles carrying a known baselined warning, and
fixtures that are malformed on purpose. 78 of the 213 profiles are deliberate negatives.
Without a manifest they either read as unexplained failures, or — if a sweep tolerates
them — they mask a conformant profile that has newly turned critical.
What the corpus actually does
Measured in a clean worktree, rebased onto
c95a8343. That matters: a working tree thathas run the suite carries 232
.iccunderTesting/where a clean checkout has 213, so measuring inplace produces the wrong baseline.
Suite is derived from the verdict rather than hand-assigned, so the manifest stays
regenerable and nobody adjudicates 213 rows by hand.
Four things that came out of the measurement
1. All 27 warnings are #1811. Every one is the
spectralViewingConditionsilluminantXYZ/surroundXYZ"appears to be normalized"diagnostic. That gives #1811 a measured scope it did not have: fixing it moves all 27
profiles from
warningtovalid, leaving zerocompatibilityrows, and is a deliberatere-baseline of this manifest. Corrected on the issue in
#1811 (comment) —
my first census said 27 of 28, with
Calc/RGBWProjector.iccas an unrelated zero-rangesampled curve. That profile was never a warning; it was the #1948 miscompilation described
below.
2. No profile validates as overall
noncompliant. The asymmetry #1809 asked me toencode has no live instance.
NonCompliant! -appears as a per-tag diagnostic line inquantity, but never as the profile verdict. The status-to-exit mapping is recorded in the
manifest header as the explicit contract, and the header says plainly that no row exercises
that pairing today, rather than implying coverage that does not exist.
3.
calcUnderStack_fLab.iccdoes not report a stack underflow. It reports"function has invalid operations" — the operation is rejected before stack depth is ever
assessed — unlike its 73 siblings. Recorded as its own reason so the manifest does not
imply it exercises the same validator path. Flagging as an observation, not a defect claim.
4. The issue's repro passes
ALL; it makes no difference here. I measured bothiccDumpProfile -v 26 <f>andiccDumpProfile --diag -v 26 <f> ALLacross all 213profiles: identical status line and identical exit code in every case. The test uses the
cheaper form and runs in ~5s.
sha256 scoping
80 tracked profiles carry a digest; the 133 generated ones carry
-. 178 of the 191tracked
TestingXML fixtures set<CreationDateTime>now</CreationDateTime>, whichicGetDateTimeValueresolves throughlocaltime_rat conversion time and which then feedsthe recomputed profile ID — so those bytes are neither clock- nor timezone-stable.
Re-verified for this PR: two conversions of
Calc/CameraModel.xmltwo seconds apart differ.The digest is read from the git object rather than from disk, so regenerating the corpus in
place cannot silently re-baseline it.
Scope
Covers
Testing/only. The 24 profiles tracked under.github/ci/regression/and.github/ci/test-data/are deliberately excluded and the manifest header says so: each isalready the input to a dedicated regression test whose assertion is narrower than a
whole-profile verdict.
Testing/hybrid/ICCandTesting/hybrid/Resultsare excludedbecause they are
iccdev.hybrid-pipelineoutput rather than corpus —Testing/hybrid/.gitignoredeclares exactly those two paths as generated, and it is theonly
.gitignoreunderTesting/that declares.iccoutput. Without that exclusion thetest is ordering-dependent, seeing 213 profiles alone and 232 after the hybrid pipeline has
run.
Verification
c95a8343, both toolchains: 0 build warnings and 146/147ctestunder gcc strict ASAN+UBSan Release (CI's own configuration) and under clang Release with
LTO. The single failure in both is
spectral-tiff-preview, which fails because this localenvironment lacks the python
imagecodecsmodule - unrelated, and failing before thischange.
expected_sanitizer: noneisverified rather than asserted.
missing from the manifest, a manifest row whose profile is gone, tracked-fixture digest
drift, and sanitizer detection. All five fail as intended; the unmodified control passes.
generateis idempotent: regenerating produces a byte-identical manifest.scoping
sha256workable.shellcheckclean under CI's own invocation. Avoidsdeclare -A: macOS ships bash 3.2and nothing else under
.github/scriptsrequires bash 4.The second half: #1948, and why this PR grew
The manifest above was red in CI on exactly one row, and the row was right about my machine
rather than about the corpus. Chasing that produced a real
IccProfLibdefect.What it is
CIccIO::Write16/Write32/Write64loaded the caller's buffer through the wide integertype before byte-swapping it:
// IccProfLib/IccIO.cpp, CIccIO::Write32 icUInt32Number *ptr = (icUInt32Number*)pBuf32; tmp = *ptr;WriteFloat32Floatforwards straight intoWrite32whenevericFloatNumberandicFloat32Numberare the same width, which is the normal build. So the object being read isa
float, read through anicUInt32Numberlvalue. Those are not similar types, so thecompiler may assume a store through one is not observable by a load through the other.
ENABLE_LTOdefaults toON, which puts the store inCIccSegmentedCurve::Writeand thisload into one optimization scope. clang at
-O3then drops the store as dead. That functionreuses a single stack slot for every breakpoint, so the slot kept its initial zero and every
breakpoint reached the file as
0.0.The read path was never affected:
icSwab32Arraywalks the buffer asicUInt8Number, whichmay alias any object. That asymmetry is why readers agreed across toolchains while writers
did not.
Why it matters
Not cosmetic. A flattened trailing breakpoint gives the sampled segment an end point equal to
its start point, and
CIccSampledCurveSegment::Begin(IccMpeBasic.cpp:1187) refuses a zerorange — so the curve set cannot be applied at all. Any profile written by an optimized clang
build carries this, including the default macOS toolchain.
gcc is unaffected today, but by the optimizer's choice rather than by the standard.
Evidence
Same source, same commit, unpatched tree, only the build changed:
-O3, LTO on (project default)0x00000000-O3 -fno-strict-aliasing0x3F800000-O3+ ASAN/UBSan0x3F8000000x3F800000-O30x3F800000-fno-strict-aliasingalone flipping it at the same-O3is the discriminator. Note thatneither ASAN nor UBSan catches this, so a green sanitizer lane is not evidence of absence.
The fix
The writers now walk the buffer as
unsigned charand reorder bytes into a small scratchbuffer, matching what
icSwab32Arrayhas always done on the read side.The bytes are shuffled directly rather than copied out with
memcpy. Both forms are welldefined, but
memcpyis intercepted under ASAN, and a call per element cost about 20% oniccdev.issue-1781-applytolink-qa-matrix— enough to push it past its timeout under parallelload, since these writers carry every CLUT a profile contains:
Write32formmemcpyVerification
.github/ci/regression/iccio-write-float-aliasing.cppdrivesCIccSegmentedCurve::Writeand asserts the emitted breakpoint bytes, then reads them back and confirms the curve still
begins. Both the store and the load live inside
IccProfLib, so it reproduces wherever thelibrary is miscompiled, independently of how the test translation unit is built.
second breakpoint wrote 0x00000000, expected 0x3F800000.Testing/*.xmlnow convert to byte-identical profiles under clang-O3and gcc
-O3, modulo the creation date (24-35) and profile ID (84-99) recorded perconversion.
hybrid/CMYK-STop_Overprint_Profilealso differs at 2685017-2685083, which isthe same date/ID pair of an embedded profile at offset 2684984, not a divergence.
Calc/calcImport.xml,Calc/calcVars.xmlandSpecRef/RefEstimationImport.xmlare notstandalone-convertible under either toolchain.
The one manifest row
Calc/RGBWProjector.iccmoves fromcompatibility/warningtopositive/valid. It isgenerated from XML at test time, so the baseline had captured this miscompilation as expected
behaviour. Regenerating the manifest instead of fixing the writers would have written my
host's miscompilation into the repository — the row count that had to change was derived from
CI's own
212 matched, 1 mismatched, not from a wholesale re-baseline.