Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C23] Add INFINITY and NAN macros to <float.h> #96659

Merged
merged 7 commits into from
Jul 3, 2024

Conversation

AaronBallman
Copy link
Collaborator

This is in support of WG14 N2848 which only define the macros if
infinity and nan are supported, so use of -ffinite-math will cause the
macros to not be defined.

This is in support of WG14 N2848 which only define the macros if
infinity and nan are supported, so use of -ffinite-math will cause the
macros to not be defined.
@AaronBallman AaronBallman added c23 clang:headers Headers provided by Clang, e.g. for intrinsics floating-point Floating-point math labels Jun 25, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 labels Jun 25, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 25, 2024

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

This is in support of WG14 N2848 which only define the macros if
infinity and nan are supported, so use of -ffinite-math will cause the
macros to not be defined.


Full diff: https://github.com/llvm/llvm-project/pull/96659.diff

4 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+5)
  • (modified) clang/lib/Headers/float.h (+6)
  • (modified) clang/test/Headers/float.c (+29)
  • (modified) clang/www/c_status.html (+1-1)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index df579ae398c5e..45c9a2cc47b09 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -336,6 +336,11 @@ C23 Feature Support
 - Properly promote bit-fields of bit-precise integer types to the field's type
   rather than to ``int``. #GH87641
 
+- Added the ``INFINITY`` and ``NAN`` macros to Clang's ``<float.h>``
+  freestanding implementation; these macros were defined in C99 but Clang
+  implements the macros as described by C23 in
+  `WG14 N2848 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2848.pdf>`_.
+
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
diff --git a/clang/lib/Headers/float.h b/clang/lib/Headers/float.h
index 642c8f06cc938..929c0854a865b 100644
--- a/clang/lib/Headers/float.h
+++ b/clang/lib/Headers/float.h
@@ -47,6 +47,8 @@
     (defined(__cplusplus) && __cplusplus >= 201103L) ||                        \
     (__STDC_HOSTED__ && defined(_AIX) && defined(_ALL_SOURCE))
 #    undef DECIMAL_DIG
+#    undef INFINITY
+#    undef NAN
 #  endif
 #  undef FLT_DIG
 #  undef DBL_DIG
@@ -93,6 +95,10 @@
 #if (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) ||              \
     (defined(__cplusplus) && __cplusplus >= 201103L)
 #define FLT_EVAL_METHOD __FLT_EVAL_METHOD__
+#if defined(__FINITE_MATH_ONLY__) && !__FINITE_MATH_ONLY__
+#  define INFINITY (__builtin_infinity())
+#  define NAN (__builtin_nan(""))
+#endif
 #endif
 #define FLT_ROUNDS (__builtin_flt_rounds())
 #define FLT_RADIX __FLT_RADIX__
diff --git a/clang/test/Headers/float.c b/clang/test/Headers/float.c
index 70c11b0537537..8dfbc32521ade 100644
--- a/clang/test/Headers/float.c
+++ b/clang/test/Headers/float.c
@@ -1,9 +1,13 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c89 -ffreestanding %s
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c99 -ffreestanding %s
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -ffreestanding %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c23 -ffreestanding %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c23 -ffreestanding -ffinite-math-only %s
 // RUN: %clang_cc1 -fsyntax-only -verify -xc++ -std=c++11 -ffreestanding %s
 // RUN: %clang_cc1 -fsyntax-only -verify -xc++ -std=c++14 -ffreestanding %s
 // RUN: %clang_cc1 -fsyntax-only -verify -xc++ -std=c++17 -ffreestanding %s
