From 0770b328c768c0d268f6b17bc14eb56e4b1fc66f Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:01:24 +0200 Subject: [PATCH 01/16] test: fix some mismatched signed/unsigned comparisons gcc likes to complain about sized/unsigned compares as they can yield surprising results. The fixes are trivial, so apply them. --- test/boost/cached_file_test.cc | 4 ++-- test/boost/chunked_managed_vector_test.cc | 24 +++++++++---------- test/boost/lister_test.cc | 2 +- test/boost/memtable_test.cc | 2 +- test/boost/partitioner_test.cc | 2 +- .../reader_concurrency_semaphore_test.cc | 2 +- test/boost/sstable_compaction_test.cc | 10 ++++---- test/unit/cross_shard_barrier_test.cc | 4 ++-- 8 files changed, 25 insertions(+), 25 deletions(-) diff --git a/test/boost/cached_file_test.cc b/test/boost/cached_file_test.cc index 2f39481bf3..87c2d16972 100644 --- a/test/boost/cached_file_test.cc +++ b/test/boost/cached_file_test.cc @@ -315,7 +315,7 @@ SEASTAR_THREAD_TEST_CASE(test_stress_eviction) { return cf_lru.evict(); }); - for (int i = 0; i < (cached_size / page_size); ++i) { + for (size_t i = 0; i < (cached_size / page_size); ++i) { read_to_string(cf, page_size * i, page_size); } @@ -335,7 +335,7 @@ SEASTAR_THREAD_TEST_CASE(test_stress_eviction) { testlog.debug("Memory: allocated={}, free={}", seastar::memory::stats().allocated_memory(), seastar::memory::stats().free_memory()); testlog.debug("Starting test..."); - for (int j = 0; j < n_pages * 16; ++j) { + for (size_t j = 0; j < n_pages * 16; ++j) { testlog.trace("Allocating"); auto stride = tests::random::get_int(1, 20); auto page_idx = tests::random::get_int(n_pages - stride); diff --git a/test/boost/chunked_managed_vector_test.cc b/test/boost/chunked_managed_vector_test.cc index a0987832fb..e0fe9c6acb 100644 --- a/test/boost/chunked_managed_vector_test.cc +++ b/test/boost/chunked_managed_vector_test.cc @@ -231,17 +231,17 @@ SEASTAR_TEST_CASE(test_chunk_reserve) { for (auto conf : { // std::make_pair(reserve size, push count) - std::make_pair(0, 4000), - std::make_pair(100, 4000), - std::make_pair(200, 4000), - std::make_pair(1000, 4000), - std::make_pair(2000, 4000), - std::make_pair(3000, 4000), - std::make_pair(5000, 4000), - std::make_pair(500, 8000), - std::make_pair(1000, 8000), - std::make_pair(2000, 8000), - std::make_pair(8000, 500), + std::make_pair(0u, 4000u), + std::make_pair(100u, 4000u), + std::make_pair(200u, 4000u), + std::make_pair(1000u, 4000u), + std::make_pair(2000u, 4000u), + std::make_pair(3000u, 4000u), + std::make_pair(5000u, 4000u), + std::make_pair(500u, 8000u), + std::make_pair(1000u, 8000u), + std::make_pair(2000u, 8000u), + std::make_pair(8000u, 500u), }) { with_allocator(region.allocator(), [&] { @@ -275,7 +275,7 @@ SEASTAR_TEST_CASE(test_correctness_when_crossing_chunk_boundary) { size_t max_chunk_size = lsa::chunked_managed_vector::max_chunk_capacity(); lsa::chunked_managed_vector v; - for (auto i = 0; i < (max_chunk_size + 1); i++) { + for (size_t i = 0; i < (max_chunk_size + 1); i++) { v.push_back(i); } BOOST_REQUIRE(v.back() == max_chunk_size); diff --git a/test/boost/lister_test.cc b/test/boost/lister_test.cc index c2784948f8..1072505c5d 100644 --- a/test/boost/lister_test.cc +++ b/test/boost/lister_test.cc @@ -168,7 +168,7 @@ SEASTAR_TEST_CASE(test_directory_lister_close) { auto dl = directory_lister(tmp.path()); auto initial = tests::random::get_int(count); BOOST_TEST_MESSAGE(fmt::format("Getting {} dir entries", initial)); - for (auto i = 0; i < initial; i++) { + for (decltype(initial) i = 0; i < initial; i++) { auto de = co_await dl.get(); BOOST_REQUIRE(de); } diff --git a/test/boost/memtable_test.cc b/test/boost/memtable_test.cc index b0ddfe195d..dca11dc5c1 100644 --- a/test/boost/memtable_test.cc +++ b/test/boost/memtable_test.cc @@ -1136,7 +1136,7 @@ SEASTAR_TEST_CASE(flushing_rate_is_reduced_if_compaction_doesnt_keep_up) { ::usleep(sleep_ms * 1000); co_await db.apply(t.schema(), freeze(gen()), tracing::trace_state_ptr(), db::commitlog::force_sync::yes, db::no_timeout); co_await t.flush(); - BOOST_ASSERT(t.sstables_count() < t.schema()->max_compaction_threshold() * 2); + BOOST_ASSERT(t.sstables_count() < size_t(t.schema()->max_compaction_threshold() * 2)); } co_await drop_table(); } diff --git a/test/boost/partitioner_test.cc b/test/boost/partitioner_test.cc index 1c258da4bf..2ff9c65b7f 100644 --- a/test/boost/partitioner_test.cc +++ b/test/boost/partitioner_test.cc @@ -561,7 +561,7 @@ SEASTAR_THREAD_TEST_CASE(test_dht_subtract_ranges) { dht::partition_range_vector ranges; ranges.reserve(count); - for (auto i = 0; i < count; i++) { + for (size_t i = 0; i < count; i++) { dht::token t0 = get_random_token(); dht::token t1 = get_random_token(); if (t1 < t0) { diff --git a/test/boost/reader_concurrency_semaphore_test.cc b/test/boost/reader_concurrency_semaphore_test.cc index 84ff23ab2e..a8f610d7a1 100644 --- a/test/boost/reader_concurrency_semaphore_test.cc +++ b/test/boost/reader_concurrency_semaphore_test.cc @@ -1363,7 +1363,7 @@ memory_limit_table create_memory_limit_table(cql_test_env& env, uint64_t target_ const auto sstable_write_concurrency = 16; - auto num_sstables = 0; + uint64_t num_sstables = 0; parallel_for_each(boost::irange(0, sstable_write_concurrency), [&] (int i) { return seastar::async([&] { while (num_sstables != target_num_sstables) { diff --git a/test/boost/sstable_compaction_test.cc b/test/boost/sstable_compaction_test.cc index d79b19a507..26d5100656 100644 --- a/test/boost/sstable_compaction_test.cc +++ b/test/boost/sstable_compaction_test.cc @@ -135,7 +135,7 @@ static std::unique_ptr make_strategy_control_for_test(bool has static void assert_table_sstable_count(table_for_tests& t, size_t expected_count) { testlog.info("sstable_set_size={}, live_sstable_count={}, expected={}", t->get_sstables()->size(), t->get_stats().live_sstable_count, expected_count); BOOST_REQUIRE(t->get_sstables()->size() == expected_count); - BOOST_REQUIRE(t->get_stats().live_sstable_count == expected_count); + BOOST_REQUIRE(uint64_t(t->get_stats().live_sstable_count) == expected_count); } SEASTAR_TEST_CASE(compaction_manager_basic_test) { @@ -4233,13 +4233,13 @@ SEASTAR_TEST_CASE(max_ongoing_compaction_test) { auto s = schemas[idx]; auto cf = tables[idx]; auto cft = column_family_test(cf); - for (auto i = 0; i < num_sstables; i++) { + for (size_t i = 0; i < num_sstables; i++) { auto muts = { make_expiring_cell(s, std::chrono::hours(1)) }; cft.add_sstable(make_sstable_containing([&sst_gen, idx] { return sst_gen(idx); }, muts)).get(); } }; - for (auto i = 0; i < num_tables; i++) { + for (size_t i = 0; i < num_tables; i++) { add_sstables_to_table(i, DEFAULT_MIN_COMPACTION_THRESHOLD); } @@ -4551,7 +4551,7 @@ SEASTAR_TEST_CASE(simple_backlog_controller_test) { auto tiers = get_total_tiers(target_disk_usage); auto t = create_table(); - for (auto tier_idx = 0; tier_idx < tiers; tier_idx++) { + for (size_t tier_idx = 0; tier_idx < tiers; tier_idx++) { auto tier_size = get_size_for_tier(tier_idx); if (tier_size > available_space) { break; @@ -4628,7 +4628,7 @@ SEASTAR_TEST_CASE(test_compaction_strategy_cleanup_method) { std::vector candidates; candidates.reserve(all_files); - for (auto i = 0; i < all_files; i++) { + for (size_t i = 0; i < all_files; i++) { auto current_step = duration_cast(step_base) * i; auto sst = make_sstable_containing(sst_gen, {make_mutation(i, next_timestamp(current_step))}); sst->set_sstable_level(sstable_level); diff --git a/test/unit/cross_shard_barrier_test.cc b/test/unit/cross_shard_barrier_test.cc index da1e95e7ff..2ff76aba20 100644 --- a/test/unit/cross_shard_barrier_test.cc +++ b/test/unit/cross_shard_barrier_test.cc @@ -103,14 +103,14 @@ int main(int argc, char **argv) { w.invoke_on_all(&worker::loop).get(); w.stop().get(); - for (int i = 0; i < smp::count * phases_scale; i++) { + for (size_t i = 0; i < smp::count * phases_scale; i++) { sharded w; w.start(utils::cross_shard_barrier()).get(); try { w.invoke_on_all(&worker::loop_with_error).get(); } catch (...) { auto ph = w.invoke_on(0, [] (auto& w) { return w.get_phase(); }).get0(); - for (int c = 1; c < smp::count; c++) { + for (size_t c = 1; c < smp::count; c++) { auto ph_2 = w.invoke_on(c, [] (auto& w) { return w.get_phase(); }).get0(); if (ph_2 != ph) { fmt::print("aborted barrier passed shard through\n"); From 429650e50873462d89d00a2f60c8b4c6b4144ac2 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:02:29 +0200 Subject: [PATCH 02/16] alternator: streams: fix signed/unsigned comparison We compare a signed variable to an unsigned one, which can yield surprising results. In this case, it is harmless since we already validated the signed input is positive, but use std::cmp_less() to quench any doubts (and warnings). --- alternator/streams.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/alternator/streams.cc b/alternator/streams.cc index 0e310105a5..6377e51d67 100644 --- a/alternator/streams.cc +++ b/alternator/streams.cc @@ -167,7 +167,7 @@ future alternator::executor::list_str // generate duplicates in a paged listing here. Can obviously miss things if they // are added between paged calls and end up with a "smaller" UUID/ARN, but that // is to be expected. - if (limit < cfs.size() || streams_start) { + if (std::cmp_less(limit, cfs.size()) || streams_start) { std::sort(cfs.begin(), cfs.end(), [](const data_dictionary::table& t1, const data_dictionary::table& t2) { return t1.schema()->id().uuid() < t2.schema()->id().uuid(); }); From 7ab65379b90e1f28cc8b234c94511e225b1f0a49 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:03:45 +0200 Subject: [PATCH 03/16] api: task_manager: fix signed/unsigned compare Trivial fix by changing the type of the induction variable. --- api/task_manager.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/task_manager.cc b/api/task_manager.cc index 3a2085d572..609e2946ef 100644 --- a/api/task_manager.cc +++ b/api/task_manager.cc @@ -205,7 +205,7 @@ void set_task_manager(http_context& ctx, routes& r, db::config& cfg) { while (!q.empty()) { auto& current = q.front(); res.push_back(co_await retrieve_status(current)); - for (auto i = 0; i < current->get_children().size(); ++i) { + for (size_t i = 0; i < current->get_children().size(); ++i) { q.push(co_await current->get_children()[i].copy()); } q.pop(); From 7bb717d2f9c099a0bca6f7b74b6217fe465cf656 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:05:32 +0200 Subject: [PATCH 04/16] treewide: prevent redefining names gcc dislikes a member name that matches a type name, as it changes the type name retroactively. Fix by fully-qualifying the type name, so it is not changed by the newly-introduced member. --- cell_locking.hh | 2 +- cql3/statements/raw/describe_statement.hh | 4 ++-- db/view/view.hh | 2 +- schema/schema.hh | 4 ++-- tasks/task_manager.hh | 8 ++++---- test/lib/sstable_test_env.hh | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cell_locking.hh b/cell_locking.hh index 90dd2c020a..d3be6c89fd 100644 --- a/cell_locking.hh +++ b/cell_locking.hh @@ -175,7 +175,7 @@ private: } class hasher { - const schema* _schema; // pointer instead of reference for default assignment + const ::schema* _schema; // pointer instead of reference for default assignment public: explicit hasher(const schema& s) : _schema(&s) { } diff --git a/cql3/statements/raw/describe_statement.hh b/cql3/statements/raw/describe_statement.hh index 25323a4d44..ab28200478 100644 --- a/cql3/statements/raw/describe_statement.hh +++ b/cql3/statements/raw/describe_statement.hh @@ -51,10 +51,10 @@ private: bool only_keyspace; }; struct describe_listing { - element_type element_type; + describe_statement::element_type element_type; }; struct describe_element { - element_type element_type; + describe_statement::element_type element_type; std::optional keyspace; sstring name; }; diff --git a/db/view/view.hh b/db/view/view.hh index 0308b92333..a657109015 100644 --- a/db/view/view.hh +++ b/db/view/view.hh @@ -111,7 +111,7 @@ public: return _row.is_live(s, column_kind(), base_tombstone, now); } - column_kind column_kind() const { + ::column_kind column_kind() const { return _key.has_value() ? column_kind::regular_column : column_kind::static_column; } diff --git a/schema/schema.hh b/schema/schema.hh index d9e3122589..f7f9cdbe15 100644 --- a/schema/schema.hh +++ b/schema/schema.hh @@ -614,12 +614,12 @@ private: int32_t _min_index_interval = DEFAULT_MIN_INDEX_INTERVAL; int32_t _max_index_interval = 2048; int32_t _memtable_flush_period = 0; - speculative_retry _speculative_retry = ::speculative_retry(speculative_retry::type::PERCENTILE, 0.99); + ::speculative_retry _speculative_retry = ::speculative_retry(speculative_retry::type::PERCENTILE, 0.99); // This is the compaction strategy that will be used by default on tables which don't have one explicitly specified. sstables::compaction_strategy_type _compaction_strategy = sstables::compaction_strategy_type::size_tiered; std::map _compaction_strategy_options; bool _compaction_enabled = true; - caching_options _caching_options; + ::caching_options _caching_options; table_schema_version _version; std::unordered_map _dropped_columns; std::map _collections; diff --git a/tasks/task_manager.hh b/tasks/task_manager.hh index 04d49c46ac..dcca3feb6c 100644 --- a/tasks/task_manager.hh +++ b/tasks/task_manager.hh @@ -48,7 +48,7 @@ private: modules _modules; config _cfg; seastar::abort_source _as; - optimized_optional _abort_subscription; + optimized_optional _abort_subscription; serialized_action _update_task_ttl_action; utils::observer _task_ttl_observer; uint32_t _task_ttl; @@ -99,8 +99,8 @@ public: foreign_task_vector _children; shared_promise<> _done; module_ptr _module; - abort_source _as; - optimized_optional _shutdown_subscription; + seastar::abort_source _as; + optimized_optional _shutdown_subscription; public: impl(module_ptr module, task_id id, uint64_t sequence_number, std::string keyspace, std::string table, std::string entity, task_id parent_id) noexcept; virtual ~impl() = default; @@ -203,7 +203,7 @@ public: } }; public: - task_manager(config cfg, abort_source& as) noexcept; + task_manager(config cfg, seastar::abort_source& as) noexcept; task_manager() noexcept; modules& get_modules() noexcept; diff --git a/test/lib/sstable_test_env.hh b/test/lib/sstable_test_env.hh index 85e9faa6c9..54add7367b 100644 --- a/test/lib/sstable_test_env.hh +++ b/test/lib/sstable_test_env.hh @@ -51,7 +51,7 @@ class test_env { tmpdir dir; std::unique_ptr db_config; directory_semaphore dir_sem; - cache_tracker cache_tracker; + ::cache_tracker cache_tracker; gms::feature_service feature_service; db::nop_large_data_handler nop_ld_handler; test_env_sstables_manager mgr; From 32cc975b2fe8ef33de7e2fb72b2f62e64e9aab42 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:06:53 +0200 Subject: [PATCH 05/16] compaction: error on invalid scrub type gcc allows an enum to contain a value outside its enum set, so we need to handle it. Since it shouldn't happen, signal an internal error. --- compaction/compaction_manager.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/compaction/compaction_manager.cc b/compaction/compaction_manager.cc index 2f601c853d..cea025b709 100644 --- a/compaction/compaction_manager.cc +++ b/compaction/compaction_manager.cc @@ -1573,6 +1573,7 @@ future compaction_manager::perform_sst case sstables::compaction_type_options::scrub::quarantine_mode::only: return sst->is_quarantined(); } + on_internal_error(cmlog, "bad scrub quarantine mode"); })); return make_ready_future>(std::move(sstables)); }, can_purge_tombstones::no); From 41a2856f788b854f28f139ac57b51e39d13b78bc Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:08:09 +0200 Subject: [PATCH 06/16] cql3: expr: fix serialize_listlike() reference-to-temporary with gcc serialize_listlike() is called with a range of either managed_bytes or managed_bytes_opt. If the former, then iterating and assigning to a loop induction variable of type managed_byted_opt& will bind the reference to a temporary managed_bytes_opt, which gcc dislikes. Fix by performing the binding in a separate statement, which allows for lifetime extension. --- cql3/expr/expression.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cql3/expr/expression.cc b/cql3/expr/expression.cc index 1e4be98dfc..8dab0ef1a7 100644 --- a/cql3/expr/expression.cc +++ b/cql3/expr/expression.cc @@ -1970,7 +1970,8 @@ static managed_bytes serialize_listlike(const Range& elements, const char* colle collection_name, elements.size(), std::numeric_limits::max())); } - for (const managed_bytes_opt& element_opt : elements) { + for (const auto& element : elements) { + const managed_bytes_opt& element_opt = element; if (element_opt) { auto& element = *element_opt; if (element.size() > std::numeric_limits::max()) { From 9ced89a41c95099b572285dfc79f60a96068dce9 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:10:34 +0200 Subject: [PATCH 07/16] keys: disambiguate construction from initializer_list Some tests initialize via an initializer_list, but gcc finds other valid constructors via vector. Disambiguate by adding a constructor that accepts the initializer_list, and forward to the wanted constructor. --- keys.hh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/keys.hh b/keys.hh index 2d85e61257..6f2e936db2 100644 --- a/keys.hh +++ b/keys.hh @@ -697,6 +697,7 @@ public: partition_key(std::vector v) : compound_wrapper(managed_bytes(c_type::serialize_value(std::move(v)))) { } + partition_key(std::initializer_list v) : partition_key(std::vector(v)) {} partition_key(partition_key&& v) = default; partition_key(const partition_key& v) = default; @@ -812,6 +813,7 @@ public: clustering_key_prefix(std::vector v) : prefix_compound_wrapper(compound::element_type::serialize_value(std::move(v))) { } + clustering_key_prefix(std::initializer_list v) : clustering_key_prefix(std::vector(v)) {} clustering_key_prefix(clustering_key_prefix&& v) = default; clustering_key_prefix(const clustering_key_prefix& v) = default; From a806024e1df227302b063ae9ce359f0d71365b19 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:12:31 +0200 Subject: [PATCH 08/16] treewide: avoid unused variables in if statements gcc warns about unused variables declared in if statements. Just drop them. --- raft/server.cc | 4 ++-- service/storage_proxy.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/raft/server.cc b/raft/server.cc index 13c53c5971..a76a4fb085 100644 --- a/raft/server.cc +++ b/raft/server.cc @@ -697,10 +697,10 @@ future server_impl::execute_modify_config(server_id from, if (const auto* ex = dynamic_cast(&e)) { co_return add_entry_reply{transient_error{std::current_exception(), ex->leader}}; } - if (const auto* ex = dynamic_cast(&e)) { + if (dynamic_cast(&e)) { co_return add_entry_reply{transient_error{std::current_exception(), {}}}; } - if (const auto* ex = dynamic_cast(&e)) { + if (dynamic_cast(&e)) { co_return add_entry_reply{transient_error{std::current_exception(), {}}}; } throw; diff --git a/service/storage_proxy.cc b/service/storage_proxy.cc index 664e129588..b163a5531c 100644 --- a/service/storage_proxy.cc +++ b/service/storage_proxy.cc @@ -3901,10 +3901,10 @@ public: void error(gms::inet_address ep, std::exception_ptr eptr) { sstring why; error_kind kind = error_kind::FAILURE; - if (auto ex = try_catch(eptr)) { + if (try_catch(eptr)) { // There might be a lot of those, so ignore kind = error_kind::RATE_LIMIT; - } else if (auto ex = try_catch(eptr)) { + } else if (try_catch(eptr)) { // do not report connection closed exception, gossiper does that kind = error_kind::DISCONNECT; } else if (try_catch(eptr)) { From 94a10ed6abf04bdc26b49cc980949333c6b3def7 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:13:24 +0200 Subject: [PATCH 09/16] repair: fix incorrect signed/unsigned compare A signed/unsigned compare can overflow. Fix by using the safer std::cmp_greater(). The problem is minor as the user is unlikely to send a negative id. --- repair/repair.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/repair/repair.cc b/repair/repair.cc index 25f2121742..e90223aef2 100644 --- a/repair/repair.cc +++ b/repair/repair.cc @@ -377,7 +377,7 @@ void repair_module::done(repair_uniq_id id, bool succeeded) { } repair_status repair_module::get(int id) const { - if (id > _sequence_number) { + if (std::cmp_greater(id, _sequence_number)) { throw std::runtime_error(format("unknown repair id {}", id)); } auto it = _status.find(id); @@ -390,7 +390,7 @@ repair_status repair_module::get(int id) const { future repair_module::repair_await_completion(int id, std::chrono::steady_clock::time_point timeout) { return seastar::with_gate(async_gate(), [this, id, timeout] { - if (id > _sequence_number) { + if (std::cmp_greater(id, _sequence_number)) { return make_exception_future(std::runtime_error(format("unknown repair id {}", id))); } return repeat_until_value([this, id, timeout] { From bc0bba10b4e4b7667511ce5aad69616a087f20fa Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:15:00 +0200 Subject: [PATCH 10/16] repair: fix signed/unsigned compare Fix the loop induction variable to have the same type as the termination value. --- repair/repair.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/repair/repair.cc b/repair/repair.cc index e90223aef2..c690038c1d 100644 --- a/repair/repair.cc +++ b/repair/repair.cc @@ -922,7 +922,7 @@ private: future<> shard_repair_task_impl::do_repair_ranges() { // Repair tables in the keyspace one after another assert(table_names().size() == table_ids.size()); - for (int idx = 0; idx < table_ids.size(); idx++) { + for (size_t idx = 0; idx < table_ids.size(); idx++) { auto table_id = table_ids[idx]; auto table_name = table_names()[idx]; // repair all the ranges in limited parallelism From 83e149c341036c49bce5c662b28583419ba7cb89 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:15:50 +0200 Subject: [PATCH 11/16] test: reader_concurrency_semaphore_test: handle all enum values gcc considers values outside the enum class enumeration lists to be valid, so handle them. In this case, we don't think they can happen, so abort. --- test/boost/reader_concurrency_semaphore_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/boost/reader_concurrency_semaphore_test.cc b/test/boost/reader_concurrency_semaphore_test.cc index a8f610d7a1..253c9014f6 100644 --- a/test/boost/reader_concurrency_semaphore_test.cc +++ b/test/boost/reader_concurrency_semaphore_test.cc @@ -1115,6 +1115,7 @@ public: case state::release_memory: return "state::release_memory"; case state::done: return "state::done"; } + std::abort(); }; private: reader_concurrency_semaphore& _sem; From 32a724fadad32948e76e3c89114dc508b81e444f Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:18:04 +0200 Subject: [PATCH 12/16] test: raft: fsm_test: disambiguate raft::configuration construction gcc thinks the constructor call is ambiguous since "{}" can match the default constructor. Fix by making the parameter type explicit. Use "{}" for the constructor call to avoid the most-vexing-parse problem. --- test/raft/fsm_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/raft/fsm_test.cc b/test/raft/fsm_test.cc index 5a37638703..cfb930d339 100644 --- a/test/raft/fsm_test.cc +++ b/test/raft/fsm_test.cc @@ -1089,7 +1089,7 @@ BOOST_AUTO_TEST_CASE(test_empty_configuration) { server_id id1 = id(); - raft::configuration cfg({}); + raft::configuration cfg{config_member_set()}; raft::log log(raft::snapshot_descriptor{.idx = index_t{0}, .config = cfg}); auto follower = create_follower(id1, std::move(log)); // Initial state is follower @@ -1107,7 +1107,7 @@ BOOST_AUTO_TEST_CASE(test_empty_configuration) { election_timeout(leader); BOOST_CHECK(leader.is_leader()); // Transitioning to an empty configuration is not supported. - BOOST_CHECK_THROW(leader.add_entry(raft::configuration({})), std::invalid_argument); + BOOST_CHECK_THROW(leader.add_entry(raft::configuration{config_member_set()}), std::invalid_argument); leader.add_entry(config_from_ids({id1, id2})); communicate(leader, follower); From bdfc0aa7488af78d32e8fdc87bae9ddadf8f4fb3 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:21:17 +0200 Subject: [PATCH 13/16] utils, types, test: extract lexicographical compare utilities UUID_test uses lexicograhical_compare from the types module. This is a layering violation, since UUIDs are at a much lower level than the database type system. In practical terms, this cause link failures with gcc due to some thread-local-storage variables defined in types.hh but not provided by any object, since we don't link with types.o in this test. Fix by extracting the relevant functions into a new header. --- test/boost/UUID_test.cc | 2 +- types/types.hh | 154 +---------------------------- utils/lexicographical_compare.hh | 162 +++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+), 154 deletions(-) create mode 100644 utils/lexicographical_compare.hh diff --git a/test/boost/UUID_test.cc b/test/boost/UUID_test.cc index 44ca537d0a..3ea1973437 100644 --- a/test/boost/UUID_test.cc +++ b/test/boost/UUID_test.cc @@ -11,7 +11,7 @@ #include #include #include "utils/UUID_gen.hh" -#include "types/types.hh" +#include "utils/lexicographical_compare.hh" BOOST_AUTO_TEST_CASE(test_generation_of_name_based_UUID) { auto uuid = utils::UUID_gen::get_name_UUID("systembatchlog"); diff --git a/types/types.hh b/types/types.hh index 337554d47b..d2e926ead1 100644 --- a/types/types.hh +++ b/types/types.hh @@ -33,6 +33,7 @@ #include "utils/managed_bytes.hh" #include "utils/bit_cast.hh" #include "utils/chunked_vector.hh" +#include "utils/lexicographical_compare.hh" #include "tasks/types.hh" class tuple_type_impl; @@ -50,159 +51,6 @@ class cql3_type; } -// Specifies position in a lexicographically ordered sequence -// relative to some value. -// -// For example, if used with a value "bc" with lexicographical ordering on strings, -// each enum value represents the following positions in an example sequence: -// -// aa -// aaa -// b -// ba -// --> before_all_prefixed -// bc -// --> before_all_strictly_prefixed -// bca -// bcd -// --> after_all_prefixed -// bd -// bda -// c -// ca -// -enum class lexicographical_relation : int8_t { - before_all_prefixed, - before_all_strictly_prefixed, - after_all_prefixed -}; - -// Like std::lexicographical_compare but injects values from shared sequence (types) to the comparator -// Compare is an abstract_type-aware less comparator, which takes the type as first argument. -template -bool lexicographical_compare(TypesIterator types, InputIt1 first1, InputIt1 last1, - InputIt2 first2, InputIt2 last2, Compare comp) { - while (first1 != last1 && first2 != last2) { - if (comp(*types, *first1, *first2)) { - return true; - } - if (comp(*types, *first2, *first1)) { - return false; - } - ++first1; - ++first2; - ++types; - } - return (first1 == last1) && (first2 != last2); -} - -// Like std::lexicographical_compare but injects values from shared sequence -// (types) to the comparator. Compare is an abstract_type-aware trichotomic -// comparator, which takes the type as first argument. -template -requires requires (TypesIterator types, InputIt1 i1, InputIt2 i2, Compare cmp) { - { cmp(*types, *i1, *i2) } -> std::same_as; -} -std::strong_ordering lexicographical_tri_compare(TypesIterator types_first, TypesIterator types_last, - InputIt1 first1, InputIt1 last1, - InputIt2 first2, InputIt2 last2, - Compare comp, - lexicographical_relation relation1 = lexicographical_relation::before_all_strictly_prefixed, - lexicographical_relation relation2 = lexicographical_relation::before_all_strictly_prefixed) { - while (types_first != types_last && first1 != last1 && first2 != last2) { - auto c = comp(*types_first, *first1, *first2); - if (c != 0) { - return c; - } - ++first1; - ++first2; - ++types_first; - } - bool e1 = first1 == last1; - bool e2 = first2 == last2; - if (e1 && e2) { - return static_cast(relation1) <=> static_cast(relation2); - } - if (e2) { - return relation2 == lexicographical_relation::after_all_prefixed ? std::strong_ordering::less : std::strong_ordering::greater; - } else if (e1) { - return relation1 == lexicographical_relation::after_all_prefixed ? std::strong_ordering::greater : std::strong_ordering::less; - } else { - return std::strong_ordering::equal; - } -} - -// Trichotomic version of std::lexicographical_compare() -template -requires requires (InputIt1 i1, InputIt2 i2, Compare c) { - { c(*i1, *i2) } -> std::same_as; -} -std::strong_ordering lexicographical_tri_compare(InputIt1 first1, InputIt1 last1, - InputIt2 first2, InputIt2 last2, - Compare comp, - lexicographical_relation relation1 = lexicographical_relation::before_all_strictly_prefixed, - lexicographical_relation relation2 = lexicographical_relation::before_all_strictly_prefixed) { - while (first1 != last1 && first2 != last2) { - auto c = comp(*first1, *first2); - if (c != 0) { - return c; - } - ++first1; - ++first2; - } - bool e1 = first1 == last1; - bool e2 = first2 == last2; - if (e1 == e2) { - return static_cast(relation1) <=> static_cast(relation2); - } - if (e2) { - return relation2 == lexicographical_relation::after_all_prefixed ? std::strong_ordering::less : std::strong_ordering::greater; - } else { - return relation1 == lexicographical_relation::after_all_prefixed ? std::strong_ordering::greater : std::strong_ordering::less; - } -} - -// A trichotomic comparator for prefix equality total ordering. -// In this ordering, two sequences are equal iff any of them is a prefix -// of the another. Otherwise, lexicographical ordering determines the order. -// -// 'comp' is an abstract_type-aware trichotomic comparator, which takes the -// type as first argument. -// -template -requires requires (TypesIterator ti, InputIt1 i1, InputIt2 i2, Compare c) { - { c(*ti, *i1, *i2) } -> std::same_as; -} -std::strong_ordering prefix_equality_tri_compare(TypesIterator types, InputIt1 first1, InputIt1 last1, - InputIt2 first2, InputIt2 last2, Compare comp) { - while (first1 != last1 && first2 != last2) { - auto c = comp(*types, *first1, *first2); - if (c != 0) { - return c; - } - ++first1; - ++first2; - ++types; - } - return std::strong_ordering::equal; -} - -// Returns true iff the second sequence is a prefix of the first sequence -// Equality is an abstract_type-aware equality checker which takes the type as first argument. -template -bool is_prefixed_by(TypesIterator types, InputIt1 first1, InputIt1 last1, - InputIt2 first2, InputIt2 last2, Equality equality) { - while (first1 != last1 && first2 != last2) { - if (!equality(*types, *first1, *first2)) { - return false; - } - ++first1; - ++first2; - ++types; - } - return first2 == last2; -} - int64_t timestamp_from_string(sstring_view s); struct runtime_exception : public std::exception { diff --git a/utils/lexicographical_compare.hh b/utils/lexicographical_compare.hh new file mode 100644 index 0000000000..148224afe3 --- /dev/null +++ b/utils/lexicographical_compare.hh @@ -0,0 +1,162 @@ +// Copyright 2023-present ScyllaDB +// SPDX-License-Identifier: AGPL-3.0-or-later + +#pragma once + +#include +#include +#include + +// Specifies position in a lexicographically ordered sequence +// relative to some value. +// +// For example, if used with a value "bc" with lexicographical ordering on strings, +// each enum value represents the following positions in an example sequence: +// +// aa +// aaa +// b +// ba +// --> before_all_prefixed +// bc +// --> before_all_strictly_prefixed +// bca +// bcd +// --> after_all_prefixed +// bd +// bda +// c +// ca +// +enum class lexicographical_relation : int8_t { + before_all_prefixed, + before_all_strictly_prefixed, + after_all_prefixed +}; + +// Like std::lexicographical_compare but injects values from shared sequence (types) to the comparator +// Compare is an abstract_type-aware less comparator, which takes the type as first argument. +template +bool lexicographical_compare(TypesIterator types, InputIt1 first1, InputIt1 last1, + InputIt2 first2, InputIt2 last2, Compare comp) { + while (first1 != last1 && first2 != last2) { + if (comp(*types, *first1, *first2)) { + return true; + } + if (comp(*types, *first2, *first1)) { + return false; + } + ++first1; + ++first2; + ++types; + } + return (first1 == last1) && (first2 != last2); +} + +// Like std::lexicographical_compare but injects values from shared sequence +// (types) to the comparator. Compare is an abstract_type-aware trichotomic +// comparator, which takes the type as first argument. +template +requires requires (TypesIterator types, InputIt1 i1, InputIt2 i2, Compare cmp) { + { cmp(*types, *i1, *i2) } -> std::same_as; +} +std::strong_ordering lexicographical_tri_compare(TypesIterator types_first, TypesIterator types_last, + InputIt1 first1, InputIt1 last1, + InputIt2 first2, InputIt2 last2, + Compare comp, + lexicographical_relation relation1 = lexicographical_relation::before_all_strictly_prefixed, + lexicographical_relation relation2 = lexicographical_relation::before_all_strictly_prefixed) { + while (types_first != types_last && first1 != last1 && first2 != last2) { + auto c = comp(*types_first, *first1, *first2); + if (c != 0) { + return c; + } + ++first1; + ++first2; + ++types_first; + } + bool e1 = first1 == last1; + bool e2 = first2 == last2; + if (e1 && e2) { + return static_cast(relation1) <=> static_cast(relation2); + } + if (e2) { + return relation2 == lexicographical_relation::after_all_prefixed ? std::strong_ordering::less : std::strong_ordering::greater; + } else if (e1) { + return relation1 == lexicographical_relation::after_all_prefixed ? std::strong_ordering::greater : std::strong_ordering::less; + } else { + return std::strong_ordering::equal; + } +} + +// Trichotomic version of std::lexicographical_compare() +template +requires requires (InputIt1 i1, InputIt2 i2, Compare c) { + { c(*i1, *i2) } -> std::same_as; +} +std::strong_ordering lexicographical_tri_compare(InputIt1 first1, InputIt1 last1, + InputIt2 first2, InputIt2 last2, + Compare comp, + lexicographical_relation relation1 = lexicographical_relation::before_all_strictly_prefixed, + lexicographical_relation relation2 = lexicographical_relation::before_all_strictly_prefixed) { + while (first1 != last1 && first2 != last2) { + auto c = comp(*first1, *first2); + if (c != 0) { + return c; + } + ++first1; + ++first2; + } + bool e1 = first1 == last1; + bool e2 = first2 == last2; + if (e1 == e2) { + return static_cast(relation1) <=> static_cast(relation2); + } + if (e2) { + return relation2 == lexicographical_relation::after_all_prefixed ? std::strong_ordering::less : std::strong_ordering::greater; + } else { + return relation1 == lexicographical_relation::after_all_prefixed ? std::strong_ordering::greater : std::strong_ordering::less; + } +} + +// A trichotomic comparator for prefix equality total ordering. +// In this ordering, two sequences are equal iff any of them is a prefix +// of the another. Otherwise, lexicographical ordering determines the order. +// +// 'comp' is an abstract_type-aware trichotomic comparator, which takes the +// type as first argument. +// +template +requires requires (TypesIterator ti, InputIt1 i1, InputIt2 i2, Compare c) { + { c(*ti, *i1, *i2) } -> std::same_as; +} +std::strong_ordering prefix_equality_tri_compare(TypesIterator types, InputIt1 first1, InputIt1 last1, + InputIt2 first2, InputIt2 last2, Compare comp) { + while (first1 != last1 && first2 != last2) { + auto c = comp(*types, *first1, *first2); + if (c != 0) { + return c; + } + ++first1; + ++first2; + ++types; + } + return std::strong_ordering::equal; +} + +// Returns true iff the second sequence is a prefix of the first sequence +// Equality is an abstract_type-aware equality checker which takes the type as first argument. +template +bool is_prefixed_by(TypesIterator types, InputIt1 first1, InputIt1 last1, + InputIt2 first2, InputIt2 last2, Equality equality) { + while (first1 != last1 && first2 != last2) { + if (!equality(*types, *first1, *first2)) { + return false; + } + ++first1; + ++first2; + ++types; + } + return first2 == last2; +} + From eaad38c68256c8032ea4129a48f5db89ae08a654 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:24:12 +0200 Subject: [PATCH 14/16] test: raft: avoid confusing string compare gcc doesn't like comparing a C string to an sstring -- apparently it has different promotion rules than clang. Fix by doing an explicit conversion. --- test/raft/raft_server_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/raft/raft_server_test.cc b/test/raft/raft_server_test.cc index ad3afd1628..02aa54c327 100644 --- a/test/raft/raft_server_test.cc +++ b/test/raft/raft_server_test.cc @@ -23,7 +23,7 @@ SEASTAR_THREAD_TEST_CASE(test_check_abort_on_client_api) { cluster.stop_server(0, "test crash").get0(); auto check_error = [](const raft::stopped_error& e) { - return e.what() == sstring("Raft instance is stopped, reason: \"test crash\""); + return sstring(e.what()) == sstring("Raft instance is stopped, reason: \"test crash\""); }; BOOST_CHECK_EXCEPTION(cluster.add_entries(1, 0).get0(), raft::stopped_error, check_error); BOOST_CHECK_EXCEPTION(cluster.get_server(0).modify_config({}, {to_raft_id(0)}).get0(), raft::stopped_error, check_error); From e75009cd4933d4d8a93a4318f0ce766c31806d4e Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:25:26 +0200 Subject: [PATCH 15/16] treewide: catch by reference gcc rightly warns about capturing by value, so capture by reference. --- test/raft/randomized_nemesis_test.cc | 10 +++++----- transport/server.cc | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/raft/randomized_nemesis_test.cc b/test/raft/randomized_nemesis_test.cc index 31342846fc..9a6c8f32d7 100644 --- a/test/raft/randomized_nemesis_test.cc +++ b/test/raft/randomized_nemesis_test.cc @@ -312,15 +312,15 @@ future> call( }).handle_exception([] (std::exception_ptr eptr) { try { std::rethrow_exception(eptr); - } catch (raft::not_a_leader e) { + } catch (raft::not_a_leader& e) { return make_ready_future>(e); - } catch (raft::not_a_member e) { + } catch (raft::not_a_member& e) { return make_ready_future>(e); - } catch (raft::dropped_entry e) { + } catch (raft::dropped_entry& e) { return make_ready_future>(e); - } catch (raft::commit_status_unknown e) { + } catch (raft::commit_status_unknown& e) { return make_ready_future>(e); - } catch (raft::stopped_error e) { + } catch (raft::stopped_error& e) { return make_ready_future>(e); } catch (raft::request_aborted&) { return make_ready_future>(timed_out_error{}); diff --git a/transport/server.cc b/transport/server.cc index fe489053ce..d7883c8e3d 100644 --- a/transport/server.cc +++ b/transport/server.cc @@ -677,7 +677,7 @@ future<> cql_server::connection::process_request() { ? get_units(_server._memory_available, mem_estimate, shedding_timeout).then_wrapped([this, length = f.length] (auto f) { try { return make_ready_future>(f.get0()); - } catch (semaphore_timed_out sto) { + } catch (semaphore_timed_out& sto) { // Cancel shedding in case no more requests are going to do that on completion if (_pending_requests_gate.get_count() == 0) { _shed_incoming_requests = false; From 19810cfc5e21cf94ee21f1ebdb894acc5b834beb Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 19 Mar 2023 19:26:04 +0200 Subject: [PATCH 16/16] transport: correctly format unknown opcode gcc allows an enum to contain values outside its members. For extra safety, as this can be user visible, format the unknown opcode and return it. --- transport/server.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/transport/server.cc b/transport/server.cc index d7883c8e3d..a56084ef6b 100644 --- a/transport/server.cc +++ b/transport/server.cc @@ -125,6 +125,7 @@ sstring to_string(cql_binary_opcode op) { case cql_binary_opcode::AUTH_SUCCESS: return "AUTH_SUCCESS"; case cql_binary_opcode::OPCODES_COUNT: return "OPCODES_COUNT"; } + return format("Unknown CQL binary opcode {}", static_cast(op)); } sstring to_string(const event::status_change::status_type t) {