[compiler-rt][nsan] Add support for nan detection #101531
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: pokeslow (cseslowpoke) ChangesAdd support for nan detection Full diff: https://github.com/llvm/llvm-project/pull/101531.diff 3 Files Affected:
diff --git a/compiler-rt/lib/nsan/nsan.cpp b/compiler-rt/lib/nsan/nsan.cpp
index 568a1b3a65575..9c505193daa00 100644
--- a/compiler-rt/lib/nsan/nsan.cpp
+++ b/compiler-rt/lib/nsan/nsan.cpp
@@ -444,6 +444,14 @@ int32_t checkFT(const FT value, ShadowFT Shadow, CheckTypeT CheckType,
const InternalFT check_value = value;
const InternalFT check_shadow = Shadow;
+ // We only check for NaNs in the value, not the shadow.
+ if (flags().check_nan && isnan(check_value)) {
+ Decorator D;
+ Printf("%s", D.Warning());
+ Printf("WARNING: NumericalStabilitySanitizer: NaN detected\n");
+ Printf("%s", D.Default());
+ }
+
// See this article for an interesting discussion of how to compare floats:
// https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/
static constexpr const FT Eps = FTInfo<FT>::kEpsilon;
diff --git a/compiler-rt/lib/nsan/nsan_flags.inc b/compiler-rt/lib/nsan/nsan_flags.inc
index 63c15475f6754..2c67c4bb7f779 100644
--- a/compiler-rt/lib/nsan/nsan_flags.inc
+++ b/compiler-rt/lib/nsan/nsan_flags.inc
@@ -47,3 +47,5 @@ NSAN_FLAG(bool, enable_loadtracking_stats, false,
"memory, the number of times nsan resumed from the original value "
"due to invalid or unknown types.")
NSAN_FLAG(bool, print_stats_on_exit, false, "If true, print stats on exit.")
+NSAN_FLAG(bool, check_nan, false,
+ "If true, check the floating-point number is nan")
\ No newline at end of file
diff --git a/compiler-rt/test/nsan/nan.cpp b/compiler-rt/test/nsan/nan.cpp
new file mode 100644
index 0000000000000..a84b51859aaed
--- /dev/null
+++ b/compiler-rt/test/nsan/nan.cpp
@@ -0,0 +1,19 @@
+// RUN: %clangxx_nsan -O0 -g %s -o %t
+// RUN: NSAN_OPTIONS=check_nan=true %run %t 2>&1 | FileCheck %s
+
+#include <cmath>
+#include <cstdio>
+
+// This function returns a NaN value for triggering the NaN detection.
+__attribute__((noinline)) float ReturnNaN() {
+ float ret = 0.0 / 0.0;
+ return ret;
+ // CHECK: WARNING: NumericalStabilitySanitizer: NaN detected
+}
+
+int main() {
+ float val = ReturnNaN();
+ printf("val: %f\n", val);
+ // CHECK: WARNING: NumericalStabilitySanitizer: NaN detected
+ return 0;
+}
|
alexander-shaposhnikov
requested review from
MaskRay,
chestnykh and
alexander-shaposhnikov
|
Have not looked at the code yet. |
Sorry for taking so long to submit this. In the past few days, I wrote some tests (maybe I can add a few more) and made it so that the stack is output when NaN is detected. |
[compiler-rt][nsan] Add additional tests for nsan and generate a stack trace for NaN reports
|
@cseslowpoke, thanks for working on this. I've added a few comments, |
vitalybuka
requested review from
alexander-shaposhnikov
and removed request for
vitalybuka
compiler-rt/test/nsan/vec_sqrt.cpp
Outdated
| __m256 vec = _mm256_loadu_ps(&input[i]); | ||
| __m256 result = _mm256_sqrt_ps(vec); | ||
| _mm256_storeu_ps(&output[i], result); | ||
| // when simd is used, the warning is not triggered |
There was a problem hiding this comment.
it looks like something is broken.
we'd like to detect cases like this, the report triggered by std::sqrt below is not relevant, need to figure out/understand what's broken for simd.
There was a problem hiding this comment.
Sure, I’ll check the generated LLVM IR to identify where the issue might be.
There was a problem hiding this comment.
I've simplified the test a bit:
#include <cassert>
#include <cmath>
#include <immintrin.h>
#include <iostream>
void simd_sqrt(const float *input, float *output, size_t size) {
size_t i = 0;
assert(size % 8 == 0);
for (; i < size; i += 8) {
__m256 vec = _mm256_loadu_ps(&input[i]);
__m256 result = _mm256_sqrt_ps(vec);
_mm256_storeu_ps(&output[i], result);
}
}
int main() {
float input[] = {0.0, 1.0, -2.0, 3.0, 4.0, 5.0, 6.0, 7.0};
float output[8];
simd_sqrt(input, output, 8);
for (int i = 0; i < 8; ++i) {
std::cout << output[i] << std::endl;
}
return 0;
}
clang++ -fsanitize=numerical -mavx -O3 -g ~/simd2.cpp -o ~/simd2.exe
It also works as expected.
There was a problem hiding this comment.
When using sqrt, it detects NaN, but I'm not sure if using _mm256_sqrt_ps would exhibit similar behavior.
|
@cseslowpoke works as expected. |
|
@cseslowpoke, let's try to complete this effort:
|
|
@cseslowpoke Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
This reverts commit 283dff4.
|
Hi, I believe this change (after re-applied in 5136521 ) fails our builds using gcc 11.x: could you take a look? |
|
I think it's because |
|
UPD. I think a proper fix would be to simply call isnan(value) |
|
Sorry for slow reaction. Our build succeeded after 65d6c47 . Thanks for (super quick) fix! |
Add support for nan detection
#100305