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 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: [email protected]
Signed-off-by: Marat Khalili <[email protected]>
Acked-by: Konstantin Ananyev <[email protected]>
Tested-by: Konstantin Ananyev <[email protected]>
```
---
### 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.
