Re: [PATCH] bpf: fix x86 call stack alignment, add tests

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 19 Dec 2025 18:26:23 +0000
Marat Khalili <marat.khalili@huawei.com> 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 <marat.khalili@huawei.com>
> ---

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: stable@dpdk.org | ✅ | 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.