fix alignment different between c and c++ in gtest by cjx-zar · Pull Request #3331 · valkey-io/valkey

cjx-zar · 2026-03-09T07:49:04Z

I found that on 32-bit platforms, the gtest fails if I add a declaration int aof_rewrite_for_replication; between two atomic 64-bit fields in the valkeyServer struct.

In C, the _Atomic modifier makes the type aligned to its own size, but for the C++ unit tests, we're stripping the _Atomic qualifier in src/unit/wrappers.h by defining it to empty. This makes struct valkeyServer have different alignment/field offsets in C-compiled code (libvalkey.a) vs C++-compiled tests (gtest). As a result, fields after these atomics are accessed at different offsets, leading to corrupted reads/writes and a segfault.

This commit changes _Atomic to always use parentheses, as in _Atomic(int) instead of _Atomic int, and for C++ defines it to be aligned in the same way as in C:

#define _Atomic(type) alignas(sizeof(type)) type

Signed-off-by: cjx-zar <jxchenczar@foxmail.com>

codecov · 2026-03-09T08:08:59Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.97%. Comparing base (8c397ea) to head (cfb02c4).
⚠️ Report is 9 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3331      +/-   ##
============================================
- Coverage     74.98%   74.97%   -0.02%     
============================================
  Files           129      129              
  Lines         71551    71632      +81     
============================================
+ Hits          53654    53705      +51     
- Misses        17897    17927      +30

Files with missing lines	Coverage Δ
src/server.h	`100.00% <ø> (ø)`

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

zuiderkwast · 2026-03-09T09:53:20Z

TBH, I don't like very much to replace _Atomic with VALKEY_ATOMIC. It's an extra indirection, just to workaround the fact that we're now using a different language for the unit tests.

Is it possible #define _Atomic in some way in the C++ tests to make this work as expected?

Or use aliases like atomic_int that exist in C11 (https://en.cppreference.com/w/c/header/stdatomic.html)

Edit: Found this discussion about stdatomic.h with C++: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60932. The comment at the bottom:

Jakub Jelinek 2021-08-11 09:58:13 UTC
Note, stdatomic.h is now in C++20.

So, let's bump to C++20? Unit tests are only for developers, not for users who just want to build Valkey. For that, you only need C11.

zuiderkwast · 2026-03-09T10:29:57Z

Another option is that we use the style _Atomic with parentheses, e.g. _Atomic(int) instead of _Atomic int. This is standardized too here: https://cppreference.net/c/language/atomic.html.

If we do that, then we'll be able to define for the C++ tests:

#define _Atomic(type) alignas(sizeof(type)) type

If some C code uses _Atomic without parentheses, we'll catch it because the C++ tests will fail to compile.

Signed-off-by: cjx-zar <jxchenczar@foxmail.com>

cjx-zar · 2026-03-09T12:45:10Z

@zuiderkwast Thanks for the suggestions! I agree that VALKEY_ATOMIC adds unnecessary indirection. I've adopted your second approach:

Changed all _Atomic usages in server.h to use the parenthesized form _Atomic(type)
Updated wrappers.h to define _Atomic(type) as alignas(sizeof(type)) type instead of stripping it entirely.

zuiderkwast

Yeah, this looks better to me. :)

Signed-off-by: cjx-zar <jxchenczar@foxmail.com>

sarthakaggarwal97

Thanks!

JimB123 · 2026-03-10T13:36:46Z

@cjx-zar Going back to the original issue... are you saying that on a 32-bit system, a uint64_t is only given 4-byte alignment? Or is the _Atomic adding an additional hidden field which then allows the new int to be inserted without affecting future field alignment?

Can you provide a bit more information about the before/after situation?