+// RUN: %clang_cc1 -fsyntax-only -verify -xc++ -std=c++23 -ffreestanding %s
+// RUN: %clang_cc1 -fsyntax-only -verify -xc++ -std=c++23 -ffreestanding -ffinite-math-only %s
 // expected-no-diagnostics
 
 /* Basic floating point conformance checks against:
@@ -107,10 +111,35 @@
     #elif   DECIMAL_DIG < 10
         #error "Mandatory macro DECIMAL_DIG is invalid."
     #endif
+
+    #if __FINITE_MATH_ONLY__ == 0
+        #ifndef INFINITY
+            #error "Mandatory macro INFINITY is missing."
+        #endif
+        #ifndef NAN
+            #error "Mandatory macro NAN is missing."
+        #endif
+    #else
+        #ifdef INFINITY
+            #error "Macro INFINITY should not be defined."
+        #endif
+
+        #ifdef NAN
+            #error "Macro NAN should not be defined."
+        #endif
+    #endif
 #else
     #ifdef DECIMAL_DIG
         #error "Macro DECIMAL_DIG should not be defined."
     #endif
+
+    #ifdef INFINITY
+        #error "Macro INFINITY should not be defined."
+    #endif
+
+    #ifdef NAN
+        #error "Macro NAN should not be defined."
+    #endif
 #endif
 
 
diff --git a/clang/www/c_status.html b/clang/www/c_status.html
index 04c1df9ebc050..bc37f8cd5e404 100644
--- a/clang/www/c_status.html
+++ b/clang/www/c_status.html
@@ -991,7 +991,7 @@ <h2 id="c2x">C23 implementation status</h2>
     <tr>
       <td>Contradiction about INFINITY macro</td>
       <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2848.pdf">N2848</a></td>
-      <td class="unknown" align="center">Unknown</td>
+      <td class="unreleased" align="center">Clang 19</td>
     </tr>
     <tr>
       <td>Require exact-width integer type interfaces</td>

@AaronBallman
Copy link
Collaborator Author

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about tying this to __FINITE_MATH_ONLY__; -ffinite-math-only doesn't mean infinity doesn't exist, it just means you're promising that you won't use floating-point arithmetic/comparison ops on infinity. Which is weird, but that's fast-math. Also, other implementations don't do this.

In C99, INFINITY and NAN are macros in math.h, not float.h. Is it correct to check __STDC_VERSION__ >= 199901L?

@AaronBallman
Copy link
Collaborator Author

I'm not sure about tying this to __FINITE_MATH_ONLY__; -ffinite-math-only doesn't mean infinity doesn't exist, it just means you're promising that you won't use floating-point arithmetic/comparison ops on infinity. Which is weird, but that's fast-math. Also, other implementations don't do this.

The requirement that you only define the macro if inf/nan "is supported" is new in C23, so I don't expect anyone to have implemented this yet. As for __FINITE_MATH_ONLY__, our documentation mention "results" and that implies (at least to me) that an expression cannot form an infinity to begin with, so the act of trying to expand INFINITY is nonsensical in that case, right?

In C99, INFINITY and NAN are macros in math.h, not float.h. Is it correct to check STDC_VERSION >= 199901L?

In C99, C11, and C17 INFINITY and NAN are only defined in math.h. In C23, the definition is now in both math.h and float.h. The float.h requirement comes from 5.2.5.3.3 Characteristics of floating types <float.h> (p29 and p3), and the math.h requirement comes from 7.12 Mathematics <math.h> (p7 and p9). I'm not 100% certain of why they're in both, but I think it's for freestanding conformance reasons (math.h isn't [always] in freestanding but float.h [always] is).

@efriedma-quic
Copy link
Collaborator

and that implies (at least to me) that an expression cannot form an infinity to begin with, so the act of trying to expand INFINITY is nonsensical in that case, right?

It's undefined behavior at runtime.

I don't think we need to worry too much about what the C standard says here; the language around floating-point types that don't have infinity was written in the old days before everyone settled on using IEEE math. And I think the warning we currently produce is probably more helpful in practice than "INFINITY not defined".


What I was getting at with the standard version thing is that in standard versions before C23, I don't think NAN is a reserved identifier if you don't include math.h. So it's forbidden for float.h to define it.

@AaronBallman
Copy link
Collaborator Author

AaronBallman commented Jun 25, 2024

and that implies (at least to me) that an expression cannot form an infinity to begin with, so the act of trying to expand INFINITY is nonsensical in that case, right?

It's undefined behavior at runtime.

I don't think we need to worry too much about what the C standard says here; the language around floating-point types that don't have infinity was written in the old days before everyone settled on using IEEE math. And I think the warning we currently produce is probably more helpful in practice than "INFINITY not defined".

I looked at my meeting notes for discussion of this paper and I think we do need to worry about what the C standard says. From my notes: The big intent from this change seems to be about making INFINITY to be a feature test macro., so if users are going to portably expect #ifdef INFINITY to mean that their use of the macro will generate an actual infinity, we really should support that IMO.

Also, we can't really rely on UB at runtime because of constant expressions have a constraint that says constant expressions have to evaluate to a constant in the range of representable values for its type. With C23 adding constexpr, this constraint matters more for floating-point computations than it did previously.

What I was getting at with the standard version thing is that in standard versions before C23, I don't think NAN is a reserved identifier if you don't include math.h. So it's forbidden for float.h to define it.

Oh, that's a good point! I'll guard this by C23 and not C99 once we figure out what it means to "support an infinity".

@jcranmer-intel
Copy link
Contributor

I looked at my meeting notes for discussion of this paper and I think we do need to worry about what the C standard says. From my notes: The big intent from this change seems to be about making INFINITY to be a feature test macro., so if users are going to portably expect #ifdef INFINITY to mean that their use of the macro will generate an actual infinity, we really should support that IMO.

I went looking through the CFP history of the proposal. It's based on a comp.std.c thread here: https://groups.google.com/g/comp.std.c/c/XCzEiwD9n_A, which became a CFP discussion here: https://mailman.oakapple.net/pipermail/cfp-interest/2021-October/002216.html (and more in the September portion of the archives, but the relevant part of the discussion is in October), and the minutes of the CFP meeting where this was discussed here: https://mailman.oakapple.net/pipermail/cfp-interest/2021-October/002253.html

The entire history of this discussion is about FP types that do not have a format for infinity (say, VAX FP), and there was commentary on the CFP thread that there was evidence that users thought that INFINITY was conditionally defined like NAN was. Note that NAN has been similarly conditionally defined for a long time, and gcc doesn't undefine it in -ffinite-math-only mode, so that's strong precedent for not undefining INFINITY in fast-math.

@AaronBallman
Copy link
Collaborator Author

I looked at my meeting notes for discussion of this paper and I think we do need to worry about what the C standard says. From my notes: The big intent from this change seems to be about making INFINITY to be a feature test macro., so if users are going to portably expect #ifdef INFINITY to mean that their use of the macro will generate an actual infinity, we really should support that IMO.

I went looking through the CFP history of the proposal. It's based on a comp.std.c thread here: https://groups.google.com/g/comp.std.c/c/XCzEiwD9n_A, which became a CFP discussion here: https://mailman.oakapple.net/pipermail/cfp-interest/2021-October/002216.html (and more in the September portion of the archives, but the relevant part of the discussion is in October), and the minutes of the CFP meeting where this was discussed here: https://mailman.oakapple.net/pipermail/cfp-interest/2021-October/002253.html

The entire history of this discussion is about FP types that do not have a format for infinity (say, VAX FP), and there was commentary on the CFP thread that there was evidence that users thought that INFINITY was conditionally defined like NAN was. Note that NAN has been similarly conditionally defined for a long time, and gcc doesn't undefine it in -ffinite-math-only mode, so that's strong precedent for not undefining INFINITY in fast-math.

I spoke with the chair of CFP and he agreed with my assessment that INFINITY and NAN should not be defined in -ffinite-math-only mode, at least in terms of intent behind N2848. He also pointed out that not defining the macro is a security win because it makes it harder for people to write code using that macro and expecting it to do anything useful, which I strongly agree with.

Aside from "compatibility/historically", do we have any technical arguments against following the intent of the standard here? (We already deviate from GCC in so many other ways, I'm not worried about a deviation here; I would presume GCC will eventually get around to doing the same thing, at least for C23 and up).

@AaronBallman
Copy link
Collaborator Author

I was thinking about this more last night. The standard says:

The macro
INFINITY
is defined if and only if the implementation supports an infinity for the type float. It expands to a
constant expression of type float representing positive or unsigned infinity.

Some interesting things to note:

  • "if and only if" is clear that there are conditions under which INFINITY will not be defined.
  • "an infinity" means some plausible, correct infinity value; because we #pragma float_control(precise, on), there is a way in which to re-enable infinities for a block of code even if -ffinite-math-only is passed on the command line. That makes it defensible to always define INFINITY. However...
  • "expands to a constant expression of type float representing positive or unsigned infinity" still prevents us from having a valid definition of the macro when infinity is disabled; we cannot make a valid constant expression because constant expressions are prohibited from overflowing, and because true infinity is considered representable but has properties that mean it doesn't overflow when used with mathematical operators. e.g., INFINITY + 1.0f is a valid constant expression only when true infinity is supported.
  • You might think "but that's fine, we can add special handling for infinity to our constant expression interpreter", except there's a requirement for constant expressions that The semantic rules for the evaluation of a constant expression are the same as for nonconstant expressions.. It would also mean that fpclassify (etc) will classify such a value as an infinity at constant expression time, but not at runtime.
  • We could also argue that -ffinite-math-only is a language dialect and so we could say "aha, none of the standard's rules apply!", but I think that's leaning on a technicality rather than helping users to write correct code. In general, we want language dialects to track as closely to the standard as they can because that's what makes the language dialect approachable for users.

I think that not defining INFINITY when in finite-math-only mode is the better option for the largest number of our users. People who want to play fast math games can call __builtin_inf() directly, but I think the fact that INFINITY won't actually behave like the standard expects it to behave and the standard asks us not to define it in that case is a compelling reason to not define it. I think that's the best user experience for people who aren't fast math aficionados (which is basically the majority of users, IMO).

WDYT?

@jcranmer-intel
Copy link
Contributor

Some thoughts of my own:

The decision being discussed here has two main repercussions:

  • An attempt to use INFINITY gets a symbol-not-found error message, or it gets whatever warning/error message we attach to __builtin_inf() in -ffinite-math-only mode.
  • Code that uses #ifndef INFINITY may or may not trigger.

In the first case, I think you get a better error message with the __builtin_inf() usecase directly. For the second case, well, the main question is "why would someone use this in the first place." From a search on sourcegraph for existing uses of check for the INFINITY macro, by far the single most common case was to immediately define their own version of INFINITY by constructing an infinite value, usually by intentional overflow.

Most programmers, if they have any understanding of floating-point, would understand floating point only in terms of IEEE 754 types, which have an infinity; the existence of floating-point formats that lack infinities is going to be new to them, much less the ability to test such systems. So the lack of an INFINITY macro is likely to be seen less as "this compiler doesn't support infinities" and more likely to be seen as "this compiler doesn't support a new enough standard," hence the workarounds to construct an appropriate infinite value. (Many of the comments actually explicitly state that they are trying to construct an infinity!)

Code that attempts to intentionally use infinity in a -ffinite-math-only mode is going to break. The choice to me is giving them something where we will immediately complain that their combination of command-line flags and code is likely to do the wrong thing, or giving them something that--in practice--amounts to silently breaking their code, where we could have obviously told them their code was broken.

If the user is explicitly savvy about the existence of fast-math, they already have the opportunity to protect themselves from users who blindly apply -ffast-math, be it the __FINITE_MATH_ONLY macro checks or pragmas to re-enable it. Not defining INFINITY for their benefit doesn't give them a lot more control, while reducing our ability to help users who may have blundered themselves into a nonsense configuration.

In short: IMHO, INFINITY should expand to something that bops the user (i.e., warn/error by default) and says "you're doing something wrong" in finite-math-only mode.

@AaronBallman
Copy link
Collaborator Author

In short: IMHO, INFINITY should expand to something that bops the user (i.e., warn/error by default) and says "you're doing something wrong" in finite-math-only mode.

@jcranmer-intel and I had a really nice offline discussion on this topic and I've come around to his view. I was thinking that the fact we don't currently diagnose use of __builtin_inf() in -ffinite-math-only mode was because it produced an actual infinity still, but after our discussion, I now agree that it (and __builtin_nan()) should diagnose if used when infinities (or NANs) are disabled. This also nicely handles the case with pragmas changing the floating-point mode to be different from what was set on the command line.

So let's split this PR into two parts: 1) I'll update this PR to always define INFINITY and NAN in <float.h>, 2) I'll introduce the warning diagnostic in a follow-up PR.

* Now defines the macros even in -ffinite-math-only mode
* Defines the macros only in C23 and up, not in C99 and up
* Fixed the test coverage, added tests for use in a constant expression
* Fixed the expansion of the INFINITY macro (should resolve a precommit CI issue)
Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have -Wnan-infinity-disabled, which I think is sufficient on the warning side? Haven't checked carefully to see what cases it covers.

clang/lib/Headers/float.h Outdated Show resolved Hide resolved
@AaronBallman
Copy link
Collaborator Author

We already have -Wnan-infinity-disabled, which I think is sufficient on the warning side? Haven't checked carefully to see what cases it covers.

Not this one. :-D https://godbolt.org/z/hq8jvse9o But yeah, that may be a good diagnostic to consider putting this under, but I think that diagnostic should be enabled by default when passing -Wfinite-math-only or other flags that disable infinity or nan support.

Copy link

github-actions bot commented Jul 1, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff de68294b4dd31370773cb7a976b2d59e0e8b9bcc 3f6278781a1cd8d2366e0c55cd19e559e6ee1549 -- clang/lib/Headers/float.h clang/test/Headers/float.c
View the diff from clang-format here.
diff --git a/clang/lib/Headers/float.h b/clang/lib/Headers/float.h
index a565a33243..77e48138b9 100644
--- a/clang/lib/Headers/float.h
+++ b/clang/lib/Headers/float.h
@@ -90,8 +90,8 @@
 
 #if (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L) ||              \
     !defined(__STRICT_ANSI__)
-#  undef INFINITY
-#  undef NAN
+#undef INFINITY
+#undef NAN
 #endif
 
 /* Characteristics of floating point types, C99 5.2.4.2.2 */
