On Wed, 6 May 2026 18:38:43 +0100 Marat Khalili <[email protected]> wrote:
> Document the new gdb-like validation debugger API, outlining how it can > be used to set breakpoints and inspect register states during > validation. > > Highlight its primary use case: writing robust tests for the eBPF > verifier using the harness in app/test/test_bpf_validate.c. > > Signed-off-by: Marat Khalili <[email protected]> > --- Regular AI review doesn't dig deep enough on this; so redid it with stronger model and prompt. Reviewed the series. It cannot be applied as-is for two compounding reasons; once those are resolved, the individual fixes look correct and well-tested. Series-level: the patches reference rte_bpf_prm_ex, enum rte_bpf_origin (RTE_BPF_ORIGIN_RAW), rte_bpf_load_ex, and rte_bpf_get_jit_ex. None of these exist in upstream master, and none are introduced by this 25-patch series — diff context in patch 5 (struct rte_bpf_prm_ex { hunk header) and patch 2 (rte_bpf_get_jit_ex near the insertion point) confirms the prerequisite must already be in the base. Please declare the dependency in a 0/25 cover letter so reviewers can apply on the right base. Patches 03, 05, 10: Build break. RTE_BPF_LOG_FUNC_LINE is used (replacing RTE_BPF_LOG_LINE) but never defined — not in bpf_impl.h, not elsewhere in DPDK, not in any of the 25 patches (verified by grep). Either add the macro to bpf_impl.h, e.g. #define RTE_BPF_LOG_FUNC_LINE(lvl, ...) \ RTE_LOG_LINE_PREFIX(lvl, BPF, "%s(): ", \ __func__ RTE_LOG_COMMA __VA_ARGS__) following the pattern in lib/eal/common/eal_trace.h, or revert the call sites to RTE_BPF_LOG_LINE(... "%s: ...", __func__, ...). Patch 05: __rte_bpf_validate_state_is_valid and __rte_bpf_validate_can_access return int but mix tri-state (true / false / -errno) with bool semantics; the caller does `if (rc == false)` against an int. Works, but consider splitting the bool case from the tri-state case for clarity. Patch 08: Test file uses rte_bpf_load_ex, struct rte_bpf_prm_ex, and RTE_BPF_ORIGIN_RAW (see series-level note). Two minor items in the test helpers: load_constant / compare_and_jump assign int64_t to .imm (int32_t) — fits_in_imm32() guards the value but an explicit (int32_t) cast would document intent; and `value >> 32` on int64_t is implementation-defined for negative values, a (uint64_t) cast before the shift would be portable. Patch 17: RTE_BPF_VALIDATE_DEBUG_EVENT_BRANCH_UNREACHABLE is inserted in the middle of the enum, shifting BRANCH_RETURN and the _END sentinel. Fine for an unreleased experimental API in the same series. Once the API stabilizes, additions should go at the end before the sentinel. Patches 01, 02, 04, 06, 07, 09, 11–16, 18, 19: no comments. The bug descriptions with reproducer programs, expected-vs-actual output, and UBSan diagnostics are very useful and the fixes are well-targeted. I did not get to patches 20–25 in this pass.