EDIT: I may have answered by own question. It seems that long long and double are only given 4-byte alignment on 32-bit systems. (I didn't know this!) With the _Atomic qualifier, they are 8-byte aligned.

JimB123

Nice find. I would have never guessed that _Atomic alters the alignment.

zuiderkwast · 2026-03-10T14:20:08Z

Oops, I was just editing the top comment before merging it. The top comment is used as the commit message when we're squash-merging, so it's good to make sure it matches what's actually being committed. Now the commit message mentions the wrong macro name, which was used in this PR before the review comments and updates.

I found that on 32-bit platforms, the gtest fails if I add a declaration `int aof_rewrite_for_replication;` between https://github.com/valkey-io/valkey/blob/fc217fca2d7de246ab8494d9d8619b7348235f47/src/server.h#L1843 and https://github.com/valkey-io/valkey/blob/fc217fca2d7de246ab8494d9d8619b7348235f47/src/server.h#L2108 The crash is caused by https://github.com/valkey-io/valkey/blob/fc217fca2d7de246ab8494d9d8619b7348235f47/src/unit/wrappers.h#L40 stripping the _Atomic qualifier in C++ builds. This makes struct valkeyServer have different alignment/field offsets in C-compiled code (libvalkey.a) vs C++-compiled tests (gtest). As a result, fields after these atomics are accessed at different offsets, leading to corrupted reads/writes and a segfault. I added VALKEY_ATOMIC, and it works as follows: - C builds (production): VALKEY_ATOMIC(uint64_t) -> _Atomic uint64_t (unchanged behavior) - C++ builds (tests via wrappers.h): VALKEY_ATOMIC(uint64_t) -> alignas(8) uint64_t (preserves 8-byte alignment) - All existing atomic_load_explicit / atomic_store_explicit calls in C code continue to work because _Atomic is present in C mode - C++ test code only does direct field access (never uses atomic operations), so alignas(N) type is sufficient --------- Signed-off-by: cjx-zar <jxchenczar@foxmail.com>

fix alignment different between c and c++ in gtest

4a6e3df

Signed-off-by: cjx-zar <jxchenczar@foxmail.com>

github-actions Bot assigned cjx-zar Mar 9, 2026

cjx-zar mentioned this pull request Mar 9, 2026

Support using base aof for full synchronization #3169

Open

madolson requested a review from JimB123 March 9, 2026 09:49

zuiderkwast mentioned this pull request Mar 9, 2026

Redesign IO threading communication model #3324

Merged

revert VALKEY_ATOMIC, change to _Atomic(type)

3018807

Signed-off-by: cjx-zar <jxchenczar@foxmail.com>

zuiderkwast approved these changes Mar 9, 2026

View reviewed changes

Comment thread src/server.h Outdated

cjx-zar requested a review from zuiderkwast March 9, 2026 15:09

format

cfb02c4

Signed-off-by: cjx-zar <jxchenczar@foxmail.com>

sarthakaggarwal97 approved these changes Mar 9, 2026

View reviewed changes

zuiderkwast approved these changes Mar 9, 2026

View reviewed changes

JimB123 approved these changes Mar 10, 2026

View reviewed changes

JimB123 merged commit 1e2ca5e into valkey-io:unstable Mar 10, 2026
58 checks passed

asagege mentioned this pull request Mar 16, 2026

Allocate metadata for each key to support forkless save operations (forkless) #3350

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix alignment different between c and c++ in gtest#3331

fix alignment different between c and c++ in gtest#3331
JimB123 merged 3 commits into
valkey-io:unstablefrom
cjx-zar:fix-alignment-32bit

cjx-zar commented Mar 9, 2026 •

edited by zuiderkwast

Loading

Uh oh!

codecov Bot commented Mar 9, 2026 •

edited

Loading

Uh oh!

zuiderkwast commented Mar 9, 2026 •

edited

Loading

Uh oh!

zuiderkwast commented Mar 9, 2026 •

edited

Loading

Uh oh!

cjx-zar commented Mar 9, 2026

Uh oh!

zuiderkwast left a comment

Uh oh!

Uh oh!

sarthakaggarwal97 left a comment

Uh oh!

JimB123 commented Mar 10, 2026 •

edited

Loading

Uh oh!

JimB123 left a comment

Uh oh!

Uh oh!

zuiderkwast commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

cjx-zar commented Mar 9, 2026 • edited by zuiderkwast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zuiderkwast commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjx-zar commented Mar 9, 2026

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

Uh oh!

JimB123 commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimB123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zuiderkwast commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cjx-zar commented Mar 9, 2026 •

edited by zuiderkwast

Loading

codecov Bot commented Mar 9, 2026 •

edited

Loading

zuiderkwast commented Mar 9, 2026 •

edited

Loading

zuiderkwast commented Mar 9, 2026 •

edited

Loading

JimB123 commented Mar 10, 2026 •

edited

Loading