bazel: upgrade to clang-19 and fix sanitizers #25120

rockwotj · 2025-02-20T01:11:22Z

bazel: only enable batch cache test if using seastar allocator
rpc: don't move service::_methods
datalake/translation: include what you use
iobuf/fuzz: include what you use
ct: make pipeline stage id move assignable
bazel: suppress leaks in openssl-fips
bazel: upgrade to clang-19
datalake: make benchmark test fast

Backports Required

Release Notes

none

dotnwat · 2025-02-20T01:56:34Z

src/v/storage/tests/BUILD

-        "@seastar//:use_system_allocator": [],
-        "//conditions:default": ["@platforms//:incompatible"],
+        "@seastar//:use_system_allocator": ["@platforms//:incompatible"],
+        "//conditions:default": [],


huh, i wonder why it was working before... that's concerning

I think the default build uses the seastar allocator still, so it was just always disabled

dotnwat · 2025-02-20T01:59:12Z

src/v/rpc/rpc_compiler.py

    {{service_name}}_service_base({{service_name}}_service_base&& o) noexcept
-      : _sc(std::move(o._sc)), _ssg(std::move(o._ssg)), _methods(std::move(o._methods)) {}
+      : _sc(std::move(o._sc)), _ssg(std::move(o._ssg)) {}


We must never move these RPC services because this would be very
buggy otherwise.

was the intention to instead delete the move consturctor?

I'm game for deleting both the move constructor and move assignment operator - I don't think either are sound for _methods. I do think we don't need to move the methods and it's fine with this change.

Ah what I was trying to say was, today in the code base there is no way we in practice move any services today, because there would be mass bugs with rpcs invoking the wrong object. Maybe we should formalize this and delete the move ctor and assignment operator

Maybe we should formalize this and delete the move ctor and assignment operator

Yes.

Pushed a commit to remove them.

travisdowns · 2025-02-20T15:33:23Z

MODULE.bazel

-        "linux-x86_64": ["http://redpanda-core-toolchain.s3-website-us-east-1.amazonaws.com/llvm-19.1.1-ubuntu-22.04-x86_64.tar.gz"],
-    },
+    llvm_version = "19.1.7",
+    # Currently our custom build of clang causes false positives with sanitizers, so we use the default clang distribution.


Huh, bummer. I guess something about the way we build it causes this.

Yeah exactly, I'm not sure what. We may have to go back to custom builds and figure out what flag is calling this, because in CI we need older libraries, but locally I need newer ones

michael-redpanda · 2025-02-20T15:54:18Z

lsan_suppressions.txt

@@ -1 +1,2 @@
 leak:seastar::net::conntrack::handle::~handle
+leak:fips/self_test.c


Just a note: self tests are run once per shard and, unfortunately, are within the validated section of the OpenSSL FIPS module which means we cannot touch it and maintain validation.

michael-redpanda · 2025-02-20T16:00:51Z

It looks like CI cpp lint (which I assume uses clang-format-18) passes against the changes in 35318ed. Do we need to ensure that core knows to use clang-format in bazel rather than in vtools? Should we remove task lint:cpp in vtools to ensure it isn't accidentally used and change CI to use bazel?

rockwotj · 2025-02-20T16:27:20Z

Should we remove task lint:cpp in vtools to ensure it isn't accidentally used and change CI to use bazel?

Yeah people should use bazel run //tools:clang_format now

nvartolomei · 2025-02-20T16:45:48Z

src/v/storage/tests/BUILD

@@ -240,8 +240,8 @@ redpanda_cc_btest(
        "batch_cache_reclaim_test.cc",
    ],
    target_compatible_with = select({
-        "@seastar//:use_system_allocator": [],
-        "//conditions:default": ["@platforms//:incompatible"],
+        "@seastar//:use_system_allocator": ["@platforms//:incompatible"],


please add a WHY to the commit messages 🙇‍♂️

michael-redpanda · 2025-02-20T18:29:58Z

Should we remove task lint:cpp in vtools to ensure it isn't accidentally used and change CI to use bazel?

Yeah people should use bazel run //tools:clang_format now

Should apply as well to buildkite?

dotnwat · 2025-02-20T19:12:41Z

Should apply as well to buildkite?

yeh i think it runs as a github action?

rockwotj · 2025-02-20T19:14:31Z

Should apply as well to buildkite?

yeh i think it runs as a github action?

redpanda/.github/workflows/lint-cpp.yml

Line 38 in ceae5b0

bazel run --lockfile_mode=off //tools:clang_format

This was incorrectly configured, the test relies on the seastar allocator's reclaim ability, which is required to pass. As is this always is disabled on CI but we only want to disable the test if the system allocator is being used because the reclaim ability is not available.

_methods is an array of non_copyable functions that have `[this]` pointers captured, we cannot move them into a new object because the `this` pointer become invalidated and pointing to the old object. We must never move these RPC services because this would be very buggy otherwise.

clang 19 is more strict on this

clang 19 is more strict here

libc++ 19 is more strict, but it requires types are copy assignable, and according to the standard copy assignable types must be move asssignable as well. https://en.cppreference.com/w/cpp/named_req/CopyAssignable

In getting the sanitizers work in openssl tests with fips enabled we need to suppress leaks there. The benchmarks are full of them, and I assume these are not caught in CMake because benchmarks don't run with sanitizers.

Upgrade to the latest clang. The real motivation here is that we want the sanitizers to work, however for some reason the custom built clang we use has false positives with coroutines and the sanitizers. However clang-18 is compiled against a super old version of ubuntu and missing libraries are missing (namely `libtinfo.5.so`) and not available on recent ubuntu and fedora distributions. Since we're upgrading the clang-19 we need to address a few deprecations in libc++, so we upgrade seastar for this.

In bazel we test benchmarks by running them once, well 1000 batches is really slow in debug mode, so running once doesn't do anything. Instead change the number of batches in debug mode to make the benchmark fast.

This isn't really safe because pointers are taken to `this`, we don't use these move operators anyways and they are a big footgun, so just remove them.

vbotbuildovich · 2025-02-20T23:40:31Z

CI test results

test results on build#62070

test_id	test_kind	job_url	test_status	passed
rptest.tests.partition_movement_test.SIPartitionMovementTest.test_shadow_indexing.num_to_upgrade=0.cloud_storage_type=CloudStorageType.ABS	ducktape	https://buildkite.com/redpanda/redpanda/builds/62070#0195252c-680c-4d5e-a96c-890949790246	FLAKY	1/2
rptest.tests.random_node_operations_test.RandomNodeOperationsTest.test_node_operations.enable_failures=True.mixed_versions=False.with_tiered_storage=False.with_iceberg=True.with_chunked_compaction=True.cloud_storage_type=CloudStorageType.S3	ducktape	https://buildkite.com/redpanda/redpanda/builds/62070#0195252c-680b-45fd-98e5-6ef8af855866	FLAKY	1/2
rptest.tests.scaling_up_test.ScalingUpTest.test_scaling_up_with_recovered_topic	ducktape	https://buildkite.com/redpanda/redpanda/builds/62070#0195252c-680c-4d5e-a96c-890949790246	FLAKY	1/2

github-actions bot added area/build area/redpanda area/wasm WASM Data Transforms labels Feb 20, 2025

dotnwat reviewed Feb 20, 2025

View reviewed changes

rockwotj force-pushed the clang-fixes-bazel branch from 7e3c785 to 64949d6 Compare February 20, 2025 14:55

rockwotj marked this pull request as ready for review February 20, 2025 14:56

rockwotj force-pushed the clang-fixes-bazel branch from 64949d6 to 35318ed Compare February 20, 2025 15:02

rockwotj enabled auto-merge February 20, 2025 15:03

rockwotj requested review from dotnwat, travisdowns, mmaslankaprv and michael-redpanda February 20, 2025 15:04

travisdowns reviewed Feb 20, 2025

View reviewed changes

michael-redpanda reviewed Feb 20, 2025

View reviewed changes

nvartolomei reviewed Feb 20, 2025

View reviewed changes

rockwotj added 10 commits February 20, 2025 19:16

datalake/translation: include what you use

27e5d13

clang 19 is more strict on this

iobuf/fuzz: include what you use

8a92543

clang 19 is more strict here

ct: make pipeline stage id move assignable

02fec8d

libc++ 19 is more strict, but it requires types are copy assignable, and according to the standard copy assignable types must be move asssignable as well. https://en.cppreference.com/w/cpp/named_req/CopyAssignable

bazel: suppress leaks in openssl-fips

4d2c666

In getting the sanitizers work in openssl tests with fips enabled we need to suppress leaks there. The benchmarks are full of them, and I assume these are not caught in CMake because benchmarks don't run with sanitizers.

datalake: make benchmark test fast

7b6e173

In bazel we test benchmarks by running them once, well 1000 batches is really slow in debug mode, so running once doesn't do anything. Instead change the number of batches in debug mode to make the benchmark fast.

treewide: format the codebase with clang-format-19

d4bf788

rpc/service: remove move ctor/operator

6687a50

This isn't really safe because pointers are taken to `this`, we don't use these move operators anyways and they are a big footgun, so just remove them.

bazel: add zlib back to clang builds

082c967

rockwotj force-pushed the clang-fixes-bazel branch from 44213ea to 082c967 Compare February 20, 2025 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bazel: upgrade to clang-19 and fix sanitizers #25120

bazel: upgrade to clang-19 and fix sanitizers #25120

rockwotj commented Feb 20, 2025

dotnwat Feb 20, 2025

rockwotj Feb 20, 2025

dotnwat Feb 20, 2025

rockwotj Feb 20, 2025

rockwotj Feb 20, 2025

travisdowns Feb 20, 2025

rockwotj Feb 20, 2025

travisdowns Feb 20, 2025

rockwotj Feb 20, 2025

michael-redpanda Feb 20, 2025

michael-redpanda commented Feb 20, 2025

rockwotj commented Feb 20, 2025

nvartolomei Feb 20, 2025

rockwotj Feb 20, 2025

michael-redpanda commented Feb 20, 2025

dotnwat commented Feb 20, 2025

rockwotj commented Feb 20, 2025

vbotbuildovich commented Feb 20, 2025

		@@ -1 +1,2 @@
		leak:seastar::net::conntrack::handle::~handle
		leak:fips/self_test.c

bazel: upgrade to clang-19 and fix sanitizers #25120

Are you sure you want to change the base?

bazel: upgrade to clang-19 and fix sanitizers #25120

Conversation

rockwotj commented Feb 20, 2025

Backports Required

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Feb 20, 2025

rockwotj commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Feb 20, 2025

dotnwat commented Feb 20, 2025

rockwotj commented Feb 20, 2025

vbotbuildovich commented Feb 20, 2025

CI test results