On Fri, 19 Dec 2025 18:26:23 +0000
Marat Khalili <[email protected]> wrote:
> Correctly align stack pointer on x86 JIT if external calls are present.
>
> Add tests for external calls from BPF program demonstrating the problem:
> * direct verification of a local variable alignment;
> * operations with 128-bit integers;
> * aligned and unaligned SSE2 instructions;
> * memcpy and rte_memcpy (may use vector instructions in their code).
>
> (Such variety is needed because not all of these tests are available or
> reproduce the problem on all targets even when the problem exists.)
>
> Fixes: cc752e43e079 ("bpf: add JIT compilation for x86_64 ISA")
>
> Signed-off-by: Marat Khalili <[email protected]>
> ---
AI code review of the test spotted some minor stuff.
## DPDK Patch Review: BPF x86 Call Stack Alignment Fix
### Commit Message
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ | 43 characters |
| Lowercase after colon | ✅ | Correct |
| Imperative mood | ✅ | "fix" is imperative |
| No trailing period | ✅ | Correct |
| **Forbidden punctuation** | ❌ **ERROR** | Subject contains comma (`,`): "fix
x86 call stack alignment**,** add tests" |
| Body wrapped ≤75 chars | ✅ | All lines within limit |
| Body doesn't start with "It" | ✅ | Starts with "Correctly" |
| Signed-off-by present | ✅ | Real name and email |
| Fixes: tag format | ✅ | 12-char SHA, quoted subject |
| Cc: [email protected] | ✅ | Present in headers for backport |
| Tag order | ✅ | Blank line between Fixes: and Signed-off-by |
**Suggested subject fix:**
```
bpf: fix x86 call stack alignment and add tests
```
or split into two patches.
---
### Code Quality
**JIT Fix (`lib/bpf/bpf_jit_x86.c`):**
| Check | Status | Notes |
|-------|--------|-------|
| Lines ≤100 chars | ✅ | All within limit |
| Comment style | ✅ | Proper `/* */` multi-line format |
| Logic correctness | ✅ | Stack alignment via `AND RSP, -alignof(max_align_t)`
is correct |
| Side effect: blank line removal | ⚠️ **INFO** | Line 1217 removes unrelated
blank line - minor cleanup mixed with fix |
The alignment approach is sound: using `-(uint32_t)alignof(max_align_t)`
creates mask `0xFFFFFFF0` for 16-byte alignment.
**Tests (`app/test/test_bpf.c`):**
| Check | Status | Notes |
|-------|--------|-------|
| Test registration macro | ✅ | Uses `REGISTER_FAST_TEST` (correct per
guidelines) |
| NULL comparisons | ✅ | Uses `== NULL` explicitly |
| Format specifiers | ✅ | Uses `%ju`/`%zu` correctly with casts |
| **Missing NULL check** | ⚠️ **WARNING** | Lines 410, 414: `malloc()` return
not checked |
| Conditional compilation | ✅ | `__SIZEOF_INT128__`, `__SSE2__` guards
appropriate |
---
### Specific Code Observations
**Minor warning - malloc without NULL check:**
```c
char *const src_buffer = malloc(size + src_offset); // line 410
// ... no NULL check
char *const dst_buffer = malloc(size + dst_offset); // line 414
// ... no NULL check
```
While test code is more tolerant, adding a check would be cleaner:
```c
if (src_buffer == NULL || dst_buffer == NULL) {
free(src_buffer);
return TEST_FAILED;
}
```
**Good practices observed:**
- Thorough test coverage: direct alignment verification, uint128, SSE2
aligned/unaligned, memcpy variants
- Proper volatile usage to prevent optimization interfering with tests
- Good inline documentation explaining the ABI requirement and fix rationale
- Appropriate use of `alignof(max_align_t)` rather than hardcoding 16
---
### Summary
| Severity | Count | Items |
|----------|-------|-------|
| **Error** | 1 | Subject contains forbidden comma |
| **Warning** | 1 | malloc() without NULL checks in test |
| **Info** | 1 | Unrelated blank line removal mixed with fix |
**Recommendation:** Request v3 with subject line fixed. The technical content
is solid - the fix correctly addresses the x86-64 ABI stack alignment
requirement for external calls, and the tests comprehensively validate the fix
across multiple scenarios.