@@ -163,9 +163,9 @@
 
 #if (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L) ||              \
     !defined(__STRICT_ANSI__)
-   /* C23 5.2.5.3.3p29-30 */
-#  define INFINITY (__builtin_inf())
-#  define NAN (__builtin_nan(""))
+/* C23 5.2.5.3.3p29-30 */
+#define INFINITY (__builtin_inf())
+#define NAN (__builtin_nan(""))
 #endif
 
 #ifdef __STDC_WANT_IEC_60559_TYPES_EXT__

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AaronBallman AaronBallman merged commit bcb7c38 into llvm:main Jul 3, 2024
7 of 8 checks passed
@AaronBallman AaronBallman deleted the aballman/inf_nan_float_h branch July 3, 2024 11:24
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
This is in support of WG14 N2848 which only define the macros if
an infinity or nan are supported. However, because we support builtins
that can produce an infinity or a NAN, and because we have pragmas
that control infinity or NAN behavior, we always define the macros even
if the user passed -ffinite-math-only on the command line.
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
This is in support of WG14 N2848 which only define the macros if
an infinity or nan are supported. However, because we support builtins
that can produce an infinity or a NAN, and because we have pragmas
that control infinity or NAN behavior, we always define the macros even
if the user passed -ffinite-math-only on the command line.
@bgra8
Copy link
Contributor

