<atomic>: std::atomic<float>::fetch_add doesn't use safe FP environment

[Myriachan]

trafficstars

Describe the bug

std::atomic<FloatType>::fetch_add is supposed to always work, without undefined behavior. If you have floating-point exceptions unmasked, and fetch_add causes a floating-point exception, your program will crash.

This is apparently non-standard. Quoting the current draft:

Remarks: If the result is not a representable value for its type ([expr.pre]) the result is unspecified, but the operations otherwise have no undefined behavior. Atomic arithmetic operations on floating-point-type should conform to the std​::​numeric_limits<floating-point-type> traits associated with the floating-point type ([limits.syn]). The floating-point environment ([cfenv]) for atomic arithmetic operations on floating-point-type may be different than the calling thread's floating-point environment.

Command-line test case

C:\Temp>type repro.cpp
#include <atomic>
#include <float.h>

#pragma fenv_access(on)

std::atomic<float> g_test = 3.40282346639e+38f;

int main()
{
    unsigned original;
    _controlfp_s(&original, 0, _EM_OVERFLOW);

    float temp = g_test.load(std::memory_order_seq_cst);
    g_test.fetch_add(temp, std::memory_order_seq_cst);

    unsigned dummy;
    _controlfp_s(&dummy, original, _EM_OVERFLOW);
    return 0;
}

#pragma fenv_access(off)

C:\Temp>cl /Ox /std:c++20 .\repro.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.36.32538 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

repro.cpp
Microsoft (R) Incremental Linker Version 14.36.32538.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:repro.exe
repro.obj

C:\Temp>.\repro.exe

C:\Temp>echo %ERRORLEVEL%
-1073741679

Expected behavior

fetch_add's addition should use a safe floating-point environment.

STL version

Microsoft Visual Studio Professional 2022 (64-bit) - LTSC 17.6
Version 17.6.6

[Myriachan]

libc++ and libstdc++ have the same problem, so perhaps this is more an issue with the Standard than with the implementations?

[CaseyCarter]

(Disclaimer: I'm not the FP expert on our team.)

My initial response is to see this as a feature rather than a bug; the user requested non-standard behavior via the non-standard _controlfp_s. If you don't want overflow exceptions, why would you specifically enable them?

Defending against this in the library would be prohibitively expensive: IIRC saving / changing / restoring the floating-point control word is order(s) of magnitude more expensive than the operations we'd be guarding.

Am I missing something?

[Myriachan]

(Disclaimer: I'm not the FP expert on our team.)

My initial response is to see this as a feature rather than a bug; the user requested non-standard behavior via the non-standard _controlfp_s. If you don't want overflow exceptions, why would you specifically enable them?

While the methods of changing the floating-point environment are non-standard, the existence of the floating-point environment and its possible effects are mentioned in the Standard.

An equivalent here would be if in the future MSVC implemented an equivalent of -fsanitize=undefined: then the undefined behavior sanitizer would trap on the addition, which doesn't comply with the Standard.

Defending against this in the library would be prohibitively expensive: IIRC saving / changing / restoring the floating-point control word is order(s) of magnitude more expensive than the operations we'd be guarding.

That really depends on locality--lock cmpxchg can itself be more expensive than two ldmxcsrs. But yes.

I've brought this issue up in the C++ Standard Discussion mailing list, since it's something in the Standard no current implementation gets correct. It could be that the best way to solve the inconsistency is to change the Standard to match reality.

[AlexGuteniev]

I've brought this issue up in the C++ Standard Discussion mailing list, since it's something in the Standard no current implementation gets correct. It could be that the best way to solve the inconsistency is to change the Standard to match reality.

From the Standard side, I'd drop fetch_add on floating atomics entirely. It is anyway either a CAS equivalent or lock-based, there aren't atomic floating instructions.

[StephanTLavavej]

We talked about this at the weekly maintainer meeting and we believe that our implementation is following the Standard's vaguely mumbled guidance here. We're relabeling this issue as "LWG issue needed" to track potential clarifications to the Standardese.

[frederick-vs-ja]

The issue seems to be that passing _EM_MEOW constants to _controlfp_s causes non-Standard effects. I don't know why this should be considered limited to atomic<float>.

I guess this is pedantically OK since [lex.name]/3 states that using such an _UGLY_IDENTIFIER renders the program ill-formed, no diagnostic required. Especially, the Standard seemingly allows that usage of a reserved identifier turns the implementation into some non-conforming mode.