Remove fast-math for clang build#1392
Draft
ThomsonTan wants to merge 1 commit into
Draft
Conversation
This was referenced Mar 16, 2026
bmehta001
added a commit
to bmehta001/cpp_client_telemetry
that referenced
this pull request
May 14, 2026
PR microsoft#1416 inadvertently regressed PR microsoft#1415 by removing -fno-finite-math-only from the three Unix REL_FLAGS branches and silencing the resulting Clang warning via -Wno-nan-infinity-disabled. That left -ffast-math implying -ffinite-math-only on Clang/AppleClang, which folds std::isnan / std::isinf to false in nlohmann::json (lib/include/mat/json.hpp) and breaks the IEEE 754 invariants that SQLite relies on for sqlite3_bind_double / sqlite3IsNaN. Rather than restore -fno-finite-math-only, address the root cause by removing -ffast-math entirely from all three Unix REL_FLAGS branches (GCC, AppleClang, generic Clang). For a telemetry library: - The hot paths are string concatenation, Bond/JSON serialization, HTTP I/O, and SQLite reads/writes - there is essentially no floating-point arithmetic for -ffast-math to optimize. - Defining __FAST_MATH__ causes some libc++ / glibc headers to shortcut isnan / isinf even with -fno-finite-math-only set, silently corrupting NaN handling in nlohmann::json output. - On x86 Linux, -ffast-math links crtfastmath.o which sets MXCSR FTZ/DAZ bits process-wide at startup, leaking flush-to-zero behavior into client applications that link this SDK - a contract violation for any consumer doing scientific compute or audio DSP. - SQLite source explicitly recommends against -ffast-math. This subsumes the partial mitigation from microsoft#1415 (-fno-finite-math-only is no longer needed once -ffast-math itself is gone) and aligns with the intent of the long-standing draft PR microsoft#1392 by ThomsonTan, extended to the GCC branch as well (GCC's -ffast-math has the same finite-math-only and crtfastmath.o behavior on x86 Linux). Also drop -Wno-nan-infinity-disabled from the Clang WARN_FLAGS - it only existed to silence the warning that flagged this exact bug, and is unnecessary once -ffast-math is no longer pulling in -ffinite-math-only. Validation: - cmake configure + Release build of UnitTests on macOS arm64 (AppleClang 21): no warnings, no -Wnan-infinity-disabled hits. - ./out/tests/unittests/UnitTests: 486/486 pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bmehta001
added a commit
to bmehta001/cpp_client_telemetry
that referenced
this pull request
May 15, 2026
PR microsoft#1416 inadvertently regressed PR microsoft#1415 by removing -fno-finite-math-only from the three Unix REL_FLAGS branches and adding -Wno-nan-infinity-disabled to silence the Clang diagnostic. That left release builds using -ffast-math without preserving the NaN/Inf semantics needed by nlohmann::json and SQLite paths. Remove -ffast-math entirely from the GCC, AppleClang, and generic Clang release flags rather than relying on -fno-finite-math-only to partially undo it. This SDK is not floating-point compute-bound; its hot paths are string, Bond/JSON serialization, HTTP I/O, and SQLite reads/writes. Avoiding -ffast-math: - preserves std::isnan/std::isinf behavior for JSON and storage code, - avoids compiler/runtime fast-math side effects such as x86 GCC's crtfastmath.o changing MXCSR FTZ/DAZ behavior process-wide, and - aligns with SQLite's guidance to avoid fast-math. This subsumes microsoft#1415's partial mitigation and aligns with microsoft#1392's intent, extended to GCC because GCC fast-math has the same broad assumptions and runtime side effects. Also remove -Wno-nan-infinity-disabled because the warning should not be suppressed once the cause is gone. Validation: - CMake Release build of UnitTests on macOS arm64 (AppleClang 21). - UnitTests passed on macOS arm64. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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.
No description provided.