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 identifies some shadow declarations in this patch.
Please fix and resubmit.

## DPDK Patch Review: BPF x86 Call Stack Alignment Fix

### Summary
This patch fixes a real x86-64 ABI compliance bug in the BPF JIT compiler where stack alignment wasn't guaranteed to be 16-byte aligned before external function calls. The fix is elegant and the test coverage is comprehensive.

---

### Commit Message Issues

| Severity | Issue | Details |
|----------|-------|---------|
| **Error** | Comma in subject line | Subject contains `,` which is a forbidden punctuation mark per `check-git-log.sh` |
| **Error** | Missing blank line before Signed-off-by | Per tag order rules, blank line required between `Fixes:`/`Cc:` group and `Signed-off-by:` group |

**Subject line:**
```
bpf: fix x86 call stack alignment, add tests
```
Should be split into two patches, or reworded:
```
bpf: fix x86 call stack alignment for external calls
```
(with tests mentioned in body, or as separate patch)

**Tag ordering should be:**
```
Fixes: cc752e43e079 ("bpf: add JIT compilation for x86_64 ISA")
Cc: stable@dpdk.org

Signed-off-by: Marat Khalili <marat.khalili@huawei.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Tested-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
```

---

### Code Review

**lib/bpf/bpf_jit_x86.c** — The fix is clean and well-documented:

```c
/* Mark RBP as used to trigger stack realignment in prolog. */
USED(st->reguse, RBP);
```

The alignment approach using `alignof(max_align_t)` is portable and correct. The comment block explaining the rationale is helpful.

**Minor observation:** Line 1219 removes an extra blank line — this is unrelated cleanup but acceptable.

**app/test/test_bpf.c** — Comprehensive test coverage:

| Severity | Issue | Location |
|----------|-------|----------|
| **Warning** | Variable shadowing | Lines 462-464: `src_offset`, `dst_offset`, `test_rc` redeclared in inner loop scope |
| **Info** | Style preference | Consider extracting the common pattern in memcpy subtests |

The shadowed variables at lines 462-464:
```c
for (size_t size = 1; size <= 1024; size <<= 1) {
    const bool src_offset = offsets & 1;  /* shadows outer scope */
    const bool dst_offset = offsets & 2;  /* shadows outer scope */
    int test_rc;                           /* shadows outer scope */
```

These are unnecessary redeclarations since the outer loop variables can be reused.

---

### Technical Assessment

The fix correctly addresses the x86-64 ABI requirement (System V AMD64 ABI §3.2.2) that RSP must be 16-byte aligned before `CALL`. The approach of:

1. Marking RBP as used when external calls exist → triggers stack frame setup
2. Aligning RSP with `AND` instruction using `-alignof(max_align_t)` 
3. Restoring original RSP from RBP in epilog

is sound and minimal in its impact on generated code for programs without external calls.

The test coverage is thorough, testing:
- Direct stack variable alignment verification
- 128-bit integer operations
- SSE2 aligned/unaligned loads/stores  
- memcpy/rte_memcpy with various sizes and alignments

---

### Verdict

**Recommended changes before merge:**

1. **Fix subject line** — Remove comma or split into two patches
2. **Add blank line** before `Signed-off-by:` block
3. **Consider** removing shadowed variable declarations in test (minor)

The technical fix itself is correct and well-implemented. With commit message fixes, this should be ready to merge.