[Clang] Fix warning for non std functions with name `infinity` #123417

AmrDeveloper · 2025-01-17T22:51:05Z

Fix reporting diagnostic for non std functions that has the name infinity

Fixes: #123231

llvmbot · 2025-01-17T22:51:29Z

@llvm/pr-subscribers-clang

Author: Amr Hesham (AmrDeveloper)

Changes

Fix reporting diagnostic for non std functions that has the name infinity

Fixes: #123231

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

3 Files Affected:

(modified) clang/lib/Sema/SemaChecking.cpp (+21-1)
(modified) clang/test/Sema/warn-infinity-nan-disabled-lnx.cpp (+38-4)
(modified) clang/test/Sema/warn-infinity-nan-disabled-win.cpp (+39-4)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 881907ac311a30..3d2b2e1e96d2dd 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -8454,6 +8454,26 @@ static bool IsInfOrNanFunction(StringRef calleeName, MathCheck Check) {
   llvm_unreachable("unknown MathCheck");
 }
 
+static bool IsInfinityFunction(const FunctionDecl *FDecl) {
+  if (FDecl->getName() != "infinity") {
+    return false;
+  }
+
+  if (const CXXMethodDecl *MDecl = dyn_cast<CXXMethodDecl>(FDecl)) {
+    const CXXRecordDecl *RDecl = MDecl->getParent();
+    if (RDecl->getName() != "numeric_limits") {
+      return false;
+    }
+
+    if (const NamespaceDecl *NSDecl =
+            dyn_cast<NamespaceDecl>(RDecl->getDeclContext())) {
+      return NSDecl->isStdNamespace();
+    }
+  }
+
+  return false;
+}
+
 void Sema::CheckInfNaNFunction(const CallExpr *Call,
                                const FunctionDecl *FDecl) {
   FPOptions FPO = Call->getFPFeaturesInEffect(getLangOpts());
@@ -8469,7 +8489,7 @@ void Sema::CheckInfNaNFunction(const CallExpr *Call,
     bool IsInfOrIsFinite =
         IsStdFunction(FDecl, "isinf") || IsStdFunction(FDecl, "isfinite");
     bool IsInfinityOrIsSpecialInf =
-        HasIdentifier && ((FDecl->getName() == "infinity") ||
+        HasIdentifier && (IsInfinityFunction(FDecl) ||
                           IsInfOrNanFunction(FDecl->getName(), MathCheck::Inf));
     if ((IsInfOrIsFinite || IsInfinityOrIsSpecialInf) && FPO.getNoHonorInfs())
       Diag(Call->getBeginLoc(), diag::warn_fp_nan_inf_when_disabled)
diff --git a/clang/test/Sema/warn-infinity-nan-disabled-lnx.cpp b/clang/test/Sema/warn-infinity-nan-disabled-lnx.cpp
index 357c9e5b641073..4f46b777c88742 100644
--- a/clang/test/Sema/warn-infinity-nan-disabled-lnx.cpp
+++ b/clang/test/Sema/warn-infinity-nan-disabled-lnx.cpp
@@ -45,24 +45,48 @@ namespace std __attribute__((__visibility__("default"))) {
   isnan(double __x);
   bool
   isnan(long double __x);
-bool
+  bool
   isfinite(float __x);
   bool
   isfinite(double __x);
   bool
   isfinte(long double __x);
- bool
+  bool
   isunordered(float __x, float __y);
   bool
   isunordered(double __x, double __y);
   bool
   isunordered(long double __x, long double __y);
+
+template <class _Ty>
+class numeric_limits {
+public:
+    [[nodiscard]] static constexpr _Ty infinity() noexcept {
+        return _Ty();
+    }
+};
 } // namespace )
 }
 
 #define NAN (__builtin_nanf(""))
 #define INFINITY (__builtin_inff())
 
+template <>
+class std::numeric_limits<float>  {
+public:
+    [[nodiscard]] static constexpr float infinity() noexcept {
+        return __builtin_huge_val();
+    }
+};
+
+template <>
+class std::numeric_limits<double>  {
+public:
+    [[nodiscard]] static constexpr double infinity() noexcept {
+        return __builtin_huge_val();
+    }
+};
+
 template <class _Ty>
 class numeric_limits {
 public:
@@ -78,6 +102,7 @@ class numeric_limits<float>  {
         return __builtin_huge_val();
     }
 };
+
 template <>
 class numeric_limits<double>  {
 public:
@@ -86,6 +111,8 @@ class numeric_limits<double>  {
     }
 };
 
+double infinity() { return 0; }
+
 int compareit(float a, float b) {
   volatile int i, j, k, l, m, n, o, p;
 // no-inf-no-nan-warning@+4 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
@@ -225,11 +252,18 @@ int compareit(float a, float b) {
 
 // no-inf-no-nan-warning@+2 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
 // no-inf-warning@+1 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
-  double y = i * numeric_limits<double>::infinity();
+  double y = i * std::numeric_limits<double>::infinity();
+
+  y = i * numeric_limits<double>::infinity(); // expected-no-diagnostics
 
 // no-inf-no-nan-warning@+2 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
 // no-inf-warning@+1 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
-  j = numeric_limits<float>::infinity();
+  j = std::numeric_limits<float>::infinity();
+
+  j = numeric_limits<float>::infinity(); // expected-no-diagnostics
+
+  y = infinity(); // expected-no-diagnostics
+
   return 0;
 
 }
diff --git a/clang/test/Sema/warn-infinity-nan-disabled-win.cpp b/clang/test/Sema/warn-infinity-nan-disabled-win.cpp
index ee4eb33a16e449..655024f5909b33 100644
--- a/clang/test/Sema/warn-infinity-nan-disabled-win.cpp
+++ b/clang/test/Sema/warn-infinity-nan-disabled-win.cpp
@@ -48,24 +48,49 @@ namespace std __attribute__((__visibility__("default"))) {
   isnan(double __x);
   bool
   isnan(long double __x);
-bool
+  bool
   isfinite(float __x);
   bool
   isfinite(double __x);
   bool
   isfinte(long double __x);
- bool
+  bool
   isunordered(float __x, float __y);
   bool
   isunordered(double __x, double __y);
   bool
   isunordered(long double __x, long double __y);
+
+template <class _Ty>
+class numeric_limits {
+public:
+    [[nodiscard]] static constexpr _Ty infinity() noexcept {
+        return _Ty();
+    }
+};
+
 } // namespace )
 }
 
 #define INFINITY ((float)(1e+300 * 1e+300))
 #define NAN      (-(float)(INFINITY * 0.0F))
 
+template <>
+class std::numeric_limits<float>  {
+public:
+    [[nodiscard]] static constexpr float infinity() noexcept {
+        return __builtin_huge_val();
+    }
+};
+
+template <>
+class std::numeric_limits<double>  {
+public:
+    [[nodiscard]] static constexpr double infinity() noexcept {
+        return __builtin_huge_val();
+    }
+};
+
 template <class _Ty>
 class numeric_limits {
 public:
@@ -81,6 +106,7 @@ class numeric_limits<float>  {
         return __builtin_huge_val();
     }
 };
+
 template <>
 class numeric_limits<double>  {
 public:
@@ -89,6 +115,8 @@ class numeric_limits<double>  {
     }
 };
 
+double infinity() { return 0; }
+
 int compareit(float a, float b) {
   volatile int i, j, k, l, m, n, o, p;
 // no-inf-no-nan-warning@+2 {{use of infinity via a macro is undefined behavior due to the currently enabled floating-point options}}
@@ -216,11 +244,18 @@ int compareit(float a, float b) {
 
 // no-inf-no-nan-warning@+2 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
 // no-inf-warning@+1 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
-  double y = i * numeric_limits<double>::infinity();
+  double y = i * std::numeric_limits<double>::infinity();
+
+  y = i * numeric_limits<double>::infinity(); // expected-no-diagnostics
 
 // no-inf-no-nan-warning@+2 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
 // no-inf-warning@+1 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
-  j = numeric_limits<float>::infinity();
+  j = std::numeric_limits<float>::infinity();
+
+  j = numeric_limits<float>::infinity(); // expected-no-diagnostics
+
+  y = infinity(); // expected-no-diagnostics
+
   return 0;
 
 }

cor3ntin · 2025-01-18T13:54:35Z

clang/lib/Sema/SemaChecking.cpp

@@ -8469,7 +8489,7 @@ void Sema::CheckInfNaNFunction(const CallExpr *Call,
    bool IsInfOrIsFinite =


As an aside, this is called on every function call, which seems widely inficient.

can we do something like

FPOptions FPO = Call->getFPFeaturesInEffect(getLangOpts()); if(FPO.getNoHonorInfs()) return;

(and remove FPO.getNoHonorInfs() in the rest of the function)

Yes, I agree, i am thinking to change the structure of the function CheckInfNaNFunction too, because if HasIdentifier is false that's means IsStdFunction, IsInfOrNanFunction and IsInfinityFunction will return false, so maybe we can just eliminate some un needed function calls by early return.

I will try and update you with result :D

I think it's better now, early return if there is no identifier and check NoHonor.. first before checking for function name

cor3ntin

Can you add a changelog entry? Thanks

AmrDeveloper · 2025-01-18T19:46:44Z

Can you add a changelog entry? Thanks

Done, thanks

cor3ntin

LGTM with nits

cor3ntin · 2025-01-18T19:48:28Z

clang/lib/Sema/SemaChecking.cpp

+  if (FDecl->getName() != "infinity") {
+    return false;
+  }
+
+  if (const CXXMethodDecl *MDecl = dyn_cast<CXXMethodDecl>(FDecl)) {
+    const CXXRecordDecl *RDecl = MDecl->getParent();
+    if (RDecl->getName() != "numeric_limits") {
+      return false;
+    }
+
+    if (const NamespaceDecl *NSDecl =
+            dyn_cast<NamespaceDecl>(RDecl->getDeclContext())) {
+      return NSDecl->isStdNamespace();
+    }
+  }
+
+  return false;
+}
+


Suggested change

if (FDecl->getName() != "infinity") {

return false;

}

if (const CXXMethodDecl *MDecl = dyn_cast<CXXMethodDecl>(FDecl)) {

const CXXRecordDecl *RDecl = MDecl->getParent();

if (RDecl->getName() != "numeric_limits") {

return false;

}

if (const NamespaceDecl *NSDecl =

dyn_cast<NamespaceDecl>(RDecl->getDeclContext())) {

return NSDecl->isStdNamespace();

}

}

return false;

}

if (FDecl->getName() != "infinity")

return false;

if (const CXXMethodDecl *MDecl = dyn_cast<CXXMethodDecl>(FDecl)) {

const CXXRecordDecl *RDecl = MDecl->getParent();

if (RDecl->getName() != "numeric_limits")

return false;

if (const NamespaceDecl *NSDecl =

dyn_cast<NamespaceDecl>(RDecl->getDeclContext()))

return NSDecl->isStdNamespace();

}

return false;

}

cor3ntin · 2025-01-18T19:48:47Z

clang/lib/Sema/SemaChecking.cpp

+  if (!FDecl->getIdentifier()) {
+    return;
+  }
+


Suggested change

if (!FDecl->getIdentifier()) {

return;

}

if (!FDecl->getIdentifier())

return;

AmrDeveloper · 2025-01-18T19:54:07Z

LGTM with nits

Thank you, I addressed the nits

zahiraam · 2025-01-20T13:44:00Z

clang/lib/Sema/SemaChecking.cpp

-  if ((IsNaNOrIsUnordered || IsSpecialNaN) && FPO.getNoHonorNaNs()) {
+  if (FPO.getNoHonorNaNs() &&
+      (IsStdFunction(FDecl, "isnan") || IsStdFunction(FDecl, "isunordered") ||
+       IsInfOrNanFunction(FDecl->getName(), MathCheck::NaN))) {


I think for readability the intermediate bool variables can be kept. Same for the condition below.

I don't mind but my point was that functions are readable for example, for the first boolean, we can easily know the meaning from the condition itself.

bool IsInfOrIsFinite = IsStdFunction(FDecl, "isinf") || IsStdFunction(FDecl, "isfinite"); bool IsInfinityOrIsSpecialInf = HasIdentifier && ((FDecl->getName() == "infinity") || IsInfOrNanFunction(FDecl->getName(), MathCheck::Inf));

Also I am thinking of performing the checking only if FPO.getNoHonorInfs() is true, so to do this with variables either I need to introduce another variable for each case to perform other conditions then if it true, or merge it with IsInfOrIsFinite so it will be like IsNoHonorInfsAndIsInfOrIsFinite and IsNoHonorInfsAndIsInfinityOrIsSpecialInf then in the if condition makes sure one of them is true and I think that's less readable that the current code.

What do you think? @zahiraam

zahiraam · 2025-01-20T20:43:53Z

clang/lib/Sema/SemaChecking.cpp

 void Sema::CheckInfNaNFunction(const CallExpr *Call,
                               const FunctionDecl *FDecl) {
+  if (!FDecl->getIdentifier())


Wouldn't something like this work?

Suggested change

if (!FDecl->getIdentifier())

if (!FDecl->getIdentifier())

return;

FPOptions FPO = Call->getFPFeaturesInEffect(getLangOpts());

bool IsNaNOrIsUnordered =

IsStdFunction(FDecl, "isnan") || IsStdFunction(FDecl, "isunordered");

bool IsSpecialNaN =

IsInfOrNanFunction(FDecl->getName(), MathCheck::NaN);

bool IsInfOrIsFinite =

IsStdFunction(FDecl, "isinf") || IsStdFunction(FDecl, "isfinite");

bool IsInfinityOrIsSpecialInf =

IsInfOrNanFunction(FDecl->getName(), MathCheck::Inf);

if ((IsNaNOrIsUnordered || IsSpecialNaN) && FPO.getNoHonorNaNs()) {

Diag(Call->getBeginLoc(), diag::warn_fp_nan_inf_when_disabled)

<< 1 << 0 << Call->getSourceRange();

return;

}

if ((IsInfOrIsFinite || IsInfinityFunction(FDecl) ||

IsInfinityOrIsSpecialInf) &&

FPO.getNoHonorInfs()) {

Diag(Call->getBeginLoc(), diag::warn_fp_nan_inf_when_disabled)

<< 0 << 0 << Call->getSourceRange();

}

The problem with this, Is that in the case of FPO.getNoHonorNaNs() == false we performed un needed 3 function calls In IsNaNOrIsUnordered and IsSpecialNaN, and in case it's true we performed 3 more un needed too IsInfOrIsFinite, IsInfinityOrIsSpecialInf

bool IsNaNOrIsUnordered = IsStdFunction(FDecl, "isnan") || IsStdFunction(FDecl, "isunordered"); bool IsSpecialNaN = IsInfOrNanFunction(FDecl->getName(), MathCheck::NaN); if ((IsNaNOrIsUnordered || IsSpecialNaN) && false) { <--- Condition will evaluate to false }

This reorder can eliminate 3 function calls in the case of FPO.getNoHonorNaNs() == true, but still don't eliminate the IsNaNOrIsUnordered and IsSpecialNaN in case of FPO.getNoHonorNaNs() == false

bool IsNaNOrIsUnordered = IsStdFunction(FDecl, "isnan") || IsStdFunction(FDecl, "isunordered"); bool IsSpecialNaN = IsInfOrNanFunction(FDecl->getName(), MathCheck::NaN); if ((IsNaNOrIsUnordered || IsSpecialNaN) && FPO.getNoHonorNaNs()) { Diag(Call->getBeginLoc(), diag::warn_fp_nan_inf_when_disabled) << 1 << 0 << Call->getSourceRange(); return; } bool IsInfOrIsFinite = IsStdFunction(FDecl, "isinf") || IsStdFunction(FDecl, "isfinite"); bool IsInfinityOrIsSpecialInf = IsInfOrNanFunction(FDecl->getName(), MathCheck::Inf); if ((IsInfOrIsFinite || IsInfinityFunction(FDecl) || IsInfinityOrIsSpecialInf) && FPO.getNoHonorInfs()) { Diag(Call->getBeginLoc(), diag::warn_fp_nan_inf_when_disabled) << 0 << 0 << Call->getSourceRange(); }

Oh! right, That makes sense!

zahiraam

LGTM. Thanks.

shafik

Minor comment and quick question.

shafik · 2025-01-21T01:06:41Z

clang/test/Sema/warn-infinity-nan-disabled-win.cpp

+        return _Ty();
+    }
+};
+
 } // namespace )
 }


