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.

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

Fix Clang ASAN issue by handling float to integer overflow in convert operator #3071

Merged
merged 4 commits into from
Jun 10, 2024
Merged

Fix Clang ASAN issue by handling float to integer overflow in convert operator #3071

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29c51b6
Fix Clang ASAN issue by disabling float-cast-overflow check in sanitizer
nives-vukovic May 10, 2024
0e6e32c
Merge remote-tracking branch 'origin/develop' into clang_asan_uint8_fix
nives-vukovic May 15, 2024
a7e03ba
Add conidition in convert apply function to handle float to integer c…
nives-vukovic May 15, 2024
4572474
Merge branch 'develop' into clang_asan_uint8_fix
causten May 30, 2024
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
3 changes: 2 additions & 1 deletion Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@nives-vukovic nives-vukovic May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@lakhinderwalia lakhinderwalia May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It was compiled as : g++ -fsanitize=address

Copy link
Member

Choose a reason for hiding this comment

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

It was compiled as : g++ -fsanitize=address

Try adding -fsanitize=undefined as well

Copy link
Contributor

Choose a reason for hiding this comment

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

(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

Copy link
Member

Choose a reason for hiding this comment

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

(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

Copy link
Contributor

@lakhinderwalia lakhinderwalia May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay. Thanks.
Maybe try, on a test_case basis, passing on an environment flag/variable:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#runtime-suppressions

def debug_flags = "-g -O2 -fno-omit-frame-pointer -fsanitize=${sanitizers} -fno-sanitize-recover=${sanitizers} -fno-sanitize=${sanitizers_disabled}"
def gpu_targets = getgputargets()
cmake_build(flags: "-DCMAKE_BUILD_TYPE=debug -DMIGRAPHX_ENABLE_PYTHON=Off -DMIGRAPHX_ENABLE_GPU=Off -DMIGRAPHX_ENABLE_CPU=On -DCMAKE_CXX_FLAGS_DEBUG='${debug_flags}' -DCMAKE_C_FLAGS_DEBUG='${debug_flags}' -DGPU_TARGETS='${gpu_targets}'")
}
Expand Down
3 changes: 1 addition & 2 deletions test/verify/test_max_pooling_ceil_3d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,5 +43,4 @@ struct test_max_pooling_ceil_3d : verify_program<test_max_pooling_ceil_3d<T>>
};

template struct test_max_pooling_ceil_3d<migraphx::shape::float_type>;
// TODO: uncomment once "Clang ASAN" CI is fixed. See PR 2973 for details
// template struct test_max_pooling_ceil_3d<migraphx::shape::uint8_type>;
template struct test_max_pooling_ceil_3d<migraphx::shape::uint8_type>;
Loading