|WARNING| eal: fix C++ builds when stdatomics is set
dpdklab at iol.unh.edu
dpdklab at iol.unh.edu
Thu Sep 24 17:06:11 CEST 2026
Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/169993
_AI Code Review_
=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-24
# Review of DPDK Patch: EAL C++ Build Fix for Stdatomics
## Summary
This patch addresses a build failure when DPDK is configured with `RTE_ENABLE_STDATOMIC=true` and C++ code includes DPDK headers. The fix splits the stdatomic implementation into separate C and C++ paths.
---
## ERRORS
None identified. The patch is a build fix that resolves a genuine compatibility issue between C11 `_Atomic` and C++ atomic implementations.
---
## WARNINGS
### 1. Missing Release Notes Entry (Warning)
**Issue:** The patch fixes a significant bug (build failure in C++) but does not update release notes.
**Why it matters:** This is a user-visible bug fix that affects C++ applications using DPDK with stdatomics enabled. Users need to know the issue is resolved.
**Fix:** Add an entry to `doc/guides/rel_notes/release_26_11.rst` (or current release notes file):
```rst
* **Fixed C++ build failures with stdatomic support.**
Resolved compilation errors when building C++ applications with DPDK
configured to use standard C11 atomics (``RTE_ENABLE_STDATOMIC``).
C++ now uses the GCC builtin atomics path while maintaining the same
API surface.
```
### 2. Potentially Confusing Comment Wording (Warning)
**Issue:** The comment "keep the builtin-based implementation for C++ and continue to expose the existing rte_atomic_* API surface" could be clearer about why this is necessary.
**Suggested improvement:**
```c
/* C++ cannot use the C11 _Atomic type specifier that DPDK public headers
* rely on (GCC does not support _Atomic(T) in C++ mode, and C++17 std::atomic
* is incompatible with extern "C" linkage and anonymous unions).
* Use GCC builtin atomics instead, which work identically in C and C++.
*/
```
---
## INFO
### 1. Code Structure (Info)
The three-way split (non-stdatomic / C stdatomic / C++ builtin) is a pragmatic solution. An alternative would be to use C++20/23 atomic features when available, but the current approach of using builtins for C++ is simpler and maintains API compatibility.
### 2. Testing Recommendation (Info)
Ensure the patch is tested with:
- C builds with `RTE_ENABLE_STDATOMIC=true`
- C++ builds with `RTE_ENABLE_STDATOMIC=true` (the failing case this fixes)
- C++ builds with `RTE_ENABLE_STDATOMIC=false` (should be unchanged)
The patch description indicates testing was done (build no longer fails), which is good.
---
## CORRECTNESS VERIFICATION
### Resource Management
No resource allocation/deallocation in this patch
### API Consistency
The C++ path provides identical macro definitions to the non-stdatomic path
All `rte_atomic_*` and `rte_memory_order_*` symbols are defined in both paths
Function signatures match between paths
### Synchronization
No shared state introduced
No race conditions possible (preprocessor-only changes)
### Style Compliance
No trailing whitespace
Consistent indentation (tabs)
No forbidden tokens
Proper header guard structure maintained
---
## OVERALL ASSESSMENT
**Verdict:** The patch is technically correct and solves a real build failure. The only actionable item is adding a release notes entry. The code is well-structured and maintains API compatibility across all build configurations.
**Recommendation:** Accept after adding release notes entry.
More information about the test-report
mailing list