Merge pull request #1330 from Idclip/sanitizer_fixes

Updated UBSAN builds to fail on errors

Author: Idclip

.github/workflows/build.yml

@@ -153,7 +153,7 @@ jobs:
       # Always run tests on weekly builds but skip Debug on commits as they take a while.
       # https://github.community/t/distinct-job-for-each-schedule/17811/2
       if: contains(github.event.schedule, '0 7 * * 1') || matrix.config.build == 'Release'
-      run: cd build && ctest -V
+      run: cd build && ctest -V -C ${{ matrix.config.build }}
 
   testmacos11:
     if: |

.github/workflows/weekly.yml

@@ -155,8 +155,8 @@ jobs:
     strategy:
       matrix:
         config:
-          - { build: 'asan',  cmake: '-DUSE_BLOSC=OFF' } # We never called blosc_destroy(), so disable blosc to silence these errors
-          - { build: 'ubsan', cmake: '' } # Currently doesn't error, just reports all issues
+          - { build: 'asan',  components: 'core,test,nano,nanotest,axcore,axtest', cmake: '-DUSE_BLOSC=OFF' } # We never called blosc_destroy(), so disable blosc to silence these errors
+          - { build: 'ubsan', components: 'core,test,axcore,axtest', cmake: '' } # Doesn't currently test NanoVDB
           #- { build: 'lsan', cmake: '' } # asan encompasses and includes lsan
           #- { build: 'tsan', cmake: '' } # requires full stack rebuild for valid results (including libc++)
           #- { build: 'msan', cmake: '' } # requires full stack rebuild for valid results (including libc++)
@@ -168,7 +168,7 @@ jobs:
       run: >
         ./ci/build.sh -v
         --build-type=${{ matrix.config.build }}
-        --components="core,test,nano,nanotest,axcore,axtest"
+        --components="${{ matrix.config.components }}"
         --cargs=\"
         -DOPENVDB_CORE_STATIC=OFF
         -DOPENVDB_AX_STATIC=OFF

cmake/scripts/ubsan.supp

@@ -0,0 +1,30 @@
+#################################################################################
+## This file is loaded by the Undefined Behaviour Sanitizer build for the unit ##
+## tests. It can be used to ignore various errors reported by the sanitizer.   ##
+## This is especially useful with upstream issues (e.g. boost/tbb). For help   ##
+## defining suppression rules, see:                                            ##
+##   https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html               ##
+## The build is configured with CMAKE_BUILD_TYPE=ubsan.                        ##
+#################################################################################
+
+##### Upstream #####
+
+# Lots of warnings from TBB, ignore them
+alignment:tbb/concurrent_hash_map.h
+vptr:tbb/parallel_reduce.h
+vptr:tbb/task.h
+
+##### OpenVDB #####
+
+# The sOn/sOff static bool data held on the LeaffBuff<bool> objects can be
+# initialised to arbitrary true/false values. See the note in LeafBuffer.h
+enum:openvdb/tools/Activate.h
+# Some 2s complement tests, ignore the negative shifts
+# @todo Should address these
+shift-base:TestMultiResGrid.cc
+shift-base:openvdb/math/Coord.h
+
+##### OpenVDB AX #####
+
+# Ignore overflows reported from old OpenSimplexNoise
+signed-integer-overflow:openvdb_ax/math/OpenSimplexNoise.cc

openvdb/openvdb/math/Coord.h

@@ -229,7 +229,8 @@ class Coord
     template<int Log2N = 20>
     size_t hash() const
     {
-        return ((1<<Log2N)-1) & (mVec[0]*73856093 ^ mVec[1]*19349663 ^ mVec[2]*83492791);
+        const uint32_t* vec = reinterpret_cast<const uint32_t*>(mVec.data());
+        return ((1<<Log2N)-1) & (vec[0]*73856093 ^ vec[1]*19349663 ^ vec[2]*83492791);
     }
 
 private:

openvdb/openvdb/math/Math.h

@@ -872,7 +872,7 @@ template<typename Type>
 inline Type
 Truncate(Type x, unsigned int digits)
 {
-    Type tenth = Pow(10,digits);
+    Type tenth = static_cast<Type>(Pow(size_t(10), digits));
     return RoundDown(x*tenth+0.5)/tenth;
 }
 

openvdb/openvdb/tree/LeafBuffer.h

@@ -446,6 +446,8 @@ class LeafBuffer<bool, Log2Dim>
 /// LeafNode::getValue() return a reference to a value.  Since it's not possible
 /// to return a reference to a bit in a node mask, we return a reference to one
 /// of the following static values instead.
+///
+/// @todo  Make these static inline with C++17
 template<Index Log2Dim> const bool LeafBuffer<bool, Log2Dim>::sOn = true;
 template<Index Log2Dim> const bool LeafBuffer<bool, Log2Dim>::sOff = false;
 

openvdb/openvdb/unittest/CMakeLists.txt

