Skip to content

Conversation

@listout
Copy link

@listout listout commented Nov 9, 2025

What?

Currently UCX is failing to build with newer compiler (for example Clang 21). This PR fixes that.

Why?

Clang 21 has become more strict. It now enables -Werror,-Wdefault-const-init-var-unsafe by default, which results in build failure.

Also std::random_shuffle has been removed since C++17 (deprecated in C++14). PR would make use of recommended std::shuffle and will fallback to std::random_shuffle if needed thus keeping old behavior intact.

How?

Not needed in this case I guess :)

Summary by CodeRabbit

  • Bug Fixes
    • Internal bitmap operations now zero-initialize temporary results to prevent potential undefined behavior.
    • Server randomization updated to use modern C++ shuffling when available, while preserving legacy behavior on older language standards.

First caught on Gentoo LLVM with Clang 21.1.5 and LLVM 21.1.5
Build with fail with the following error message

core/ucp_context.c:2673:47: error: default initialization of an object of type
      'typeof (*tl_bitmap_super)' (aka 'const ucp_tl_bitmap_t') leaves the object uninitialized
      [-Werror,-Wdefault-const-init-var-unsafe]
/var/tmp/portage/sys-cluster/ucx-1.19.0/work/ucx-1.19.0/src/ucs/datastruct/static_bitmap.h:250:5: note:
      expanded from macro 'UCS_STATIC_BITMAP_NOT'
  250 |     UCS_STATIC_BITMAP_UNARY_OP(_bitmap, not, UCS_PP_UNIQUE_ID)
      |     ^
/var/tmp/portage/sys-cluster/ucx-1.19.0/work/ucx-1.19.0/src/ucs/datastruct/static_bitmap.h:239:5: note:
      expanded from macro 'UCS_STATIC_BITMAP_UNARY_OP'
  239 |     _UCS_STATIC_BITMAP_UNARY_OP(_bitmap, _op_name, _uid)
      |     ^
/var/tmp/portage/sys-cluster/ucx-1.19.0/work/ucx-1.19.0/src/ucs/datastruct/static_bitmap.h:232:29: note:
      expanded from macro '_UCS_STATIC_BITMAP_UNARY_OP'
  232 |         ucs_typeof(_bitmap) _r_##_uid; \
      |                             ^
<scratch space>:54:1: note: expanded from here
   54 | _r_0
      | ^
core/ucp_context.c:2672:25: error: default initialization of an object of type
      'typeof (*tl_bitmap)' (aka 'const ucp_tl_bitmap_t') leaves the object uninitialized
      [-Werror,-Wdefault-const-init-var-unsafe]
 2672 |     ucp_tl_bitmap_t b = UCS_STATIC_BITMAP_AND(*tl_bitmap,
      |                         ^

The cause is probably Clang now being more strict and ucs_typeof(_bitmap) _r_##_uid being declared and not used.

Signed-off-by: Brahmajit Das <[email protected]>
@coderabbitai
Copy link

coderabbitai bot commented Nov 9, 2025

Walkthrough

Two bitmap macros in src/ucs/datastruct/static_bitmap.h are changed to zero-initialize their local temporary result variables before use. test/apps/iodemo/io_demo.cc adds conditional compilation to prefer std::shuffle on modern C++ and fall back to std::random_shuffle otherwise.

Changes

Cohort / File(s) Summary
Bitmap macro initialization
src/ucs/datastruct/static_bitmap.h
_UCS_STATIC_BITMAP_UNARY_OP and _UCS_STATIC_BITMAP_BINARY_OP now initialize the local temporary result (_r_##_uid) to {0} before calling the bitmap operation functions. No external APIs changed.
C++17 compatibility: server randomization
test/apps/iodemo/io_demo.cc
Adds #include <random> and conditional compilation to use std::shuffle with std::random_device + std::mt19937 for C++17+, falling back to std::random_shuffle with the existing RNG for older standards when shuffling test_opts.servers.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Small, local macro initialization change (low logic density).
  • Minor conditional compilation and use of standard shuffle APIs.
  • Pay attention to:
    • Macro expansion correctness and any potential side-effects from zero-initialization.
    • Proper compilation on older toolchains and include placement for io_demo.cc.

Poem

🐰
I nudge the bits and tuck them tight,
Zeroed paws before they write.
Shuffle old, shuffle new,
Seeds and hops in morning dew,
A rabbit hums — the build runs right. 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Clang21 build fix' is directly related to the PR's main objective of fixing build failures with Clang 21, but is somewhat generic and could apply to many different fixes without conveying what specific issue was addressed.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d2109e and a950e7a.

📒 Files selected for processing (1)
  • test/apps/iodemo/io_demo.cc (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/apps/iodemo/io_demo.cc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: UCX PR (Codestyle ctags check)
  • GitHub Check: UCX PR (Codestyle AUTHORS file update check)
  • GitHub Check: UCX PR (Codestyle codespell check)
  • GitHub Check: UCX PR (Codestyle commit title)
  • GitHub Check: UCX PR (Codestyle format code)
  • GitHub Check: UCX release DRP (Prepare CheckRelease)
  • GitHub Check: UCX release (Prepare CheckRelease)
  • GitHub Check: UCX snapshot (Prepare Check)

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e29e5f5 and 56456ab.

📒 Files selected for processing (2)
  • src/ucs/datastruct/static_bitmap.h (2 hunks)
  • test/apps/iodemo/io_demo.cc (3 hunks)
🔇 Additional comments (4)
test/apps/iodemo/io_demo.cc (2)

23-23: LGTM!

The <random> header is correctly added to support std::random_device and std::mt19937 used in the C++17+ code path.


3003-3013: Good modernization for C++17 compatibility.

The conditional compilation correctly addresses the removal of std::random_shuffle in C++17. The implementation properly uses std::shuffle with a proper random number generator for C++17+ while maintaining backward compatibility.

src/ucs/datastruct/static_bitmap.h (2)

229-237: LGTM!

Zero-initializing _r_##_uid properly addresses Clang 21's -Wdefault-const-init-var-unsafe warning. The variable is immediately populated by the bitmap operation, so this initialization has no runtime impact beyond satisfying the compiler check.


254-265: LGTM!

Zero-initializing _r_##_uid properly addresses Clang 21's -Wdefault-const-init-var-unsafe warning. The variable is immediately populated by the bitmap binary operation, so this initialization has no runtime impact beyond satisfying the compiler check.

random_shuffle was deprecated in C++14 and completely removed in C++17.
With newer compilers like Clang 21, the build fails. This commit should
preserve older behavior and use std::shuffle when available.

Signed-off-by: Brahmajit Das <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant