[#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

@@ -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)

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.

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

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

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

src/yb/docdb/wait_queue.cc

@@ -509,7 +509,7 @@ struct WaitingTxn : public std::enable_shared_from_this<WaitingTxn> {
     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_;
   }

src/yb/gutil/stl_util.h

@@ -48,6 +48,7 @@
 #include <cassert>
 #include <deque>
 #include <functional>
+#include <iterator>
 #include <optional>
 #include <set>
 #include <string>

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_;
 }

src/yb/master/clone/clone_state_entity.h

@@ -59,13 +59,13 @@ class CloneStateInfo : public MetadataCowWrapper<PersistentCloneStateInfo> {
   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<CountDownLatch> NumTserversWithStaleMetacache();

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<Counter> cache_misses_counter = down_cast<Counter *>(
       FindOrDie(metric_map,

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_;
   }

src/yb/rpc/rpc-test.cc

@@ -671,7 +671,7 @@ TEST_F(TestRpc, TestSendingReceivingMemTrackers) {
 
 Result<MetricPtr> 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()) {

src/yb/rpc/rpc_stub-test.cc

@@ -859,13 +859,13 @@ TEST_F(RpcStubTest, TrafficMetrics) {
     }
     return down_cast<Counter*>(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(

src/yb/tablet/tablet_metadata.h

@@ -410,7 +410,10 @@ class RaftGroupMetadata : public RefCountedThreadSafe<RaftGroupMetadata>,
 
   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);
 

src/yb/util/metric_entity.h

@@ -171,7 +171,9 @@ class MetricEntity : public RefCountedThreadSafe<MetricEntity> {
   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

src/yb/util/metrics-test.cc

@@ -535,26 +535,26 @@ TEST_F(MetricsTest, RetirementTest) {
 
   const string kMetricName = "foo";
   scoped_refptr<Counter> 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
   // a number of times to hit all the cases.
   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());
   }
 }
 

src/yb/util/mt-metrics-test.cc

@@ -128,7 +128,7 @@ TEST_F(MultiThreadedMetricsTest, AddCounterToRegistryTest) {
   int num_counters = 1000;
   std::function<void()> 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

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)) {

src/yb/util/shared_ptr_tuple.h

@@ -87,7 +87,7 @@ struct SharedPtrTuple : public TypeIndexer<Types ...> {
   template <class Type>
   bool has_type() const { return super::template index_of<Type>() >= 0; }
 
-  void reset() { super::template reset_tail(&storage_); }
+  void reset() { super::template reset_tail<>(&storage_); }
 
  protected:
   typedef TypeIndexer<Types ...> super;

src/yb/yql/pgwrapper/pg_mini-test.cc

@@ -1671,7 +1671,7 @@ class PgMiniTestAutoScanNextPartitions : public PgMiniTest {
 
 template <class T>
 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;