Fix Clang ASAN issue by handling float to integer overflow in convert operator #3071
Conversation
|
Explanation can be found in the link: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3071 +/- ##
========================================
Coverage 91.92% 91.92%
========================================
Files 489 489
Lines 19275 19278 +3
========================================
+ Hits 17719 17722 +3
Misses 1556 1556 ☔ View full report in Codecov by Sentry. |
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
Jenkinsfile
Outdated
| @@ -165,7 +165,8 @@ rocmtest clang_debug: rocmnode('mi100+') { cmake_build -> | |||
| }, clang_asan: rocmnode('nogpu') { cmake_build -> | |||
| stage('Clang ASAN') { | |||
| def sanitizers = "undefined,address" | |||
| def debug_flags = "-g -O2 -fno-omit-frame-pointer -fsanitize=${sanitizers} -fno-sanitize-recover=${sanitizers}" | |||
| def sanitizers_disabled = "float-cast-overflow" | |||
There was a problem hiding this comment.
Doing it this way means we lose asan coverage of float-cast-overflow on the entire code base. Is there a way to target just the function/file?
There was a problem hiding this comment.
agree with chris.
@nives-vukovic can you try disabling sanitizer just on the convert op's compute function and see if that works ?
https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address
There was a problem hiding this comment.
The issue is reported in AMDMIGraphX/src/include/migraphx/shape.hpp, and when I add __attribute__((no_sanitize("float-cast-overflow"))) above:
type operator()(U u) const
{
return type(u);
}
in 'as' struct, the issue is not reported on our system. However, this is a common function used in many places so I don't know if this would be a satisfactory solution for you.
There was a problem hiding this comment.
I dont not think it is a good idea to disable sanitizer on type operator()(U u) either.
We can conditionally handle floating point to integer conversion inside the convert operator itself.
shape::visit(type, [&](auto as) {
// clamping value between target_type's max and min doesn't work for NaNs,
if(std::isnan(static_cast<double>(x)))
{
y = as.nan();
}
---------------------> Here
// if "type" is integer and "x" has floating point then, first do the clamping and then do the conversion.
------------------------
else
{
// clamp overflowing/underflowing values to min()/max() instead of +/-infinity
// during downcasting
y = std::min(std::max(as(x), as.min()), as.max());
}
});
There was a problem hiding this comment.
Doing this works inside the convert.hpp's apply() function.
else if(shape::is_integral(type) and std::is_floating_point_v<decltype(x)>)
{
// for the floating point to integer conversion, clamp first and then convert to
// avoid undefined behaviour
y = as(std::min(std::max(static_cast<double>(x), static_cast<double>(as.min())),
static_cast<double>(as.max())));
}
There was a problem hiding this comment.
It was compiled as : g++ -fsanitize=address
There was a problem hiding this comment.
It was compiled as :
g++ -fsanitize=address
Try adding -fsanitize=undefined as well
There was a problem hiding this comment.
(Yes that option does highlight the issue. Thanks.)
One answer is: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-undefined
There was a problem hiding this comment.
(Yes that option does highlight the issue. Thanks.)
One answer is: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-undefined
It has been tried but that would disable sanitizer on entire type cast function in migraphx. it is not desired. Therefore need to handle it inside the convert.hpp itself
There was a problem hiding this comment.
Okay. Thanks.
Maybe try, on a test_case basis, passing on an environment flag/variable:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#runtime-suppressions
nives-vukovic
changed the title
lakhinderwalia
changed the title
No description provided.