Repository navigation
Conversation
is_ok_subnormal compared fp64 != 0.0 with a floating-point operator. Under FTZ (enabled by default at -O2+ by some compilers, e.g. Intel icc/icx via -fp-model=fast unless -fp-model=precise is given), the CPU treats a genuinely nonzero subnormal operand as 0.0 for this comparison without changing the value in memory, so the check wrongly rejects every subnormal float literal (e.g. "x = 5e-324") on such builds. Compare the raw bit pattern instead, which is unaffected by FTZ/DAZ hardware state. Fixes cktan#49.
Two independent CI failures, both in the vendored TOML parser:
- Every job built with -DHDF5_ENABLE_WARNINGS_AS_ERRORS=ON (the plain
-Werror jobs, Parallel Debug/Release Special, ROS3 VFD jni/ffm)
compiled tomlc17.c with HDF5's full internal warning set and failed
on real warnings in the vendored code (mostly -Werror=cast-align).
Third-party vendored code shouldn't be held to HDF5's own lint
standard, so src/CMakeLists.txt now appends -w (/W0 on MSVC) to that
one file's COMPILE_OPTIONS, the same way HDF5_DISABLE_COMPILER_WARNINGS
suppresses warnings for the whole build.
- The Intel oneAPI (icx) job failed tfilter2's test_config_canonicalization
("hex rewrite is value-transparent at powers of two") with "TOML parse
error ... error parsing float". Root cause: scan_float()'s subnormal
check compares the parsed value with "fp64 != 0.0", a floating-point
comparison. Intel's icc/icx enable flush-to-zero (FTZ) by default at
-O2 and above via -fp-model=fast, under which the CPU treats a
genuinely nonzero subnormal operand as 0.0 for this comparison without
altering the value in memory, so the check wrongly rejects every
subnormal float literal (reachable here via H5Z__rewrite_hexfloats()'s
hex-to-decimal rewrite of 0x1p-1074/0x1p-1073). Fixed by comparing the
raw bit pattern instead, which an integer comparison can't flush.
Reproduced locally by forcing FTZ+DAZ via SSE intrinsics around a
standalone harness; confirmed the exact failure before the fix and a
clean parse after it, with the existing upstream test suite
(scankey/scanvalue/parser/merge/cpp/source/equiv/pool) still passing.
Reported upstream as cktan/tomlc17#49, fix
proposed as cktan/tomlc17#50. Applied locally
ahead of an upstream release, following this file's existing pattern
for tracking un-released upstream fixes (see src/tomlc17/README.md).
Review: PR #50 — fix-ftz-subnormal-comparisonCommit: IntentParent commit Findings1. HIGH —
|
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | HIGH | tomlc17.c:2685 |
-1e-400 silently accepted as -0.0; underflow rejection broken for negatives. Mask sign bit. |
| 2 | MEDIUM | tomlc17.c:2685 |
isfinite() folds to true under -ffast-math/-ffinite-math-only, letting inf through — the exact build modes this PR targets. Test exponent bits instead. |
| 3 | LOW | tomlc17.c:2677 |
Comment blames FTZ; the operand-side flag is actually DAZ. |
| 4 | LOW | tomlc17.c:2684 |
memcpy length hardcodes 8 bytes instead of sizeof(fp64). |
Recommendation
Request changes. Fix #1 and #2 (both are confirmed correctness
regressions/holes verified by building and running, not just
inspection), add regression tests for -1e-400 and ideally a
fast-math build variant, and optionally fold in #3/#4. Then re-review
and merge.
Review feedback on cktan#50: the previous "fp64_bits != 0 && isfinite(fp64)" test had two holes. fp64_bits != 0 is true for -0.0, so strtod("-1e-400") -- which returns -0.0 with ERANGE -- was accepted, breaking the underflow rejection that 64a063b established and making the parser sign-asymmetric for the same input class. isfinite() is folded to 1 by gcc and clang under -ffast-math and -ffinite-math-only, so in exactly the fast-math builds this change targets, 1e400 was accepted as infinity. Mask off the sign bit and test the exponent bits directly, so the whole decision is integer work on the bits: accept an ERANGE result only when its magnitude is nonzero and below the infinity/NaN exponent. Also copy sizeof(fp64) rather than a hardcoded 8 bytes, with a static_assert on the width, and reword the comment -- FTZ/DAZ are only part of it, a fast-math build is also free to assume away subnormals and infinities at compile time. Adds test/scanvalue e3 (-1e-400) and e4 (1e400).
brtnfld
changed the title
|
Thanks — #1 and #2 both reproduce. Pushed Rather than patching the two tests separately, the whole decision is now integer work on the bits: static_assert(sizeof(fp64) == sizeof(uint64_t), "double must be 64 bits");
uint64_t fp64_bits;
memcpy(&fp64_bits, &fp64, sizeof(fp64));
uint64_t fp64_mag = fp64_bits & 0x7fffffffffffffffULL; // drop the sign bit
int is_ok_subnormal =
(errno == ERANGE) && fp64_mag != 0 && fp64_mag < 0x7ff0000000000000ULL;Masking the sign bit covers #1, #1 and #2 reproduce as describedBuilt the pre-fix tree against a harness that toggles FTZ+DAZ via SSE intrinsics:
#3 — the comment names the wrong flagYou're right that FTZ was wrong, but I couldn't back DAZ either, so I reworded around the mechanism rather than naming a bit. On x86-64 So neither flag explains the original icx failure in #49 at the hardware level. The plausible mechanism there is compile-time: TestsAdded Local run: Fast-math CI variant — not in this pushThe Makefiles set |
Review of cktan#50 asked for a build variant that exercises the compiler modes the scan_float() fix exists for. There was no way to add flags to a build: every Makefile assigns CFLAGS with ":=", so setting CFLAGS on the command line replaces the per-directory flags rather than adding to them. Append $(EXTRA_CFLAGS) to CFLAGS in each of the eleven Makefiles that define it, placed before the CXXFLAGS subst in simple/ and test/cpp/ so the C++ builds inherit it too. Make propagates a command-line variable to sub-makes, so one setting on the top-level make reaches every directory. The new job builds and tests with -ffast-math, which lets the compiler assume no operand is subnormal and every operand is finite -- the assumption behind both the original bug and the isfinite() hole found in review -- and which additionally sets FTZ+DAZ at process start, so the job covers the run-time flags as well. The folding happens at -O0 too, so it works with the workflow's existing DEBUG=1. Verified locally: scankey, scanvalue, parser, merge, cpp, source, equiv and pool all pass both with and without EXTRA_CFLAGS=-ffast-math.
Upstream review of cktan/tomlc17#50 found two holes in the version of that patch vendored here, both in the same is_ok_subnormal line: - "fp64_bits != 0" is true for -0.0, and strtod("-1e-400") returns -0.0 with ERANGE, so a negative underflow was accepted as -0 instead of rejected. This one needs no special build to reach -- it was live in every HDF5 configuration. - isfinite() is folded to 1 by gcc and clang under -ffast-math and -ffinite-math-only, and by icx under -fp-model=fast, so on exactly the Intel oneAPI job this patch exists to fix, "1e400" parsed as infinity. Confirmed against the vendored file before this change, built as HDF5 builds it: -O2 -O2 -ffast-math x = -1e-400 accept, -0 accept, -0 x = 1e400 reject accept, inf Replaced with the reviewed upstream form, which masks the sign bit and tests the exponent bits instead of calling isfinite(), so the whole decision is integer work on the bit pattern. Verbatim from upstream 9445ce3 apart from the vendoring reference comment; post-patch checksum in README.md updated to match.
The existing inf/nan cases use TOML's inf/nan keywords, which the parser tokenizes directly. Neither covers a literal that strtod() cannot represent, which is where both regressions found reviewing cktan/tomlc17#50 lived: tol = 1e400 overflowed to infinity and was accepted wherever a fast-math build had folded isfinite() away tol = -1e-400 underflowed to -0.0 and was accepted everywhere, the sign bit alone making the bit pattern nonzero Verified the -1e-400 case fails against the previously vendored parser and passes against the current one. The 1e400 case passes either way in a normal build -- it only bites under fast-math -- but it pins the contract down for a build that enables it.
My earlier comment here, and my comment on PR cktan#50, said the failure was a compile-time one and that ucomisd ignores DAZ. That was wrong. The test I based it on never verified that MXCSR had actually been written, so it was reading a no-op as evidence. With the register state checked, against the code as of 64a063b: default (FTZ=0 DAZ=0) x = 5e-324 -> ACCEPT FTZ only (FTZ=1 DAZ=0) x = 5e-324 -> ACCEPT FTZ+DAZ (FTZ=1 DAZ=1) x = 5e-324 -> REJECT So DAZ is the flag responsible, exactly as the review of this PR said, and FTZ on its own does nothing here -- FTZ acts on results, DAZ on inputs. Issue cktan#49 was right that this is an MXCSR-state bug; it just named the wrong bit. The isfinite() hole is separate and does hold as described: gcc and clang fold it to 1 under -ffast-math and -ffinite-math-only. The comment now gives both causes and says which one produced the report. No functional change -- the bit-pattern test already covers both.
|
Correction: my previous comment was wrong about the mechanism, and your I claimed DAZ is the flag responsible, exactly as you said -- and FTZ alone does Your finding #2 does hold as described -- gcc and clang fold Pushed One thing worth knowing about the fast-math CI job I added in |
|
Thanks for the fix, and for the careful follow-up on the DAZ mechanism. This landed on main as 4d2a53f, squash-merged locally with you as the author, so GitHub shows the PR as closed rather than merged. I trimmed it to the The bit-pattern check is still worth having for normal builds. Denormals-are-zero is process-wide, so an application linked with |
2 participants
Fixes #49.
is_ok_subnormalinscan_float()(added by #48 / 64a063b) compares the parsed value with a floating-point!=operator:Under flush-to-zero (FTZ) mode — the default for Intel's icc/icx at
-O2and above via-fp-model=fast, unless-fp-model=precise/-fp-model=strictis given — an SSE floating-point comparison against a genuinely nonzero subnormal operand is performed as if it were0.0, without changing the value in memory.is_ok_subnormalthen evaluates false and the parser rejects a valid float literal likex = 5e-324witherror parsing float, even thoughstrtod()returned the correctly rounded value.This PR swaps the comparison for a bit-pattern check, which is unaffected by FTZ/DAZ hardware state:
Verified locally by forcing FTZ+DAZ via SSE intrinsics around a small harness exercising
scan_float()/toml_parse(): the boundary subnormal literals fail to parse before this change and parse correctly after it. Ran the existing suite (make test) with the fix applied —scankey,scanvalue,parser,merge,cpp,source,equiv, andpoolall pass (stdtestneedsgo/toml-test, not available in my environment, so I couldn't exercise it either way).I didn't add a new test case to
test/scanvalue's data-driven harness, since that harness doesn't set FTZ/DAZ and so can't actually exercise this bug (it would just prove subnormal literals still parse under normal FPU state, which #48 already covers). Happy to add a small dedicated test that forces FTZ via SSE intrinsics if that's useful — wanted to check preference first rather than guess at test infra conventions.