|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