bgra8 commented Jul 8, 2024

@AaronBallman N2848 explicitly mentions that NAN and INFINITY macros should expand to a constant expression of type float not double like was implemented in this patch.

__builtin_nanf and __builtin_inff should have been used instead, right?

@AaronBallman
Copy link
Collaborator Author

@AaronBallman N2848 explicitly mentions that NAN and INFINITY macros should expand to a constant expression of type float not double like was implemented in this patch.

__builtin_nanf and __builtin_inff should have been used instead, right?

WHOOOOOOPPPPSSS yup, looks like I also missed some test coverage. :-D I'll get that fixed up ASAP, thank you!

@AaronBallman
Copy link
Collaborator Author

@AaronBallman N2848 explicitly mentions that NAN and INFINITY macros should expand to a constant expression of type float not double like was implemented in this patch.
__builtin_nanf and __builtin_inff should have been used instead, right?

WHOOOOOOPPPPSSS yup, looks like I also missed some test coverage. :-D I'll get that fixed up ASAP, thank you!

This should be fixed now in 2f2b931, thank you!

@bgra8
Copy link
Contributor

bgra8 commented Jul 8, 2024

@AaronBallman N2848 explicitly mentions that NAN and INFINITY macros should expand to a constant expression of type float not double like was implemented in this patch.
__builtin_nanf and __builtin_inff should have been used instead, right?