we should annotate the std namespace as well // std

shafik · 2025-01-21T01:08:02Z

clang/test/Sema/warn-infinity-nan-disabled-win.cpp


 // no-inf-no-nan-warning@+2 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
 // no-inf-warning@+1 {{use of infinity is undefined behavior due to the currently enabled floating-point options}}
-  j = numeric_limits<float>::infinity();
+  j = std::numeric_limits<float>::infinity();


So what happens if we are using using namespace std and use numeric_limits<...>::infinity() do we expect a diagnostic?

So what happens if we are using using namespace std and use numeric_limits<...>::infinity() do we expect a diagnostic?

Yes should expect a diagnostic because we check Decl name space not the call expr,

I will create a pr to cover this case and update minor comment related to namespace

I will tag you as reviewer once I push it

Thanks

erichkeane · 2025-01-21T14:43:27Z

clang/test/Sema/warn-infinity-nan-disabled-win.cpp

+  j = numeric_limits<float>::infinity(); // expected-no-diagnostics
+
+  y = infinity(); // expected-no-diagnostics
+


I would like a dependent test or two, to make sure we don't get this wrong/double-diagnose :

template<typename T> void foo() { std::numeric_limits<T>::infinity(); std::numeric_limits<double>::infinity(); } foo<float>();

I found that we got use of infinity is undefined behavior due to the currently enabled floating-point options for std::numeric_limits::infinity(); for each copy generated from foo function, I will check it more and handle it in different pr

