Merge some fixes for compiling with the -D_GLIBCXX_DEBUG flag.

This fixes some UB that never caused any issues before. It also adds a CI job using the debug version. Related PR: #1176
ginkgo-project · Apr 4, 2024 · 2be2d78 · 2be2d78
2 parents 9bd8c0d + 6665885
commit 2be2d78
Show file tree

Hide file tree

Showing 12 changed files with 45 additions and 32 deletions.
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
@@ -517,6 +517,21 @@ build/nocuda/openmpi/clang/omp/debug/static:
     FAST_TESTS: "ON"
     BUILD_SHARED_LIBS: "OFF"
 
+build/nocuda/openmpi/clang/omp/glibcxx-debug-release/shared:
+  extends:
+    - .build_and_test_template
+    - .default_variables
+    - .quick_test_condition
+    - .use_gko-nocuda-nompi-gnu9-llvm8
+  variables:
+    CXX_COMPILER: "clang++"
+    BUILD_OMP: "ON"
+    MPI_AS_ROOT: "ON"
+    BUILD_MPI: "ON"
+    CXX_FLAGS: "-Wpedantic -D_GLIBCXX_DEBUG=1"
+    # The tests are prohibitively slow in Debug
+    BUILD_TYPE: "Release"
+
 # nocuda with the oldest supported compiler
 build/nocuda/nompi/gcc/omp/release/static:
   extends:

diff --git a/core/reorder/mc64.cpp b/core/reorder/mc64.cpp
@@ -301,7 +301,9 @@ void shortest_augmenting_path(
                 // If col is already marked because it previously was in q_j
                 // we have to disregard it.
                 queue.pop_min();
-                col = queue.min_node();
+                if (!queue.empty()) {
+                    col = queue.min_node();
+                }
             }
             if (queue.empty()) {
                 break;

diff --git a/core/test/log/profiler_hook.cpp b/core/test/log/profiler_hook.cpp
@@ -110,8 +110,8 @@ TEST(ProfilerHook, LogsPolymorphicObjectLinOp)
 {
     std::vector<std::string> expected{"begin:copy(obj,obj)",
                                       "end:copy(obj,obj)",
-                                      "begin:move(obj,obj)",
-                                      "end:move(obj,obj)",
+                                      "begin:move(obj_copy,obj)",
+                                      "end:move(obj_copy,obj)",
                                       "begin:apply(obj)",
                                       "begin:op",
                                       "end:op",
@@ -132,14 +132,18 @@ TEST(ProfilerHook, LogsPolymorphicObjectLinOp)
     auto logger = gko::log::ProfilerHook::create_custom(
         std::move(hooks.first), std::move(hooks.second));
     auto linop = gko::share(DummyLinOp::create(exec));
+    auto linop_copy = linop->clone();
     auto factory = DummyLinOp::build().on(exec);
     auto scalar = DummyLinOp::create(exec, gko::dim<2>{1, 1});
     logger->set_object_name(linop, "obj");
+    logger->set_object_name(linop_copy, "obj_copy");
     logger->set_object_name(factory, "obj_factory");
     exec->add_logger(logger);
 
     linop->copy_from(linop);
-    linop->move_from(linop);
+    // self move-assignment is potentially illegal for std::vector in pre-C++23,
+    // this would causes the libstdc++ debug mode to abort, so use the copy
+    linop->move_from(linop_copy);
     linop->apply(linop, linop);
     linop->apply(scalar, linop, scalar, linop);
     factory->generate(linop);

diff --git a/core/test/utils/matrix_generator.hpp b/core/test/utils/matrix_generator.hpp
@@ -107,7 +107,7 @@ matrix_data<ValueType, IndexType> generate_random_matrix_data(
             size_type(0),
             std::min(static_cast<size_type>(nonzero_dist(engine)), num_cols));
         std::uniform_int_distribution<IndexType> col_dist{
-            0, static_cast<IndexType>(num_cols) - 1};
+            0, std::max(static_cast<IndexType>(num_cols) - 1, IndexType{})};
         if (nnz_in_row > num_cols / 2) {
             present_cols.assign(num_cols, true);
             // remove num_cols - nnz_in_row entries from present_cols
@@ -324,7 +324,9 @@ matrix_data<ValueType, IndexType> generate_random_triangular_matrix_data(
         // randomly generate number of nonzeros in this row
         const auto min_col = lower_triangular ? 0 : row;
         const auto max_col =
-            lower_triangular ? row : static_cast<IndexType>(size) - 1;
+            lower_triangular
+                ? row
+                : std::max(static_cast<IndexType>(size) - 1, IndexType{});
         const auto max_row_nnz = max_col - min_col + 1;
         const auto nnz_in_row = std::max(
             size_type(0), std::min(static_cast<size_type>(nonzero_dist(engine)),

diff --git a/omp/base/index_set_kernels.cpp b/omp/base/index_set_kernels.cpp
@@ -135,9 +135,8 @@ void global_to_local(std::shared_ptr<const DefaultExecutor> exec,
             continue;
         }
         const auto bucket = std::distance(
-            subset_begin + 1,
-            std::upper_bound(subset_begin + 1, subset_begin + num_subsets + 1,
-                             index));
+            subset_end,
+            std::upper_bound(subset_end, subset_end + num_subsets, index));
         if (index >= subset_end[bucket] || index < subset_begin[bucket]) {
             local_indices[i] = invalid_index<IndexType>();
         } else {

diff --git a/omp/matrix/csr_kernels.cpp b/omp/matrix/csr_kernels.cpp
@@ -601,8 +601,8 @@ void convert_to_fbcsr(std::shared_ptr<const DefaultExecutor> exec,
     std::sort(entries, entries + nnz,
               [&](entry a, entry b) { return to_block(a) < to_block(b); });
     // set row pointers by jumps in block row index
-    gko::vector<IndexType> col_idx_vec{{exec}};
-    gko::vector<ValueType> value_vec{{exec}};
+    gko::vector<IndexType> col_idx_vec{exec};
+    gko::vector<ValueType> value_vec{exec};
     int64 block_row = -1;
     int64 block_col = -1;
     for (size_type i = 0; i < nnz; i++) {

diff --git a/omp/matrix/fbcsr_kernels.cpp b/omp/matrix/fbcsr_kernels.cpp
@@ -145,8 +145,8 @@ void fill_in_matrix_data(std::shared_ptr<const DefaultExecutor> exec,
                    std::make_tuple(b.row / block_size, b.column / block_size);
         });
     auto row_ptrs_ptr = row_ptrs.get_data();
-    gko::vector<IndexType> col_idx_vec{{exec}};
-    gko::vector<ValueType> value_vec{{exec}};
+    gko::vector<IndexType> col_idx_vec{exec};
+    gko::vector<ValueType> value_vec{exec};
     int64 block_row = -1;
     int64 block_col = -1;
     for (size_type i = 0; i < in_nnz; i++) {

diff --git a/omp/reorder/rcm_kernels.cpp b/omp/reorder/rcm_kernels.cpp
@@ -674,7 +674,7 @@ void write_permutation(std::shared_ptr<const OmpExecutor> exec,
 
                 // Sort neighbours. Can not be more than there are nodes.
                 const IndexType size = valid_neighbours.size();
-                sort_small(&valid_neighbours[0], size,
+                sort_small(valid_neighbours.data(), size,
                            [&](IndexType l, IndexType r) {
                                return degrees[l] < degrees[r];
                            });

diff --git a/omp/test/base/index_set.cpp b/omp/test/base/index_set.cpp
@@ -31,8 +31,7 @@ class index_set : public ::testing::Test {
 
     gko::array<index_type> setup_random_indices(int num_indices = 100)
     {
-        std::random_device rd;
-        std::mt19937 gen(rd());
+        std::mt19937 gen(15);
         std::uniform_int_distribution<index_type> dist(0, num_indices);
         std::vector<index_type> index_vec(num_indices);
         for (auto& i : index_vec) {

diff --git a/reference/matrix/csr_kernels.cpp b/reference/matrix/csr_kernels.cpp
@@ -481,8 +481,8 @@ void convert_to_fbcsr(std::shared_ptr<const DefaultExecutor> exec,
     std::sort(entries, entries + nnz,
               [&](entry a, entry b) { return to_block(a) < to_block(b); });
     // set row pointers by jumps in block row index
-    gko::vector<IndexType> col_idx_vec{{exec}};
-    gko::vector<ValueType> value_vec{{exec}};
+    gko::vector<IndexType> col_idx_vec{exec};
+    gko::vector<ValueType> value_vec{exec};
     int64 block_row = -1;
     int64 block_col = -1;
     for (size_type i = 0; i < nnz; i++) {

diff --git a/reference/matrix/fbcsr_kernels.cpp b/reference/matrix/fbcsr_kernels.cpp
@@ -144,8 +144,8 @@ void fill_in_matrix_data(std::shared_ptr<const DefaultExecutor> exec,
                    std::make_tuple(b.row / block_size, b.column / block_size);
         });
     auto row_ptrs_ptr = row_ptrs.get_data();
-    gko::vector<IndexType> col_idx_vec{{exec}};
-    gko::vector<ValueType> value_vec{{exec}};
+    gko::vector<IndexType> col_idx_vec{exec};
+    gko::vector<ValueType> value_vec{exec};
     int64 block_row = -1;
     int64 block_col = -1;
     for (size_type i = 0; i < in_nnz; i++) {

diff --git a/test/factorization/par_ilu_kernels.cpp b/test/factorization/par_ilu_kernels.cpp
@@ -22,6 +22,7 @@
 #include <ginkgo/core/matrix/dense.hpp>
 
 
+#include "core/base/iterator_factory.hpp"
 #include "core/factorization/factorization_kernels.hpp"
 #include "core/test/utils.hpp"
 #include "matrices/config.hpp"
@@ -74,20 +75,11 @@ class ParIlu : public CommonTestFixture {
         auto values = mtx->get_values();
         auto col_idxs = mtx->get_col_idxs();
         const auto row_ptrs = mtx->get_const_row_ptrs();
-        for (int row = 0; row < num_rows; ++row) {
+        for (index_type row = 0; row < num_rows; ++row) {
             const auto row_start = row_ptrs[row];
             const auto row_end = row_ptrs[row + 1];
-            const int num_row_elements = row_end - row_start;
-            auto idx_dist = std::uniform_int_distribution<index_type>(
-                row_start, row_end - 1);
-            for (int i = 0; i < num_row_elements / 2; ++i) {
-                auto idx1 = idx_dist(rand_engine);
-                auto idx2 = idx_dist(rand_engine);
-                if (idx1 != idx2) {
-                    swap(values[idx1], values[idx2]);
-                    swap(col_idxs[idx1], col_idxs[idx2]);
-                }
-            }
+            const auto it = gko::detail::make_zip_iterator(col_idxs, values);
+            std::shuffle(it + row_start, it + row_end, rand_engine);
         }
         return mtx;
     }