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

[LLDB] Operators <= and >= return a wrong result when comparing with a floating point NaN in expression evaluation #85947

Closed
kuilpd opened this issue Mar 20, 2024 · 4 comments · Fixed by #108060
Labels

Comments

@kuilpd
Copy link
Contributor

kuilpd commented Mar 20, 2024

Operators <= and >= are defined through operator< (Scalar.cpp:857), which in turn calls a compare function. If the compare function returns cmpUnordered, operator< return false, which gets negated and operators <= and >= return an incorrect value true.

Source file with 2 comparisons, to see the operator results from the compiler:

#include <limits>

int main() {
  float fnan = std::numeric_limits<float>::quiet_NaN();
  double dnan = std::numeric_limits<double>::quiet_NaN();
  bool float_comparison = 1.0f <= fnan;
  bool double_comparison = 1.0 >= dnan;
  return 0;
}

LLDB log:

(lldb) expr float_comparison
(bool) $0 = false
(lldb) expr double_comparison
(bool) $1 = false
(lldb) expr 1.0f <= fnan
(bool) $2 = true
(lldb) expr 1.0 >= dnan
(bool) $3 = true

Maybe operators <= and >= should analyze result separately, like in APFloat.h:1240.

@kuilpd kuilpd added the lldb label Mar 20, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 20, 2024

@llvm/issue-subscribers-lldb

Author: Ilia Kuklin (kuilpd)

Operators `<=` and `>=` are defined through `operator<` ([Scalar.cpp:857](https://github.com/llvm/llvm-project/blob/7812fcf3d79ef7fe9ec6bcdfc8fd9143864956cb/lldb/source/Utility/Scalar.cpp#L857)), which in turn calls a compare function. If the compare function returns `cmpUnordered`, `operator<` return `false`, which gets negated and operators `<=` and `>=` return an incorrect value `true`.

Source file with 2 comparisons, to see the operator results from the compiler:

#include &lt;limits&gt;

int main() {
  float fnan = std::numeric_limits&lt;float&gt;::quiet_NaN();
  double dnan = std::numeric_limits&lt;double&gt;::quiet_NaN();
  bool float_comparison = 1.0f &lt;= fnan;
  bool double_comparison = 1.0 &gt;= dnan;
  return 0;
}

LLDB log:

(lldb) expr float_comparison
(bool) $0 = false
(lldb) expr double_comparison
(bool) $1 = false
(lldb) expr 1.0f &lt;= fnan
(bool) $2 = true
(lldb) expr 1.0 &gt;= dnan
(bool) $3 = true

Maybe operators &lt;= and &gt;= should analyze result separately, like in APFloat.h:1240.

@Michael137
Copy link
Member

Maybe operators <= and >= should analyze result separately, like in APFloat.h:1240.

Thanks for the report and analysis. Agreed, checking for the possibility of an unordered comparison result seems like the way to go. operator== suffers from the same issue I guess

@kuilpd
Copy link
Contributor Author

kuilpd commented Mar 20, 2024

operator== suffers from the same issue I guess

In this particular case 1.0f != fnan is supposed to be true anyway. Although there might be other cases where just negating the result of == is incorrect, I'm not sure.

@Explorer09
Copy link

Wait. Wasn't the != relation meant to cover unordered result in terms of IEEE 754?

(a != b) and !(a == b) should be strictly equivalent. It is the <> relation that might need to address the unordered results.

kuilpd added a commit that referenced this issue Sep 18, 2024
…g to a floating point NaN (#108060)

Implement operators `<=` and `>=` to explicitly check the comparison
results to be `cmpLessThan` or `cmpEqual` instead of negating the result
of `operators<`.

Fixes #85947
tmsri pushed a commit to tmsri/llvm-project that referenced this issue Sep 19, 2024
…g to a floating point NaN (llvm#108060)

Implement operators `<=` and `>=` to explicitly check the comparison
results to be `cmpLessThan` or `cmpEqual` instead of negating the result
of `operators<`.

Fixes llvm#85947
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants