Assert on out-of-range accesses #37

davidbeckingsale · 2018-06-12T22:52:43Z

No description provided.

rrsettgast · 2018-06-12T23:25:44Z

src/ManagedArray.inl

@@ -46,6 +46,8 @@
 #include "ManagedArray.hpp"
 #include "ArrayManager.hpp"

+#include <assert.h>


should this be
#include <cassert>

It can be <cassert> or assert.h . I think <assert.h> might valid too? What do you think @davidbeckingsale ?

If they both work I'm okay with cassert.

@artv3 @davidbeckingsale @rrsettgast as a style and consistency point, I think it's best to use C++ header files; i.e., cassert instead of assert.h. In particular the C++ headers (without .h) are guaranteed to be standard compliant and this may not be the case for the *.h forms.

Agree. My only concern was whether the assert.h you get when using nvcc was a different file which supplies the GPU assert definition. I think @artv3 has verified that both work however.

cassert works for nvcc.

rrsettgast · 2018-06-12T23:27:12Z

src/ManagedArray.inl

@@ -117,7 +119,7 @@ CHAI_HOST_DEVICE ManagedArray<T>::ManagedArray(ManagedArray const& other):
 */
 if (!std::is_const<T>::value) {
 CHAI_LOG("ManagedArray", "T is non-const, registering touch of pointer" << m_active_pointer);
- T_non_const* non_const_pointer = const_cast<T_non_const*>(m_active_pointer);
+// T_non_const* non_const_pointer = const_cast<T_non_const*>(m_active_pointer);


@davidbeckingsale this was a compilation warning for unused variable. is it needed for something?

rrsettgast · 2018-06-12T23:28:08Z

src/tests/managed_array_tests.cpp

@@ -410,3 +409,12 @@ CUDA_TEST(ManagedArray, Move)
 array.free();
 }
 #endif // defined(CHAI_ENABLE_CUDA)
+


@artv3 I think that the intention was to use @davidbeckingsale tests. Can you revert this file to his commit if that is a better idea?

Should I remove yours? Or keep them as well?

I think David's tests cover the space better...right? well at any rate, there needs to be a cuda test.

davidbeckingsale · 2018-06-14T21:58:10Z

cmake/ChaiBasics.cmake

@@ -43,3 +43,5 @@

 set (CMAKE_CUDA_FLAGS "${CMAKE_CUDA_FLAGS} --expt-extended-lambda")
 include(${CMAKE_SOURCE_DIR}/cmake/thirdparty/SetupChaiThirdparty.cmake)
+
+include("${CMAKE_CURRENT_LIST_DIR}/ChaiOptions.cmake")


I don't think we need this extra file.

davidbeckingsale · 2018-06-14T21:58:28Z

cmake/ChaiOptions.cmake

@@ -0,0 +1,2 @@
+
+option( CHAI_ARRAY_BOUNDS_CHECK "Enables Bounds Checking for chai::ManagedArray<>::operator[]" OFF )


This option can go in the main CMakeLists, and should be of the form ENABLE_*

davidbeckingsale · 2018-06-14T21:58:58Z

src/config.hpp.in

@@ -47,5 +47,6 @@
 #cmakedefine CHAI_ENABLE_IMPLICIT_CONVERSIONS
 #cmakedefine CHAI_DISABLE_RM
 #cmakedefine CHAI_ENABLE_UM
+#cmakedefine CHAI_ARRAY_BOUNDS_CHECK


This should be CHAI_ENABLE_*, matching the option from the CMakeLists, prefixed with CHAI_

davidbeckingsale · 2018-06-14T21:59:27Z

src/tests/CMakeLists.txt

@@ -55,10 +55,24 @@ blt_add_executable(
 SOURCES managed_array_tests.cpp
 DEPENDS_ON ${managed_array_test_depends})

+blt_add_executable(
+ NAME managed_array_bounds_check_tests


I'm not clear on the benefit of the extra file? These tests can just be guarded in the main file.

But then the tests are only run if the option was specified to cmake...which isn't a big deal, but it also means that to enable the tests in travis, you would need to add an extra ci test. This way the bounds checking is always run for every ci test case.

But they should only be run if the CMake option is on, otherwise the bounds are not checked...

… upper and lower bounds tests.

davidbeckingsale · 2018-06-15T13:05:33Z

I think this is looking good, but Im not sure why the nvcc builds are failing...

Does this build locally for you?

rrsettgast · 2018-06-15T16:34:30Z

@davidbeckingsale It builds fine when I tried it on the rajaorg/compiler:nvcc8 docker image. This image uses gcc5.

davidbeckingsale · 2018-07-13T22:24:52Z

CMakeLists.txt

@@ -71,6 +71,9 @@ endif()
 set(ENABLE_COPY_HEADERS On CACHE BOOL "")
 set(BLT_CXX_STD c++11 CACHE STRING "")

+if( CMAKE_BUILD_TYPE STREQUAL "Debug" )
+ set( CHAI_ENABLE_BOUNDS_CHECK ON CACHE BOOL "Enable Bounds Checking for chai::ManagedArray<>::operator[]" FORCE )


This should be an option at the top of the CMakeLists file.

adayton1 · 2019-01-16T17:33:33Z

Is this working now? Using assert on the device kills the kernel, but I believe you have to synchronize and check the error code on the host to actually see that there was a GPU assert.

davidbeckingsale · 2019-01-17T00:09:50Z

@adayton1 - give it a whirl and let me know 😄

artv3 · 2019-09-16T19:56:26Z

@adayton1 , cmake will sometimes add a NDEBUG define which will cause asserts to no work. Will you be covering this case too? We see it in RAJA with the release build.

artv3 · 2019-09-23T14:38:01Z

src/chai/ManagedArray.inl

+#if defined(NDEBUG)
+#undef NDEBUG // This makes sure assert is enabled even for release builds
+#endif // defined(NDEBUG)
+


Could this have unintended consequences?

davidbeckingsale and others added 7 commits May 24, 2018 10:15

Add assert for out of range

e1f32c4

added compile time option to perform range checking on ManagedArray

a7bde92

Merge branch 'develop' into feature/settgast/addRangeChecking

827c11a

undo previous change to implicit conversion test

6a7b929

Updates for test

a989c60

added cassert header

5bbb458

merged Randys branch

e81eb80

rrsettgast reviewed Jun 12, 2018

View reviewed changes

artv3 and others added 2 commits June 13, 2018 09:04

reverted managed_array_test.cpp and added <cassert>

908df3f

Merge branch 'develop' into feature/CHAI-44-debug-range-check

a0a6664

davidbeckingsale commented Jun 14, 2018

View reviewed changes

rrsettgast force-pushed the feature/CHAI-44-debug-range-check branch from 7d63c8d to a0a6664 Compare June 14, 2018 23:08

removed unnecessary cmake option file, fixed bug in forall.hpp, added…

f38337d

… upper and lower bounds tests.

rrsettgast force-pushed the feature/CHAI-44-debug-range-check branch from 46a1fa5 to f38337d Compare June 15, 2018 00:10

davidbeckingsale commented Jul 13, 2018

View reviewed changes

adayton1 added 4 commits September 16, 2019 12:13

Resolve conflicts after merging in develop

a7af717

Fix bad merge

6c55bf9

Add cmake option for bounds checking

dfa4b55

Remove unnecessary variable

deba461

adayton1 added 4 commits September 16, 2019 14:42

Another fix after bad merge

183358a

Remove old tests in incorrect location

4eb6636

Make sure asserts are turned on, even for release builds

495e9b4

Use ASSERT instead of EXPECT to match the rest of the tests

56044d4

Fix cuda build

a2526d8

artv3 reviewed Sep 23, 2019

View reviewed changes

adayton1 added 2 commits September 25, 2019 15:20

Add always assert function

6f9e038

Resolved conflicts

87375a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assert on out-of-range accesses #37

Assert on out-of-range accesses #37

davidbeckingsale commented Jun 12, 2018

rrsettgast Jun 12, 2018 •

edited

Loading

artv3 Jun 12, 2018

davidbeckingsale Jun 13, 2018

rhornung67 Jun 13, 2018 •

edited

Loading

davidbeckingsale Jun 13, 2018

adayton1 Jan 16, 2019

rrsettgast Jun 12, 2018

rrsettgast Jun 12, 2018 •

edited

Loading

artv3 Jun 12, 2018

rrsettgast Jun 12, 2018 •

edited

Loading

davidbeckingsale Jun 14, 2018

davidbeckingsale Jun 14, 2018

davidbeckingsale Jun 14, 2018

davidbeckingsale Jun 14, 2018

rrsettgast Jun 14, 2018

davidbeckingsale Jun 14, 2018

davidbeckingsale commented Jun 15, 2018

rrsettgast commented Jun 15, 2018

davidbeckingsale Jul 13, 2018

adayton1 Sep 16, 2019

adayton1 commented Jan 16, 2019

davidbeckingsale commented Jan 17, 2019

artv3 commented Sep 16, 2019

artv3 Sep 23, 2019

		@@ -0,0 +1,2 @@

		option( CHAI_ARRAY_BOUNDS_CHECK "Enables Bounds Checking for chai::ManagedArray<>::operator[]" OFF )

Assert on out-of-range accesses #37

Are you sure you want to change the base?

Assert on out-of-range accesses #37

Conversation

davidbeckingsale commented Jun 12, 2018

rrsettgast Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhornung67 Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrsettgast Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrsettgast Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbeckingsale commented Jun 15, 2018

rrsettgast commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adayton1 commented Jan 16, 2019

davidbeckingsale commented Jan 17, 2019

artv3 commented Sep 16, 2019

Choose a reason for hiding this comment

rrsettgast Jun 12, 2018 •

edited

Loading

rhornung67 Jun 13, 2018 •

edited

Loading

rrsettgast Jun 12, 2018 •

edited

Loading

rrsettgast Jun 12, 2018 •

edited

Loading