WHOOOOOOPPPPSSS yup, looks like I also missed some test coverage. :-D I'll get that fixed up ASAP, thank you!

This should be fixed now in 2f2b931, thank you!

Thanks a lot for the quick fix!

daltenty added a commit that referenced this pull request Aug 8, 2024
since C23 this macro is defined by float.h, which clang implements in
it's float.h since #96659 landed.

However, regcomp.c in LLVMSupport happened to define it's own macro with
that name, leading to problems when bootstrapping. This change renames
the offending macro.
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 8, 2024
since C23 this macro is defined by float.h, which clang implements in
it's float.h since llvm#96659 landed.

However, regcomp.c in LLVMSupport happened to define it's own macro with
that name, leading to problems when bootstrapping. This change renames
the offending macro.

(cherry picked from commit 899f648)
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 10, 2024
since C23 this macro is defined by float.h, which clang implements in
it's float.h since llvm#96659 landed.

However, regcomp.c in LLVMSupport happened to define it's own macro with
that name, leading to problems when bootstrapping. This change renames
the offending macro.

(cherry picked from commit 899f648)
kstoimenov pushed a commit to kstoimenov/llvm-project that referenced this pull request Aug 15, 2024
since C23 this macro is defined by float.h, which clang implements in
it's float.h since llvm#96659 landed.

However, regcomp.c in LLVMSupport happened to define it's own macro with
that name, leading to problems when bootstrapping. This change renames
the offending macro.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 c23 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category floating-point Floating-point math
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants