|WARNING| [v2] eal: fix C++ builds when stdatomics is set
dpdklab at iol.unh.edu
dpdklab at iol.unh.edu
Fri Sep 25 15:38:09 CEST 2026
Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/170037
_AI Code Review_
=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-25
# DPDK Patch Review
## Summary
This patch fixes C++ build failures when DPDK is configured to use stdatomic by providing a separate C++ implementation path that uses GCC builtins instead of C11 _Atomic types.
---
## Errors
### 1. C++ build testing not enabled in CI for stdatomic mode (Build Configuration)
**File:** `.ci/linux-build.sh`
The patch moves `check_includes=true` outside the `if [ "$STDATOMIC" = "true" ]` block,
which enables C++ header checks for stdatomic builds.
However, the logic sets `CXX` based on the C compiler choice (`CC`) regardless of `STDATOMIC`.
This means C++ testing is now enabled for stdatomic builds, but the condition should be explicit.
**Current code:**
```bash
if [ "$STDATOMIC" = "true" ]; then
OPTS="$OPTS -Denable_stdatomic=true"
fi
OPTS="$OPTS -Dcheck_includes=true"
if [ "${CC%%clang}" != "$CC" ]; then
export CXX=clang++
else
export CXX=g++
fi
```
**Issue:** The logic is correct but could be clearer. The change enables C++ checks for stdatomic builds,
which is the intended fix. The CXX setting is appropriate (matches CC's compiler family).
Actually, reviewing this more carefully: the original code had `check_includes=true` only when `STDATOMIC` was false.
The patch now enables it unconditionally, which is the correct fix for testing C++ with stdatomic.
The CXX logic was already there but unreachable in stdatomic builds; now it's always reached.
**This is actually correct** - the patch properly enables C++ header checking for stdatomic builds.
No issue here.
---
## Warnings
None.
---
## Info
### 1. Documentation of C++ stdatomic limitations (Documentation)
**File:** `lib/eal/include/rte_stdatomic.h`
The comment explaining why C++ uses builtins is clear and helpful:
```c
/* C++ cannot use the C11 _Atomic member layout that DPDK public headers
* rely on, so keep the builtin-based implementation for C++ and continue
* to expose the existing rte_atomic_* API surface.
*/
```
**Suggestion:** Consider adding a note about when this workaround can be removed (e.g., "This workaround
is needed until DPDK can require C++23 where std::atomic provides compatible semantics").
### 2. Potential for future simplification
The patch creates three code paths:
1. C with stdatomic (new, working)
2. C++ with builtins (new workaround)
3. Fallback to builtins (existing)
Paths 2 and 3 are nearly identical. When DPDK drops support for compilers without stdatomic,
the C++ path could potentially be unified with the fallback path or modernized to use C++11 std::atomic directly.
This is not a problem with the current patch, just a note for future refactoring.
---
## Conclusion
**The patch is correct and ready for merge.**
The solution properly addresses the C++ build failure by providing a separate implementation path
for C++ that uses GCC builtins (which work in C++ mode) instead of C11 _Atomic types (which don't map cleanly to C++17).
The CI script change correctly enables C++ header checking for stdatomic builds, which was previously disabled.
No errors found. The code follows DPDK style, has appropriate comments explaining the workaround,
and the three-way split (C stdatomic / C++ builtins / legacy builtins) is a pragmatic solution
to the C vs C++ atomics compatibility issue.
More information about the test-report
mailing list