Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 20, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51dd53f
[Clang] Fix invalid use of infinity warning
AmrDeveloper Jan 17, 2025
0023e15
Optimize CheckInfNaNFunction function
AmrDeveloper Jan 18, 2025
0bb6cf1
Add changelog entry
AmrDeveloper Jan 18, 2025
6b72d63
Address code review nits
AmrDeveloper Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,8 @@ Improvements to Clang's diagnostics
}
- Diagnose invalid declarators in the declaration of constructors and destructors (#GH121706).

- Fix false positives warning for non-std functions with name `infinity` (#123231).

Improvements to Clang's time-trace
----------------------------------

Expand Down
47 changes: 32 additions & 15 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8454,26 +8454,43 @@ 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) {
if (!FDecl->getIdentifier())
Copy link
Contributor

Choose a reason for hiding this comment

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

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();
}

Copy link
Member Author

@AmrDeveloper AmrDeveloper Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! right, That makes sense!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you

return;

FPOptions FPO = Call->getFPFeaturesInEffect(getLangOpts());
bool HasIdentifier = FDecl->getIdentifier() != nullptr;
bool IsNaNOrIsUnordered =
IsStdFunction(FDecl, "isnan") || IsStdFunction(FDecl, "isunordered");
bool IsSpecialNaN =
HasIdentifier && IsInfOrNanFunction(FDecl->getName(), MathCheck::NaN);
if ((IsNaNOrIsUnordered || IsSpecialNaN) && FPO.getNoHonorNaNs()) {
if (FPO.getNoHonorNaNs() &&
(IsStdFunction(FDecl, "isnan") || IsStdFunction(FDecl, "isunordered") ||
IsInfOrNanFunction(FDecl->getName(), MathCheck::NaN))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Diag(Call->getBeginLoc(), diag::warn_fp_nan_inf_when_disabled)
<< 1 << 0 << Call->getSourceRange();
} else {
bool IsInfOrIsFinite =
IsStdFunction(FDecl, "isinf") || IsStdFunction(FDecl, "isfinite");
bool IsInfinityOrIsSpecialInf =
HasIdentifier && ((FDecl->getName() == "infinity") ||
IsInfOrNanFunction(FDecl->getName(), MathCheck::Inf));
if ((IsInfOrIsFinite || IsInfinityOrIsSpecialInf) && FPO.getNoHonorInfs())
Diag(Call->getBeginLoc(), diag::warn_fp_nan_inf_when_disabled)
<< 0 << 0 << Call->getSourceRange();
return;
}

if (FPO.getNoHonorInfs() &&
(IsStdFunction(FDecl, "isinf") || IsStdFunction(FDecl, "isfinite") ||
IsInfinityFunction(FDecl) ||
IsInfOrNanFunction(FDecl->getName(), MathCheck::Inf))) {
Diag(Call->getBeginLoc(), diag::warn_fp_nan_inf_when_disabled)
<< 0 << 0 << Call->getSourceRange();
}
}

Expand Down
42 changes: 38 additions & 4 deletions clang/test/Sema/warn-infinity-nan-disabled-lnx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -78,6 +102,7 @@ class numeric_limits<float> {
return __builtin_huge_val();
}
};

template <>
class numeric_limits<double> {
public:
Expand All @@ -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}}
Expand Down Expand Up @@ -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;

}
43 changes: 39 additions & 4 deletions clang/test/Sema/warn-infinity-nan-disabled-win.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should annotate the std namespace as well // std


#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:
Expand All @@ -81,6 +106,7 @@ class numeric_limits<float> {
return __builtin_huge_val();
}
};

template <>
class numeric_limits<double> {
public:
Expand All @@ -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}}
Expand Down Expand Up @@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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>();

Copy link
Member Author

@AmrDeveloper AmrDeveloper Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

return 0;

}
Loading