@@ -208,8 +208,18 @@ if(USE_BLOSC OR OpenVDB_USES_BLOSC OR USE_ZLIB OR OpenVDB_USES_ZLIB)
   list(APPEND OPENVDB_CORE_DEPENDENT_LIBS ZLIB::ZLIB)
 endif()
 
-target_link_libraries(vdb_test
-  ${OPENVDB_TEST_DEPENDENT_LIBS}
-)
+target_link_libraries(vdb_test ${OPENVDB_TEST_DEPENDENT_LIBS})
+add_test(NAME vdb_unit_test COMMAND $<TARGET_FILE:vdb_test> -v)
+
+# For the undefined behaviour sanitizer, add the suppression file and
+# additional options
+
+get_filename_component(PATH_TO_PROJECT_ROOT ${CMAKE_CURRENT_LIST_DIR} DIRECTORY)
+get_filename_component(PATH_TO_PROJECT_ROOT ${PATH_TO_PROJECT_ROOT} DIRECTORY)
+get_filename_component(PATH_TO_PROJECT_ROOT ${PATH_TO_PROJECT_ROOT} DIRECTORY)
+set(UBSAN_SUPRESSION_FILE ${PATH_TO_PROJECT_ROOT}/cmake/scripts/ubsan.supp)
+
+set_tests_properties(vdb_unit_test PROPERTIES
+    ENVIRONMENT
+      "$<$<CONFIG:UBSAN>:UBSAN_OPTIONS=halt_on_error=1 report_error_type=1 suppressions=${UBSAN_SUPRESSION_FILE}>")
 
-add_test(vdb_unit_test vdb_test -v)

openvdb/openvdb/unittest/TestCoord.cc

@@ -315,8 +315,8 @@ TEST_F(TestCoord, testCoordBBox)
         EXPECT_EQ(count, n);
     }
 
-    {// bit-wise operations
-        const openvdb::Coord min(-1,-2,3), max(2,3,5);
+    {// bit-wise operations (note that the API doesn't define behaviour for shifting neg coords)
+        const openvdb::Coord min(1,2,3), max(2,3,5);
         const openvdb::CoordBBox b(min, max);
         EXPECT_EQ(openvdb::CoordBBox(min>>1,max>>1), b>>size_t(1));
         EXPECT_EQ(openvdb::CoordBBox(min>>3,max>>3), b>>size_t(3));

openvdb/openvdb/unittest/TestIndexIterator.cc

@@ -321,7 +321,7 @@ TEST_F(TestIndexIterator, testProfile)
 
     { // for loop
         ProfileTimer timer("ForLoop: sum");
-        volatile int sum = 0;
+        volatile uint64_t sum = 0;
         for (int i = 0; i < elements; i++) {
             sum += i;
         }
@@ -330,7 +330,7 @@ TEST_F(TestIndexIterator, testProfile)
 
     { // index iterator
         ProfileTimer timer("IndexIter: sum");
-        volatile int sum = 0;
+        volatile uint64_t sum = 0;
         ValueVoxelCIter iter(0, elements);
         for (; iter; ++iter) {
             sum += *iter;
@@ -350,7 +350,7 @@ TEST_F(TestIndexIterator, testProfile)
 
     { // manual value iteration
         ProfileTimer timer("ValueIteratorManual: sum");
-        volatile int sum = 0;
+        volatile uint64_t sum = 0;
         auto indexIter(leafNode.cbeginValueOn());
         int offset = 0;
         for (; indexIter; ++indexIter) {
@@ -366,7 +366,7 @@ TEST_F(TestIndexIterator, testProfile)
 
     { // value on iterator (all on)
         ProfileTimer timer("ValueIndexIter: sum");
-        volatile int sum = 0;
+        volatile uint64_t sum = 0;
         auto indexIter(leafNode.cbeginValueAll());
         IndexIter<LeafNode::ValueAllCIter, NullFilter>::ValueIndexIter iter(indexIter);
         for (; iter; ++iter) {

openvdb/openvdb/unittest/TestPointConversion.cc

@@ -138,7 +138,7 @@ genPoints(const int numPoints, const double scale, const bool stride,
     AttributeWrapper<float>::Handle uniformHandle(uniform);
     AttributeWrapper<openvdb::Name>::Handle stringHandle(string);
 
-    int i = 0;
+    size_t i = 0;
 
     // loop over a [0 to n) x [0 to n) grid.
     for (int a = 0; a < n; ++a) {
@@ -164,14 +164,14 @@ genPoints(const int numPoints, const double scale, const bool stride,
 
             if (stride)
             {
-                xyzHandle.set(i, 0, i);
-                xyzHandle.set(i, 1, i*i);
-                xyzHandle.set(i, 2, i*i*i);
+                xyzHandle.set(i, 0, static_cast<int>(i));
+                xyzHandle.set(i, 1, static_cast<int>(i*i));
+                xyzHandle.set(i, 2, static_cast<int>(i*i*i));
             }
 
             // add points with even id to the group
             if ((i % 2) == 0) {
-                group.setOffsetOn(i);
+                group.setOffsetOn(static_cast<int>(i));
                 stringHandle.set(i, /*stride*/0, "testA");
             }
             else {