The same problem will happen with isinf, isnan and isunordered functions because the diagnostic will be reported for every generated function, we need to think of a way to process it once @erichkeane

Sorry, I'm not getting your point in the previous message. Can you explain it more for me?

From my example, I would expect TWO diagnostics (one per use). But I don't want 3 or 4 diagnostics.

Yes I expected two too but what I got is two same warns on std::numeric_limits<double>::infinity(); and one on td::numeric_limits<T>::infinity(), I am trying to understand from where the extra one come from

We cannot really, since there is no such thing as 'phase 2' for most of the program (non templates). Typically if you want something ONLY during phase 2, you do it during tree-transform or during the calls to the 'Rebuild' functions.

So you ALSO have to do it during phase-1, but the phase-2 work can suppress its diagnostic if it has already been done (assuming you are close enough to rebuild to do that).

The fix is likely that you'll have to mvoe where this is diagnosed to somewhere that isn't currently being called by the template instantiation, and re-add it then.

Cool, I will understand the Sema structure more and move it :D

Thanks

Do we have a similar case with a place that covers most cases of using CallExpr, I found a function like CheckInvalidBuiltinCountedByRef, but it does not cover all cases @erichkeane

None I can think of at the moment unfortunately. If i run across one, I'll bring it up.

None I can think of at the moment unfortunately. If i run across one, I'll bring it up.

