From 597d0c83e04f0b87cd5db6b18d8675e598303b9e Mon Sep 17 00:00:00 2001 From: Eric Sheng Date: Thu, 27 Feb 2025 22:22:52 -0800 Subject: [PATCH] [#25826] build: Switch builds to clang 19 Summary: Switch most builds to clang 19. AArch64 release builds were left on clang 17 for now since our build workers have just barely not enough memory to do LTO. Code changes were needed to address three new enabled warnings: - VLAs (variable length arrays) are not standard C++, and usage now emits warnings. This warning was disabled. - Returning references to a variable past mutex release, e.g. ``` const std::string& get_string() { std::lock_guard lock(mutex_); return str_; } std::string str_ GUARDED_BY(mutex_); ``` is now a warning, and is also unsafe. These instances were replaced with copies. - The following is now a warning: ``` a->template foo(); ``` and was replaced with the equivalent ``` a->template foo<>(); ``` Jira: DB-15122 Test Plan: Jenkins Reviewers: sergei, asrivastava, #db-approvers Reviewed By: sergei, asrivastava, #db-approvers Subscribers: yql, svc_phabricator, bkolagani, ybase Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D39053 --- CMakeLists.txt | 5 +++++ build-support/common-build-env-test.sh | 10 +++++----- build-support/common-build-env.sh | 3 ++- jenkins_jobs.yml | 8 ++++---- requirements_frozen.txt | 2 +- src/yb/docdb/wait_queue.cc | 4 ++-- src/yb/gutil/stl_util.h | 1 + src/yb/master/clone/clone_state_entity.cc | 6 +++--- src/yb/master/clone/clone_state_entity.h | 6 +++--- src/yb/master/master-test.cc | 2 +- src/yb/rpc/outbound_call.h | 2 +- src/yb/rpc/rpc-test.cc | 2 +- src/yb/rpc/rpc_stub-test.cc | 4 ++-- src/yb/tablet/tablet_metadata.h | 5 ++++- src/yb/util/metric_entity.h | 4 +++- src/yb/util/metrics-test.cc | 10 +++++----- src/yb/util/mt-metrics-test.cc | 2 +- src/yb/util/priority_thread_pool.cc | 2 +- src/yb/util/shared_ptr_tuple.h | 2 +- src/yb/yql/pgwrapper/pg_mini-test.cc | 2 +- 20 files changed, 47 insertions(+), 35 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2944caa5fd48..10c35474938d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -373,6 +373,11 @@ if(IS_CLANG) ADD_CXX_FLAGS("-Wthread-safety-reference") ADD_CXX_FLAGS("-Wthread-safety-precise") ADD_CXX_FLAGS("-Wshorten-64-to-32") + + # Allow variable length arrays in C++. + if("${COMPILER_VERSION}" VERSION_GREATER_EQUAL "18.0.0") + ADD_CXX_FLAGS("-Wno-vla-cxx-extension") + endif() endif() if(USING_LINUXBREW AND IS_CLANG) diff --git a/build-support/common-build-env-test.sh b/build-support/common-build-env-test.sh index 5ca9f56fde5c..5613482ca183 100755 --- a/build-support/common-build-env-test.sh +++ b/build-support/common-build-env-test.sh @@ -179,11 +179,11 @@ test_set_cmake_build_type_and_compiler_type release darwin clang re test_set_cmake_build_type_and_compiler_type release linux-gnu clang release clang 0 test_set_cmake_build_type_and_compiler_type release linux-gnu gcc release gcc 0 test_set_cmake_build_type_and_compiler_type release linux-gnu gcc11 release gcc11 0 -test_set_cmake_build_type_and_compiler_type debug linux-gnu auto debug clang17 0 -test_set_cmake_build_type_and_compiler_type FaStDeBuG linux-gnu auto fastdebug clang17 0 -test_set_cmake_build_type_and_compiler_type release linux-gnu auto release clang17 0 -test_set_cmake_build_type_and_compiler_type tsan linux-gnu auto fastdebug clang17 0 -test_set_cmake_build_type_and_compiler_type asan linux-gnu auto fastdebug clang17 0 +test_set_cmake_build_type_and_compiler_type debug linux-gnu auto debug clang19 0 +test_set_cmake_build_type_and_compiler_type FaStDeBuG linux-gnu auto fastdebug clang19 0 +test_set_cmake_build_type_and_compiler_type release linux-gnu auto release clang19 0 +test_set_cmake_build_type_and_compiler_type tsan linux-gnu auto fastdebug clang19 0 +test_set_cmake_build_type_and_compiler_type asan linux-gnu auto fastdebug clang19 0 # ------------------------------------------------------------------------------------------------- # Test existence of scripts pointed to by specical "script path" variables. diff --git a/build-support/common-build-env.sh b/build-support/common-build-env.sh index 2efc43425387..c65f68bcc47a 100644 --- a/build-support/common-build-env.sh +++ b/build-support/common-build-env.sh @@ -230,6 +230,7 @@ readonly -a VALID_COMPILER_TYPES=( clang16 clang17 clang18 + clang19 ) make_regex_from_list VALID_COMPILER_TYPES "${VALID_COMPILER_TYPES[@]}" @@ -550,7 +551,7 @@ set_default_compiler_type() { YB_COMPILER_TYPE=clang adjust_compiler_type_on_mac elif [[ $OSTYPE =~ ^linux ]]; then - YB_COMPILER_TYPE=clang17 + YB_COMPILER_TYPE=clang19 else fatal "Cannot set default compiler type on OS $OSTYPE" fi diff --git a/jenkins_jobs.yml b/jenkins_jobs.yml index 947a763dec56..e089d720e0a8 100644 --- a/jenkins_jobs.yml +++ b/jenkins_jobs.yml @@ -2,12 +2,12 @@ # Default architecture is x86_64 jobs: - os: alma8 - compiler: clang17 + compiler: clang19 build_type: asan release_artifact: false - os: alma8 - compiler: clang17 + compiler: clang19 build_type: tsan release_artifact: false @@ -18,7 +18,7 @@ jobs: release_artifact: false - os: alma8 - compiler: clang17 + compiler: clang19 build_type: release build_opts: YB_LINKING_TYPE=full-lto release_artifact: true @@ -44,7 +44,7 @@ jobs: release_artifact: true - os: ubuntu22.04 - compiler: clang17 + compiler: clang19 build_type: debug release_artifact: false test_opts: YB_COMPILE_ONLY=1 diff --git a/requirements_frozen.txt b/requirements_frozen.txt index 5f4348755e6a..0f344f3556fb 100644 --- a/requirements_frozen.txt +++ b/requirements_frozen.txt @@ -18,7 +18,7 @@ exceptiongroup==1.2.2 idna==3.10 iniconfig==2.0.0 jmespath==1.0.1 -llvm-installer==1.4.9 +llvm-installer==1.4.10 mypy==1.11.2 mypy-extensions==1.0.0 overrides==7.7.0 diff --git a/src/yb/docdb/wait_queue.cc b/src/yb/docdb/wait_queue.cc index 038f8c7f0ee8..866e6f20853a 100644 --- a/src/yb/docdb/wait_queue.cc +++ b/src/yb/docdb/wait_queue.cc @@ -509,7 +509,7 @@ struct WaitingTxn : public std::enable_shared_from_this { return waiters_; } - const TabletId& GetStatusTablet() EXCLUDES(mutex_) { + TabletId GetStatusTablet() EXCLUDES(mutex_) { SharedLock l(mutex_); return status_tablet_; } @@ -685,7 +685,7 @@ class BlockerData { const TransactionId& id() const { return id_; } - const TabletId& status_tablet() EXCLUDES(mutex_) { + TabletId status_tablet() EXCLUDES(mutex_) { SharedLock r_lock(mutex_); return status_tablet_; } diff --git a/src/yb/gutil/stl_util.h b/src/yb/gutil/stl_util.h index 8f6b19830404..686b68f2feac 100644 --- a/src/yb/gutil/stl_util.h +++ b/src/yb/gutil/stl_util.h @@ -48,6 +48,7 @@ #include #include #include +#include #include #include #include diff --git a/src/yb/master/clone/clone_state_entity.cc b/src/yb/master/clone/clone_state_entity.cc index bfdfde0efd9f..19de17a6432b 100644 --- a/src/yb/master/clone/clone_state_entity.cc +++ b/src/yb/master/clone/clone_state_entity.cc @@ -48,7 +48,7 @@ void CloneStateInfo::SetEpoch(const LeaderEpoch& epoch) { epoch_ = epoch; } -const TxnSnapshotId& CloneStateInfo::SourceSnapshotId() { +TxnSnapshotId CloneStateInfo::SourceSnapshotId() { std::lock_guard l(mutex_); return source_snapshot_id_; } @@ -58,7 +58,7 @@ void CloneStateInfo::SetSourceSnapshotId(const TxnSnapshotId& source_snapshot_id source_snapshot_id_ = source_snapshot_id; } -const TxnSnapshotId& CloneStateInfo::TargetSnapshotId() { +TxnSnapshotId CloneStateInfo::TargetSnapshotId() { std::lock_guard l(mutex_); return target_snapshot_id_; } @@ -68,7 +68,7 @@ void CloneStateInfo::SetTargetSnapshotId(const TxnSnapshotId& target_snapshot_id target_snapshot_id_ = target_snapshot_id; } -const TxnSnapshotRestorationId& CloneStateInfo::RestorationId() { +TxnSnapshotRestorationId CloneStateInfo::RestorationId() { std::lock_guard l(mutex_); return restoration_id_; } diff --git a/src/yb/master/clone/clone_state_entity.h b/src/yb/master/clone/clone_state_entity.h index b8f17aaa69f5..873c38511566 100644 --- a/src/yb/master/clone/clone_state_entity.h +++ b/src/yb/master/clone/clone_state_entity.h @@ -59,13 +59,13 @@ class CloneStateInfo : public MetadataCowWrapper { LeaderEpoch Epoch(); void SetEpoch(const LeaderEpoch& epoch); - const TxnSnapshotId& SourceSnapshotId(); + TxnSnapshotId SourceSnapshotId(); void SetSourceSnapshotId(const TxnSnapshotId& source_snapshot_id); - const TxnSnapshotId& TargetSnapshotId(); + TxnSnapshotId TargetSnapshotId(); void SetTargetSnapshotId(const TxnSnapshotId& target_snapshot_id); - const TxnSnapshotRestorationId& RestorationId(); + TxnSnapshotRestorationId RestorationId(); void SetRestorationId(const TxnSnapshotRestorationId& restoration_id); std::shared_ptr NumTserversWithStaleMetacache(); diff --git a/src/yb/master/master-test.cc b/src/yb/master/master-test.cc index c7a84f6254a6..5556fafc42a7 100644 --- a/src/yb/master/master-test.cc +++ b/src/yb/master/master-test.cc @@ -1057,7 +1057,7 @@ TEST_F(MasterTest, TestCatalogHasBlockCache) { // Check block cache metrics directly and verify // that the counters are greater than 0 - const auto metric_map = mini_master_->master()->metric_entity()->UnsafeMetricsMapForTests(); + const auto metric_map = mini_master_->master()->metric_entity()->TEST_UsageMetricsMap(); scoped_refptr cache_misses_counter = down_cast( FindOrDie(metric_map, diff --git a/src/yb/rpc/outbound_call.h b/src/yb/rpc/outbound_call.h index b0af0e85e113..6a93a3e0f6dd 100644 --- a/src/yb/rpc/outbound_call.h +++ b/src/yb/rpc/outbound_call.h @@ -346,7 +346,7 @@ class OutboundCall : public RpcCall { thread_pool_failure_ = status; } - const Status& thread_pool_failure() const EXCLUDES(mtx_) { + Status thread_pool_failure() const EXCLUDES(mtx_) { std::lock_guard lock(mtx_); return thread_pool_failure_; } diff --git a/src/yb/rpc/rpc-test.cc b/src/yb/rpc/rpc-test.cc index 50da5ff05a37..5284bd5b9e1d 100644 --- a/src/yb/rpc/rpc-test.cc +++ b/src/yb/rpc/rpc-test.cc @@ -671,7 +671,7 @@ TEST_F(TestRpc, TestSendingReceivingMemTrackers) { Result GetMetric( const MetricEntityPtr& metric_entity, const MetricPrototype& prototype) { - const auto& metric_map = metric_entity->UnsafeMetricsMapForTests(); + const auto& metric_map = metric_entity->TEST_UsageMetricsMap(); auto it = metric_map.find(&prototype); if (it == metric_map.end()) { diff --git a/src/yb/rpc/rpc_stub-test.cc b/src/yb/rpc/rpc_stub-test.cc index f93b1ea82174..17409ba62b93 100644 --- a/src/yb/rpc/rpc_stub-test.cc +++ b/src/yb/rpc/rpc_stub-test.cc @@ -859,13 +859,13 @@ TEST_F(RpcStubTest, TrafficMetrics) { } return down_cast(it->second.get()); }; - auto server_metrics = server_messenger()->metric_entity()->UnsafeMetricsMapForTests(); + auto server_metrics = server_messenger()->metric_entity()->TEST_UsageMetricsMap(); auto* service_request_bytes = ASSERT_RESULT(find_metric_by_name( server_metrics, "service_request_bytes_yb_rpc_test_CalculatorService_RepeatedEcho")); auto* service_response_bytes = ASSERT_RESULT(find_metric_by_name( server_metrics, "service_response_bytes_yb_rpc_test_CalculatorService_RepeatedEcho")); - auto client_metrics = client_messenger_->metric_entity()->UnsafeMetricsMapForTests(); + auto client_metrics = client_messenger_->metric_entity()->TEST_UsageMetricsMap(); auto* proxy_request_bytes = ASSERT_RESULT(find_metric_by_name( client_metrics, "proxy_request_bytes_yb_rpc_test_CalculatorService_RepeatedEcho")); auto* proxy_response_bytes = ASSERT_RESULT(find_metric_by_name( diff --git a/src/yb/tablet/tablet_metadata.h b/src/yb/tablet/tablet_metadata.h index 255dc86c3e87..932f7e562246 100644 --- a/src/yb/tablet/tablet_metadata.h +++ b/src/yb/tablet/tablet_metadata.h @@ -410,7 +410,10 @@ class RaftGroupMetadata : public RefCountedThreadSafe, docdb::KeyBounds MakeKeyBounds() const; - const std::string& wal_dir() const { return wal_dir_; } + std::string wal_dir() const { + std::lock_guard lock(data_mutex_); + return wal_dir_; + } Status set_namespace_id(const NamespaceId& namespace_id); diff --git a/src/yb/util/metric_entity.h b/src/yb/util/metric_entity.h index 52fd48d863b0..23a1d8a8c21a 100644 --- a/src/yb/util/metric_entity.h +++ b/src/yb/util/metric_entity.h @@ -171,7 +171,9 @@ class MetricEntity : public RefCountedThreadSafe { Status WriteForPrometheus(PrometheusWriter* writer, const MetricPrometheusOptions& opts); - const MetricMap& UnsafeMetricsMapForTests() const { return metric_map_; } + const MetricMap& TEST_UsageMetricsMap() const NO_THREAD_SAFETY_ANALYSIS { + return metric_map_; + } // Mark that the given metric should never be retired until the metric // registry itself destructs. This is useful for system metrics such as diff --git a/src/yb/util/metrics-test.cc b/src/yb/util/metrics-test.cc index 12d0e9d834bd..059ad41eea57 100644 --- a/src/yb/util/metrics-test.cc +++ b/src/yb/util/metrics-test.cc @@ -535,11 +535,11 @@ TEST_F(MetricsTest, RetirementTest) { const string kMetricName = "foo"; scoped_refptr counter = METRIC_reqs_pending.Instantiate(entity_); - ASSERT_EQ(1, entity_->UnsafeMetricsMapForTests().size()); + ASSERT_EQ(1, entity_->TEST_UsageMetricsMap().size()); // Since we hold a reference to the counter, it should not get retired. entity_->RetireOldMetrics(); - ASSERT_EQ(1, entity_->UnsafeMetricsMapForTests().size()); + ASSERT_EQ(1, entity_->TEST_UsageMetricsMap().size()); // When we de-ref it, it should not get immediately retired, either, because // we keep retirable metrics around for some amount of time. We try retiring @@ -547,14 +547,14 @@ TEST_F(MetricsTest, RetirementTest) { counter = nullptr; for (int i = 0; i < 3; i++) { entity_->RetireOldMetrics(); - ASSERT_EQ(1, entity_->UnsafeMetricsMapForTests().size()); + ASSERT_EQ(1, entity_->TEST_UsageMetricsMap().size()); } // If we wait for longer than the retirement time, and call retire again, we'll // actually retire it. SleepFor(MonoDelta::FromMilliseconds(FLAGS_metrics_retirement_age_ms * 1.5)); entity_->RetireOldMetrics(); - ASSERT_EQ(0, entity_->UnsafeMetricsMapForTests().size()); + ASSERT_EQ(0, entity_->TEST_UsageMetricsMap().size()); } TEST_F(MetricsTest, TestRetiringEntities) { @@ -577,7 +577,7 @@ TEST_F(MetricsTest, NeverRetireTest) { for (int i = 0; i < 3; i++) { entity_->RetireOldMetrics(); - ASSERT_EQ(1, entity_->UnsafeMetricsMapForTests().size()); + ASSERT_EQ(1, entity_->TEST_UsageMetricsMap().size()); } } diff --git a/src/yb/util/mt-metrics-test.cc b/src/yb/util/mt-metrics-test.cc index 3e7d2197a76e..47eb988a2a90 100644 --- a/src/yb/util/mt-metrics-test.cc +++ b/src/yb/util/mt-metrics-test.cc @@ -128,7 +128,7 @@ TEST_F(MultiThreadedMetricsTest, AddCounterToRegistryTest) { int num_counters = 1000; std::function f = std::bind(RegisterCounters, entity, "prefix", num_counters); RunWithManyThreads(&f, num_threads); - ASSERT_EQ(num_threads * num_counters, entity->UnsafeMetricsMapForTests().size()); + ASSERT_EQ(num_threads * num_counters, entity->TEST_UsageMetricsMap().size()); } } // namespace yb diff --git a/src/yb/util/priority_thread_pool.cc b/src/yb/util/priority_thread_pool.cc index 2b1ca9f38c13..a30cb141a8a6 100644 --- a/src/yb/util/priority_thread_pool.cc +++ b/src/yb/util/priority_thread_pool.cc @@ -188,7 +188,7 @@ class PriorityThreadPoolInternalTask { } private: - const std::string& TaskToString() const { + const std::string& TaskToString() const NO_THREAD_SAFETY_ANALYSIS { if (!task_to_string_ready_.load(std::memory_order_acquire)) { std::lock_guard lock(task_to_string_mutex_); if (!task_to_string_ready_.load(std::memory_order_acquire)) { diff --git a/src/yb/util/shared_ptr_tuple.h b/src/yb/util/shared_ptr_tuple.h index 28465bf24a17..f4512e651179 100644 --- a/src/yb/util/shared_ptr_tuple.h +++ b/src/yb/util/shared_ptr_tuple.h @@ -87,7 +87,7 @@ struct SharedPtrTuple : public TypeIndexer { template bool has_type() const { return super::template index_of() >= 0; } - void reset() { super::template reset_tail(&storage_); } + void reset() { super::template reset_tail<>(&storage_); } protected: typedef TypeIndexer super; diff --git a/src/yb/yql/pgwrapper/pg_mini-test.cc b/src/yb/yql/pgwrapper/pg_mini-test.cc index 5fd936c81e7f..c59b9ae80e4e 100644 --- a/src/yb/yql/pgwrapper/pg_mini-test.cc +++ b/src/yb/yql/pgwrapper/pg_mini-test.cc @@ -1671,7 +1671,7 @@ class PgMiniTestAutoScanNextPartitions : public PgMiniTest { template T* GetMetricOpt(const MetricEntity& metric_entity, const MetricPrototype& prototype) { - const auto& map = metric_entity.UnsafeMetricsMapForTests(); + const auto& map = metric_entity.TEST_UsageMetricsMap(); auto it = map.find(&prototype); if (it == map.end()) { return nullptr;