From 9ba170a01860962866a43ec7b9b80eb21e082a70 Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Thu, 24 Oct 2024 07:14:15 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=B9=20Fix=20GCC=20complaints=20(#2400)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Tree Fixes --- likely a spurious --- warning in `tree.cpp`: ``` [2/6] Building CXX object arbor/CMakeFiles/arbor.dir/tree.cpp.o In file included from /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/algorithm:60, from /Users/hater/src/arbor/arbor/tree.cpp:1: In function 'typename __gnu_cxx::__enable_if::__value, void>::__type std::__fill_a1(_ForwardIterator, _ForwardIterator, const _Tp&) [with _ForwardIterator = unsigned int*; _Tp = unsigned int]', inlined from 'void std::__fill_a(_FIte, _FIte, const _Tp&) [with _FIte = unsigned int*; _Tp = unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:998:21, inlined from '_OutputIterator std::__fill_n_a(_OutputIterator, _Size, const _Tp&, random_access_iterator_tag) [with _OutputIterator = unsigned int*; _Size = long unsigned int; _Tp = unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:1151:20, inlined from '_OI std::fill_n(_OI, _Size, const _Tp&) [with _OI = unsigned int*; _Size = long unsigned int; _Tp = unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:1180:29, inlined from 'static _ForwardIterator std::__uninitialized_default_n_1::__uninit_default_n(_ForwardIterator, _Size) [with _ForwardIterator = unsigned int*; _Size = long unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_uninitialized.h:668:29, inlined from 'static _ForwardIterator std::__uninitialized_default_n_1::__uninit_default_n(_ForwardIterator, _Size) [with _ForwardIterator = unsigned int*; _Size = long unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_uninitialized.h:660:9, inlined from '_ForwardIterator std::__uninitialized_default_n(_ForwardIterator, _Size) [with _ForwardIterator = unsigned int*; _Size = long unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_uninitialized.h:712:20, inlined from '_ForwardIterator std::__uninitialized_default_n_a(_ForwardIterator, _Size, allocator<_Tp>&) [with _ForwardIterator = unsigned int*; _Size = long unsigned int; _Tp = unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_uninitialized.h:779:44, inlined from 'void std::vector<_Tp, _Alloc>::_M_default_initialize(size_type) [with _Tp = unsigned int; _Alloc = std::allocator]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_vector.h:1720:36, inlined from 'std::vector<_Tp, _Alloc>::vector(size_type, const allocator_type&) [with _Tp = unsigned int; _Alloc = std::allocator]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_vector.h:558:30, inlined from 'arb::tree::iarray arb::tree::select_new_root(int_type)' at /Users/hater/src/arbor/arbor/tree.cpp:219:34: /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:952:18: warning: 'void* __builtin_memset(void*, int, long unsigned int)' offset 4 is out of the bounds [0, 0] [-Warray-bounds=] 952 | *__first = __tmp; ``` Also use `iota` to generate contiguous indices. # Tests ## `test_simd.cpp` GCC doesn't acknowledge that `simd v[i] = x` for all `i = 0 ... len(v)` initializes `v`. Force initialization to zero. ## `test_network_generation.cpp` Compare across signs ## `test_distributed_for_each.cpp` Compare across signs; resolve by casting to correct type.    ## `test_range.cpp` GCC really turns the dial up to 11 here, complaining about oob access in ``` char str[] = "howdy"; auto rg = range_n(str, strlen(str)); // NOTE: Putting 5 here is OK, so is sizeof(str) - 1 util::sort(rg); ``` Same happens with `std::sort(str, str + strlen(str))`, so I am pretty sure this is not a real thing. For reference: ``` /Users/hater/src/arbor/test/unit/test_range.cpp: In member function 'virtual void range_sort_Test::TestBody()': /Users/hater/src/arbor/test/unit/test_range.cpp:446:14: note: at offset [8, 4611686018427387903] into object 'cstr' of size 6 446 | char cstr[] = "howdy"; | ^~~~ In function 'std::_Require >, std::is_move_constructible<_Tp>, std::is_move_assignable<_Tp> > std::swap(_Tp&, _Tp&) [with _Tp = char]', inlined from 'void std::iter_swap(_ForwardIterator1, _ForwardIterator2) [with _ForwardIterator1 = char*; _ForwardIterator2 = char*]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:185:11, inlined from 'void std::__move_median_to_first(_Iterator, _Iterator, _Iterator, _Iterator, _Compare) [with _Iterator = char*; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:91:20, inlined from '_RandomAccessIterator std::__unguarded_partition_pivot(_RandomAccessIterator, _RandomAccessIterator, _Compare) [with _RandomAccessIterator = char*; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:1855:34, inlined from 'void std::__introsort_loop(_RandomAccessIterator, _RandomAccessIterator, _Size, _Compare) [with _RandomAccessIterator = char*; _Size = long int; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:1889:38, inlined from 'void std::__sort(_RandomAccessIterator, _RandomAccessIterator, _Compare) [with _RandomAccessIterator = char*; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:1905:25, inlined from 'void std::__sort(_RandomAccessIterator, _RandomAccessIterator, _Compare) [with _RandomAccessIterator = char*; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:1900:5, inlined from 'void std::sort(_RAIter, _RAIter) [with _RAIter = char*]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:4771:18, inlined from 'std::enable_if_t<((bool)(! std::is_const::reference>::value))> arb::util::sort(Seq&&) [with Seq = range]' at /Users/hater/src/arbor/arbor/util/rangeutil.hpp:154:14, inlined from 'virtual void range_sort_Test::TestBody()' at /Users/hater/src/arbor/test/unit/test_range.cpp:448:19: /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/move.h:223:11: warning: array subscript [8, 4611686018427387903] is outside array bounds of 'char [6]' [-Warray-bounds=] 223 | __b = _GLIBCXX_MOVE(__tmp); | ^ /Users/hater/src/arbor/test/unit/test_range.cpp: In member function 'virtual void range_sort_Test::TestBody()': /Users/hater/src/arbor/test/unit/test_range.cpp:446:14: note: at offset [8, 4611686018427387903] into object 'cstr' of size 6 446 | char cstr[] = "howdy"; ``` I am still going to 'fix' this, since I am for warning free code and don't want to annotate with a pragma. # General Remove spurious includes. --------- Co-authored-by: boeschf <48126478+boeschf@users.noreply.github.com> --- arbor/tree.cpp | 28 ++++---- arbor/tree.hpp | 4 -- arbor/util/range.hpp | 8 +-- .../test_distributed_for_each.cpp | 13 ++-- .../test_network_generation.cpp | 8 +-- test/unit/common.hpp | 1 + test/unit/test_range.cpp | 64 ++++++++----------- test/unit/test_simd.cpp | 4 +- 8 files changed, 55 insertions(+), 75 deletions(-) diff --git a/arbor/tree.cpp b/arbor/tree.cpp index 3ae342dff0..1407303575 100644 --- a/arbor/tree.cpp +++ b/arbor/tree.cpp @@ -29,7 +29,7 @@ tree::tree(std::vector parent_index) { parents_[0] = no_parent; // compute offsets into children_ array - arb::util::make_partition(child_index_, child_count(parents_)); + util::make_partition(child_index_, child_count(parents_)); std::vector pos(parents_.size(), 0); for (auto i = 1u; i < parents_.size(); ++i) { @@ -199,11 +199,9 @@ tree::iarray tree::select_new_root(int_type root) { } // maps new indices to old indices - iarray indices (num_nodes); - // fill array with indices - for (auto i: make_span(num_nodes)) { - indices[i] = i; - } + iarray indices(num_nodes); + std::iota(indices.begin(), indices.end(), 0); + // perform sort by depth index to get the permutation std::sort(indices.begin(), indices.end(), [&](auto i, auto j){ if (reduced_depth[i] != reduced_depth[j]) { @@ -214,16 +212,12 @@ tree::iarray tree::select_new_root(int_type root) { } return depth[i] < depth[j]; }); - // maps old indices to new indices - iarray indices_inv (num_nodes); - // fill array with indices - for (auto i: make_span(num_nodes)) { - indices_inv[i] = i; - } - // perform sort - std::sort(indices_inv.begin(), indices_inv.end(), [&](auto i, auto j){ - return indices[i] < indices[j]; - }); + + // inverse permutation + iarray indices_inv(num_nodes, 0); + std::iota(indices_inv.begin(), indices_inv.end(), 0); + std::sort(indices_inv.begin(), indices_inv.end(), + [&](auto i, auto j){ return indices[i] < indices[j]; }); // translate the parent vetor to new indices for (auto i: make_span(num_nodes)) { @@ -241,7 +235,7 @@ tree::iarray tree::select_new_root(int_type root) { // recompute the children array memory::copy(new_parents, parents_); - arb::util::make_partition(child_index_, child_count(parents_)); + util::make_partition(child_index_, child_count(parents_)); std::vector pos(parents_.size(), 0); for (auto i = 1u; i < parents_.size(); ++i) { diff --git a/arbor/tree.hpp b/arbor/tree.hpp index 137598318a..75b6fb2f8f 100644 --- a/arbor/tree.hpp +++ b/arbor/tree.hpp @@ -2,16 +2,12 @@ #include #include -#include -#include #include #include #include -#include "memory/memory.hpp" #include "util/rangeutil.hpp" -#include "util/span.hpp" namespace arb { diff --git a/arbor/util/range.hpp b/arbor/util/range.hpp index 3a7257b7c1..d01ce35856 100644 --- a/arbor/util/range.hpp +++ b/arbor/util/range.hpp @@ -158,10 +158,9 @@ template auto canonical_view(Seq&& s) { using std::begin; using std::end; - - return make_range( - make_sentinel_iterator(begin(s), end(s)), - make_sentinel_end(begin(s), end(s))); + auto b = begin(s); + auto e = end(s); + return make_range(make_sentinel_iterator(b, e), make_sentinel_end(b, e)); } // Strictly evaluate end point in sentinel-terminated range and present as a range over @@ -171,7 +170,6 @@ template auto strict_view(Seq&& s) { using std::begin; using std::end; - auto b = begin(s); auto e = end(s); return make_range(b, b==e? b: std::next(util::upto(b, e))); diff --git a/test/unit-distributed/test_distributed_for_each.cpp b/test/unit-distributed/test_distributed_for_each.cpp index 5c545e8c53..b9ca83b228 100644 --- a/test/unit-distributed/test_distributed_for_each.cpp +++ b/test/unit-distributed/test_distributed_for_each.cpp @@ -40,10 +40,9 @@ TEST(distributed_for_each, one_zero) { for (int i = 0; i < rank; ++i) { data.push_back(rank); } auto sample = [&](const util::range& range) { - const auto origin_rank = range.empty() ? 0 : range.front(); - + std::size_t origin_rank = range.empty() ? 0 : range.front(); EXPECT_EQ(origin_rank, range.size()); - for (const auto& value: range) { EXPECT_EQ(value, origin_rank); } + for (std::size_t value: range) { EXPECT_EQ(value, origin_rank); } ++call_count; }; @@ -71,14 +70,14 @@ TEST(distributed_for_each, multiple) { auto sample = [&](const util::range& range_1, const util::range& range_2, const util::range*>& range_3) { - const auto origin_rank = range_1.empty() ? 0 : range_1.front(); + std::size_t origin_rank = range_1.empty() ? 0 : range_1.front(); EXPECT_EQ(origin_rank + 1, range_1.size()); EXPECT_EQ(range_2.size(), 2 * range_1.size()); EXPECT_EQ(range_3.size(), 3 * range_1.size()); - for (const auto& value: range_1) { EXPECT_EQ(value, origin_rank); } - for (const auto& value: range_2) { EXPECT_EQ(value, double(origin_rank)); } - for (const auto& value: range_3) { EXPECT_EQ(value, std::complex(origin_rank)); } + for (std::size_t value: range_1) { EXPECT_EQ(value, origin_rank); } + for (auto value: range_2) { EXPECT_EQ(value, double(origin_rank)); } + for (auto value: range_3) { EXPECT_EQ(value, std::complex(origin_rank)); } ++call_count; }; diff --git a/test/unit-distributed/test_network_generation.cpp b/test/unit-distributed/test_network_generation.cpp index f7e3b55fe3..1a6e094742 100644 --- a/test/unit-distributed/test_network_generation.cpp +++ b/test/unit-distributed/test_network_generation.cpp @@ -125,8 +125,8 @@ TEST(network_generation, all) { } for (const auto& group: decomp.groups()) { - const auto num_dest = group.kind == cell_kind::spike_source ? 0 : 1; - for (const auto gid: group.gids) { + std::size_t num_dest = group.kind == cell_kind::spike_source ? 0 : 1; + for (std::size_t gid: group.gids) { EXPECT_EQ(connections_by_dest[gid].size(), num_cells * num_dest); } } @@ -141,7 +141,7 @@ TEST(network_generation, cable_only) { const auto weight = 2.0; const auto delay = 3.0; - const auto num_cells = 3 * num_ranks; + const std::size_t num_cells = 3 * num_ranks; auto rec = network_test_recipe(num_cells, selection, weight, delay); @@ -161,7 +161,7 @@ TEST(network_generation, cable_only) { for (const auto gid: group.gids) { // Only one third is a cable cell EXPECT_EQ(connections_by_dest[gid].size(), - group.kind == cell_kind::cable ? num_cells / 3 : 0); + group.kind == cell_kind::cable ? num_cells / 3 : 0); } } } diff --git a/test/unit/common.hpp b/test/unit/common.hpp index e02b9df20a..23a08da975 100644 --- a/test/unit/common.hpp +++ b/test/unit/common.hpp @@ -12,6 +12,7 @@ #include #include #include +#include #include diff --git a/test/unit/test_range.cpp b/test/unit/test_range.cpp index 02e889fc0c..de86ef7ecd 100644 --- a/test/unit/test_range.cpp +++ b/test/unit/test_range.cpp @@ -14,7 +14,6 @@ #include "util/meta.hpp" #include "util/range.hpp" #include "util/rangeutil.hpp" -#include "util/sentinel.hpp" #include "util/transform.hpp" #include "common.hpp" @@ -442,41 +441,34 @@ struct foo { }; TEST(range, sort) { - char cstr[] = "howdy"; - - auto cstr_range = util::make_range(std::begin(cstr), null_terminated); - - // Alas, no forward_iterator sort yet, so make a strict (non-sentinel) - // range to sort on below - - // simple sort - util::sort(util::strict_view(cstr_range)); - EXPECT_EQ("dhowy"s, cstr); - - // reverse sort by transform c to -c - util::sort_by(util::strict_view(cstr_range), [](char c) { return -c; }); - EXPECT_EQ("ywohd"s, cstr); - - // stable sort: move capitals to front, numbers to back - auto rank = [](char c) { - return std::isupper(c)? 0: std::isdigit(c)? 2: 1; - }; - - char mixed[] = "t5hH4E3erLL2e1O"; - auto mixed_range = util::make_range(std::begin(mixed), null_terminated); - - util::stable_sort_by(util::strict_view(mixed_range), rank); - EXPECT_EQ("HELLOthere54321"s, mixed); - - - // sort with user-provided less comparison function - - std::vector X = {{0, 5}, {1, 4}, {2, 3}, {3, 2}, {4, 1}, {5, 0}}; - - util::sort(X, [](const foo& l, const foo& r) {return l.y{{5, 0}, {4, 1}, {3, 2}, {2, 3}, {1, 4}, {0, 5}})); - util::sort(X, [](const foo& l, const foo& r) {return l.x{{0, 5}, {1, 4}, {2, 3}, {3, 2}, {4, 1}, {5, 0}})); + { + // simple sort + char cstr[] = "howdy"; + util::sort(util::range_n(cstr, 5)); + EXPECT_EQ("dhowy"s, cstr); + } + { + // reverse sort by transform c to -c + char cstr[] = "howdy"; + util::sort_by(util::range_n(cstr, 5), + [](char c) { return -c; }); + EXPECT_EQ("ywohd"s, cstr); + } + { + // stable sort: move capitals to front, numbers to back + char mixed[] = "t5hH4E3erLL2e1O"; + util::stable_sort_by(util::strict_view(util::make_range(std::begin(mixed), null_terminated)), + [](char c) { return std::isupper(c)? 0: std::isdigit(c)? 2: 1; }); + EXPECT_EQ("HELLOthere54321"s, mixed); + } + { + // sort with user-provided less comparison function + std::vector X = {{0, 5}, {1, 4}, {2, 3}, {3, 2}, {4, 1}, {5, 0}}; + util::sort(X, [](const foo& l, const foo& r) {return l.y{{5, 0}, {4, 1}, {3, 2}, {2, 3}, {1, 4}, {0, 5}})); + util::sort(X, [](const foo& l, const foo& r) {return l.x{{0, 5}, {1, 4}, {2, 3}, {3, 2}, {4, 1}, {5, 0}})); + } } TEST(range, sum) { diff --git a/test/unit/test_simd.cpp b/test/unit/test_simd.cpp index 87ea359cb4..0a6e116c0d 100644 --- a/test/unit/test_simd.cpp +++ b/test/unit/test_simd.cpp @@ -360,13 +360,13 @@ TYPED_TEST_P(simd_value, comparison) { for (unsigned i = 0; ib;