Thank you, I created a PR for the test cases, and I will continue searching for a good function or a place so I can create a one

[Clang] Fix invalid use of infinity warning

51dd53f

AmrDeveloper added clang Clang issues not falling into any other category clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer labels Jan 17, 2025

AmrDeveloper requested review from AaronBallman, erichkeane and zahiraam January 17, 2025 22:51

llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Jan 17, 2025

AmrDeveloper changed the title ~~[Clang] Fix invalid warning for non std functions with name infinity~~ [Clang] Fix warning for non std functions with name infinity Jan 17, 2025

cor3ntin reviewed Jan 18, 2025

View reviewed changes

Optimize CheckInfNaNFunction function

0023e15

AmrDeveloper requested a review from cor3ntin January 18, 2025 18:22

cor3ntin reviewed Jan 18, 2025

View reviewed changes

Add changelog entry

0bb6cf1

cor3ntin approved these changes Jan 18, 2025

View reviewed changes

Address code review nits

6b72d63

zahiraam reviewed Jan 20, 2025

View reviewed changes

zahiraam approved these changes Jan 20, 2025

View reviewed changes

AmrDeveloper merged commit e5992b6 into llvm:main Jan 20, 2025
9 checks passed

shafik reviewed Jan 21, 2025

View reviewed changes

erichkeane reviewed Jan 21, 2025

View reviewed changes

AmrDeveloper mentioned this pull request Jan 21, 2025

[Clang] Improve testing of non std functions with name infinity #123806

Open

		@@ -8469,7 +8489,7 @@ void Sema::CheckInfNaNFunction(const CallExpr *Call,
		bool IsInfOrIsFinite =

		j = numeric_limits<float>::infinity(); // expected-no-diagnostics

		y = infinity(); // expected-no-diagnostics

[Clang] Fix warning for non std functions with name infinity #123417

[Clang] Fix warning for non std functions with name infinity #123417

Uh oh!

Conversation

AmrDeveloper commented Jan 17, 2025

Uh oh!

llvmbot commented Jan 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper commented Jan 18, 2025

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper commented Jan 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zahiraam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shafik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[Clang] Fix warning for non std functions with name `infinity` #123417

[Clang] Fix warning for non std functions with name `infinity` #123417

AmrDeveloper Jan 20, 2025 •

edited

Loading

shafik left a comment •

edited

Loading

AmrDeveloper Jan 21, 2025 •

edited

Loading