Add EbpfDomain Catch2 test suite #912
Conversation
Signed-off-by: Prevail Contributor <prevail@example.com>
WalkthroughAdds a new Catch2 test suite for EbpfDomain, introduces a friend test-access class, and wires the test into the CMake build gated by VERIFIER_ENABLE_TESTS. No functional changes to production logic beyond the friend declaration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request Test Coverage Report for Build 17777152218Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
CMakeLists.txt(1 hunks)src/crab/ebpf_domain.hpp(1 hunks)src/test/test_ebpf_domain.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
CMakeLists.txt
📄 CodeRabbit inference engine (AGENTS.md)
Maintain C++20 build configuration for the core verifier (e.g., CMAKE_CXX_STANDARD 20) in the top-level CMakeLists.txt
Files:
CMakeLists.txt
**/*.{c,cc,cpp,h,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,cc,cpp,h,hpp}: Run the project formatter (./scripts/format-code --staged) before committing to ensure consistent clang-format styling
Ensure new C/C++ sources include the standard SPDX license header (validate via ./scripts/check-license.sh)
Files:
src/crab/ebpf_domain.hppsrc/test/test_ebpf_domain.cpp
**/*.{h,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
Headers should prefer #pragma once for include guards; follow existing patterns within each subdirectory
Files:
src/crab/ebpf_domain.hpp
src/{asm_*,cfg,arith,crab,linux}/**/*.{cc,cpp,h,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
Add new verifier logic under the matching subsystem directory (asm_*, cfg, arith, crab, linux) to keep separation of concerns
Files:
src/crab/ebpf_domain.hpp
src/test/**/*.{cc,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
Place Catch2 unit/integration tests in src/test and add focused tests when modifying verifier behaviour
Files:
src/test/test_ebpf_domain.cpp
**/*.{cc,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
Favor explicit error handling and early exits in the verifier to surface problems rather than relying on implicit behaviour
Files:
src/test/test_ebpf_domain.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: vbpf/prevail#0
File: AGENTS.md:0-0
Timestamp: 2025-09-15T22:00:28.296Z
Learning: Applies to src/test/**/*.{cc,cpp} : Place Catch2 unit/integration tests in src/test and add focused tests when modifying verifier behaviour
📚 Learning: 2025-09-15T22:00:28.296Z
Learnt from: CR
PR: vbpf/prevail#0
File: AGENTS.md:0-0
Timestamp: 2025-09-15T22:00:28.296Z
Learning: Applies to src/test/**/*.{cc,cpp} : Place Catch2 unit/integration tests in src/test and add focused tests when modifying verifier behaviour
Applied to files:
CMakeLists.txtsrc/test/test_ebpf_domain.cpp
🧬 Code graph analysis (1)
src/test/test_ebpf_domain.cpp (2)
src/crab/ebpf_domain.hpp (6)
reg(90-90)reg(92-92)reg(94-94)EbpfDomain(41-41)EbpfDomain(42-42)constraints(69-69)src/platform.hpp (3)
map_fd(36-36)platform_specific_type(18-18)map_type(26-26)
🔇 Additional comments (8)
CMakeLists.txt (2)
116-116: Test suite wiring looks correct (gated, linked, tagged).
src/test/test_ebpf_domain.cppis added underVERIFIER_ENABLE_TESTSand linked withCatch2::Catch2WithMain. No production impact.
33-39: Confirm Catch2 pin and C++20 designated‑initializer support across CICatch2 is pinned to v3.10.0 (current v3 release; released Aug 25, 2025). MSVC (VS 2022) supports C++20 designated initializers only when compiled with /std:c++20 or /std:c++latest and enforces restrictions (designators must follow member declaration order; cannot mix designated and positional initializers; nested-designator support is limited). Ensure CI images for GCC/Clang/MSVC enable C++20 and use Catch2 v3.10.0, and verify tests’ designated‑initializer usage is compatible with MSVC restrictions.
Locations: CMakeLists.txt: 33–39; also applies to 146–151, 203–224
src/test/test_ebpf_domain.cpp (5)
1-2: License header present and correct.Meets SPDX requirement for new sources.
76-145: Test platform shim is cohesive and thread‑local safe.Function pointers and TLS map types isolate tests from production platform.
178-182: Helper for domain construction is clear.Top stack + injected numeric domain mirrors production constructors.
184-190:as_setbottom handling is correct.Avoids leaking “bottom” markers into expectations.
490-519: Map descriptor range tests cover both consistent and inconsistent cases well.Good coverage of type join, range limiting, and property aggregation.
Also applies to: 521-535, 537-550
src/crab/ebpf_domain.hpp (1)
36-36: Test-only friend is fine — only referenced in tests.
Search shows EbpfDomainTestAccess only in src/test/test_ebpf_domain.cpp; friend declaration at src/crab/ebpf_domain.hpp:36. No ABI/behavior change; no action required.
| using namespace prevail; | ||
|
|
||
| namespace prevail { | ||
| class EbpfDomainTestAccess { | ||
| public: | ||
| static NumAbsDomain& numeric(EbpfDomain& dom) { return dom.m_inv; } | ||
| static const NumAbsDomain& numeric(const EbpfDomain& dom) { return dom.m_inv; } | ||
| static ArrayDomain& stack(EbpfDomain& dom) { return dom.stack; } | ||
| static const ArrayDomain& stack(const EbpfDomain& dom) { return dom.stack; } | ||
| static TypeDomain& types(EbpfDomain& dom) { return dom.type_inv; } | ||
| static const TypeDomain& types(const EbpfDomain& dom) { return dom.type_inv; } | ||
|
|
||
| static std::optional<Variable> type_offset_variable(const Reg& reg, int type) { | ||
| return EbpfDomain::get_type_offset_variable(reg, type); | ||
| } | ||
|
|
||
| static std::optional<Variable> type_offset_variable(const EbpfDomain& dom, const Reg& reg) { | ||
| return dom.get_type_offset_variable(reg); | ||
| } | ||
|
|
||
| static std::optional<Variable> type_offset_variable(const EbpfDomain& dom, const Reg& reg, | ||
| const NumAbsDomain& inv) { | ||
| return dom.get_type_offset_variable(reg, inv); | ||
| } | ||
|
|
||
| static std::optional<uint32_t> map_type(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_type(reg); } | ||
|
|
||
| static std::optional<uint32_t> map_inner_map_fd(const EbpfDomain& dom, const Reg& reg) { | ||
| return dom.get_map_inner_map_fd(reg); | ||
| } | ||
|
|
||
| static Interval map_key_size(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_key_size(reg); } | ||
|
|
||
| static Interval map_value_size(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_value_size(reg); } | ||
|
|
||
| static Interval map_max_entries(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_max_entries(reg); } | ||
|
|
||
| static bool map_fd_range(const EbpfDomain& dom, const Reg& reg, int32_t* start_fd, int32_t* end_fd) { | ||
| return dom.get_map_fd_range(reg, start_fd, end_fd); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick
Make the test accessor non-instantiable.
Prevent accidental construction; it’s a pure static helper.
class EbpfDomainTestAccess {
public:
+ EbpfDomainTestAccess() = delete;
+ EbpfDomainTestAccess(const EbpfDomainTestAccess&) = delete;
+ EbpfDomainTestAccess& operator=(const EbpfDomainTestAccess&) = delete;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using namespace prevail; | |
| namespace prevail { | |
| class EbpfDomainTestAccess { | |
| public: | |
| static NumAbsDomain& numeric(EbpfDomain& dom) { return dom.m_inv; } | |
| static const NumAbsDomain& numeric(const EbpfDomain& dom) { return dom.m_inv; } | |
| static ArrayDomain& stack(EbpfDomain& dom) { return dom.stack; } | |
| static const ArrayDomain& stack(const EbpfDomain& dom) { return dom.stack; } | |
| static TypeDomain& types(EbpfDomain& dom) { return dom.type_inv; } | |
| static const TypeDomain& types(const EbpfDomain& dom) { return dom.type_inv; } | |
| static std::optional<Variable> type_offset_variable(const Reg& reg, int type) { | |
| return EbpfDomain::get_type_offset_variable(reg, type); | |
| } | |
| static std::optional<Variable> type_offset_variable(const EbpfDomain& dom, const Reg& reg) { | |
| return dom.get_type_offset_variable(reg); | |
| } | |
| static std::optional<Variable> type_offset_variable(const EbpfDomain& dom, const Reg& reg, | |
| const NumAbsDomain& inv) { | |
| return dom.get_type_offset_variable(reg, inv); | |
| } | |
| static std::optional<uint32_t> map_type(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_type(reg); } | |
| static std::optional<uint32_t> map_inner_map_fd(const EbpfDomain& dom, const Reg& reg) { | |
| return dom.get_map_inner_map_fd(reg); | |
| } | |
| static Interval map_key_size(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_key_size(reg); } | |
| static Interval map_value_size(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_value_size(reg); } | |
| static Interval map_max_entries(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_max_entries(reg); } | |
| static bool map_fd_range(const EbpfDomain& dom, const Reg& reg, int32_t* start_fd, int32_t* end_fd) { | |
| return dom.get_map_fd_range(reg, start_fd, end_fd); | |
| } | |
| }; | |
| using namespace prevail; | |
| namespace prevail { | |
| class EbpfDomainTestAccess { | |
| public: | |
| EbpfDomainTestAccess() = delete; | |
| EbpfDomainTestAccess(const EbpfDomainTestAccess&) = delete; | |
| EbpfDomainTestAccess& operator=(const EbpfDomainTestAccess&) = delete; | |
| static NumAbsDomain& numeric(EbpfDomain& dom) { return dom.m_inv; } | |
| static const NumAbsDomain& numeric(const EbpfDomain& dom) { return dom.m_inv; } | |
| static ArrayDomain& stack(EbpfDomain& dom) { return dom.stack; } | |
| static const ArrayDomain& stack(const EbpfDomain& dom) { return dom.stack; } | |
| static TypeDomain& types(EbpfDomain& dom) { return dom.type_inv; } | |
| static const TypeDomain& types(const EbpfDomain& dom) { return dom.type_inv; } | |
| static std::optional<Variable> type_offset_variable(const Reg& reg, int type) { | |
| return EbpfDomain::get_type_offset_variable(reg, type); | |
| } | |
| static std::optional<Variable> type_offset_variable(const EbpfDomain& dom, const Reg& reg) { | |
| return dom.get_type_offset_variable(reg); | |
| } | |
| static std::optional<Variable> type_offset_variable(const EbpfDomain& dom, const Reg& reg, | |
| const NumAbsDomain& inv) { | |
| return dom.get_type_offset_variable(reg, inv); | |
| } | |
| static std::optional<uint32_t> map_type(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_type(reg); } | |
| static std::optional<uint32_t> map_inner_map_fd(const EbpfDomain& dom, const Reg& reg) { | |
| return dom.get_map_inner_map_fd(reg); | |
| } | |
| static Interval map_key_size(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_key_size(reg); } | |
| static Interval map_value_size(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_value_size(reg); } | |
| static Interval map_max_entries(const EbpfDomain& dom, const Reg& reg) { return dom.get_map_max_entries(reg); } | |
| static bool map_fd_range(const EbpfDomain& dom, const Reg& reg, int32_t* start_fd, int32_t* end_fd) { | |
| return dom.get_map_fd_range(reg, start_fd, end_fd); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/test/test_ebpf_domain.cpp around lines 33-73, the EbpfDomainTestAccess
class is a pure static helper but is currently instantiable; make it
non-instantiable by deleting its constructors and assignment operators: add a
deleted default constructor (EbpfDomainTestAccess() = delete;), delete the
destructor if desired, and delete copy and move constructors/assignment
(EbpfDomainTestAccess(const EbpfDomainTestAccess&) = delete;
EbpfDomainTestAccess& operator=(const EbpfDomainTestAccess&) = delete;
EbpfDomainTestAccess(EbpfDomainTestAccess&&) = delete; EbpfDomainTestAccess&
operator=(EbpfDomainTestAccess&&) = delete;) so the class cannot be constructed
or copied.
| class EbpfDomainTestEnvironment { | ||
| public: | ||
| EbpfDomainTestEnvironment(const ebpf_context_descriptor_t& context, std::vector<EbpfMapDescriptor> descriptors = {}, | ||
| std::unordered_map<uint32_t, EbpfMapType> map_types = {}) { | ||
| thread_local_options = {}; | ||
| variable_registry.clear(); | ||
| g_test_map_types = std::move(map_types); | ||
|
|
||
| ProgramInfo info{}; | ||
| info.platform = &kTestPlatform; | ||
| info.map_descriptors = std::move(descriptors); | ||
| info.type = EbpfProgramType{ | ||
| .name = "test", | ||
| .context_descriptor = &context, | ||
| .platform_specific_data = 0, | ||
| .section_prefixes = {}, | ||
| .is_privileged = false, | ||
| }; | ||
| thread_local_program_info.set(info); | ||
| } | ||
|
|
||
| ~EbpfDomainTestEnvironment() { | ||
| g_test_map_types.clear(); | ||
| thread_local_program_info.clear(); | ||
| variable_registry.clear(); | ||
| thread_local_options = {}; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick
RAII env resets global TLS; consider snapshot/restore for stricter isolation.
Optional: snapshot previous thread_local_options, program info, and g_test_map_types then restore in dtor instead of zeroing to defaults. Reduces inter-test coupling if future tests rely on prior non-defaults.
🤖 Prompt for AI Agents
In src/test/test_ebpf_domain.cpp around lines 149 to 176, the RAII test
environment unconditionally clears and zeroes global/thread-local state
(g_test_map_types, thread_local_program_info, thread_local_options,
variable_registry) which may break tests that rely on prior non-default TLS;
modify the constructor to save copies of the previous thread_local_options,
thread_local_program_info (or its underlying ProgramInfo), and g_test_map_types
before mutating them, and modify the destructor to restore those saved snapshots
instead of clearing/zeroing — ensure you use move semantics only for the new
test data but keep backups as values, and restore them in the destructor to
return global/TLS state to its original values.
| TEST_CASE("EbpfDomain widen optionally clamps to constant limits", "[ebpf_domain]") { | ||
| using namespace dsl_syntax; | ||
|
|
||
| EbpfDomainTestEnvironment env(kContextWithMeta); | ||
|
|
||
| const auto r0 = reg_pack(R0_RETURN_VALUE); | ||
|
|
||
| NumAbsDomain bounded = NumAbsDomain::top(); | ||
| bounded.add_constraint(r0.svalue >= -5); | ||
| bounded.add_constraint(r0.svalue <= 5); | ||
| EbpfDomain precise = make_domain(bounded); | ||
|
|
||
| EbpfDomain unconstrained = make_domain(NumAbsDomain::top()); | ||
|
|
||
| EbpfDomain widened_no_limits = precise.widen(unconstrained, false); | ||
| EbpfDomain widened_with_limits = precise.widen(unconstrained, true); | ||
|
|
||
| const auto no_limits_interval = EbpfDomainTestAccess::numeric(widened_no_limits).eval_interval(r0.svalue); | ||
| REQUIRE(no_limits_interval.ub().is_infinite()); | ||
|
|
||
| const auto limited_interval = EbpfDomainTestAccess::numeric(widened_with_limits).eval_interval(r0.svalue); | ||
| REQUIRE(limited_interval.ub().is_finite()); | ||
| REQUIRE(limited_interval.ub().narrow<int32_t>() == std::numeric_limits<int32_t>::max()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Widening clamp assertion is precise; guard includes for fixed-width ints.
int32_t is used later; include <cstdint> explicitly to avoid transitive-dependency flakiness.
#include <algorithm>
#include <array>
+#include <cstdint>
#include <limits>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/test_ebpf_domain.cpp around lines 326 to 349, the test uses int32_t
but does not include the header that defines fixed-width integer types; add a
direct include for <cstdint> at the top of this file (or the nearest test header
includes) so int32_t is guaranteed to be available and the test doesn't rely on
transitive includes.
| TEST_CASE("EbpfDomain setup_entry initialises registers and packet", "[ebpf_domain]") { | ||
| EbpfDomainTestEnvironment env(kContextWithMeta); | ||
|
|
||
| SECTION("with r1 initialisation") { | ||
| EbpfDomain dom = EbpfDomain::setup_entry(true); | ||
| const auto& inv = EbpfDomainTestAccess::numeric(dom); | ||
| const auto r10 = reg_pack(R10_STACK_POINTER); | ||
| const auto r1 = reg_pack(R1_ARG); | ||
|
|
||
| REQUIRE(inv.eval_interval(r10.svalue) == Interval(EBPF_TOTAL_STACK_SIZE, PTR_MAX)); | ||
| REQUIRE(inv.eval_interval(r10.stack_offset) == Interval(EBPF_TOTAL_STACK_SIZE, EBPF_TOTAL_STACK_SIZE)); | ||
| REQUIRE(inv.eval_interval(r10.type) == Interval(T_STACK, T_STACK)); | ||
|
|
||
| REQUIRE(inv.eval_interval(r1.svalue) == Interval(1, PTR_MAX)); | ||
| REQUIRE(inv.eval_interval(r1.ctx_offset) == Interval(0, 0)); | ||
| REQUIRE(inv.eval_interval(r1.type) == Interval(T_CTX, T_CTX)); | ||
|
|
||
| REQUIRE(inv.eval_interval(variable_registry->packet_size()) == Interval(0, MAX_PACKET_SIZE - 1)); | ||
| REQUIRE(inv.eval_interval(variable_registry->meta_offset()) == Interval(-4098, 0)); | ||
| } | ||
|
|
||
| SECTION("without r1 initialisation") { | ||
| EbpfDomain dom = EbpfDomain::setup_entry(false); | ||
| const auto& inv = EbpfDomainTestAccess::numeric(dom); | ||
| const auto r1 = reg_pack(R1_ARG); | ||
| const Interval type = inv.eval_interval(r1.type); | ||
| REQUIRE(type.is_top()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Avoid magic number for metadata lower bound.
Name the -4098 bound once to ease future changes.
- REQUIRE(inv.eval_interval(variable_registry->meta_offset()) == Interval(-4098, 0));
+ constexpr int kMetaLowerBound = -4098;
+ REQUIRE(inv.eval_interval(variable_registry->meta_offset()) == Interval(kMetaLowerBound, 0));Apply similarly in the packet initialization test.
Also applies to: 400-418
🤖 Prompt for AI Agents
In src/test/test_ebpf_domain.cpp around lines 370 to 398 (also apply the same
change at 400-418), replace the magic literal -4098 with a named constant:
declare a descriptive constant (e.g. const int META_OFFSET_MIN = -4098) near the
test file top or in the test fixture and use that constant in the
REQUIRE(assertion) for meta_offset() so the bound is named once and reused in
both packet and metadata initialization tests; update any other occurrences in
the referenced range to use the same constant.
| TEST_CASE("EbpfDomain constant limits bound registers and counters", "[ebpf_domain]") { | ||
| EbpfDomainTestEnvironment env(kContextWithMeta); | ||
| thread_local_options.cfg_opts.check_for_termination = true; | ||
| variable_registry->loop_counter("loop"); | ||
|
|
||
| EbpfDomain limits = EbpfDomain::calculate_constant_limits(); | ||
| const auto& inv = EbpfDomainTestAccess::numeric(limits); | ||
|
|
||
| const auto r0 = reg_pack(R0_RETURN_VALUE); | ||
| const Interval r0_bounds = inv.eval_interval(r0.svalue); | ||
| REQUIRE(r0_bounds == Interval(0, std::numeric_limits<int32_t>::max())); | ||
| REQUIRE(inv.eval_interval(r0.uvalue) == Interval(uint32_t{0}, std::numeric_limits<uint32_t>::max())); | ||
| REQUIRE(inv.eval_interval(r0.stack_offset) == Interval(0, EBPF_TOTAL_STACK_SIZE)); | ||
|
|
||
| const Variable counter = variable_registry->loop_counter("loop"); | ||
| REQUIRE(inv.eval_interval(counter) == Interval(0, std::numeric_limits<int32_t>::max())); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Option toggle scope.
You flip check_for_termination mid-test; if future sections add more assertions after this, state may leak. Optional: use a small guard to restore the prior value on scope exit.
struct OptionsGuard {
ebpf_verifier_options_t saved;
OptionsGuard() : saved(thread_local_options) {}
~OptionsGuard() { thread_local_options = saved; }
};
// usage:
// OptionsGuard guard;
// thread_local_options.cfg_opts.check_for_termination = true;🤖 Prompt for AI Agents
In src/test/test_ebpf_domain.cpp around lines 552 to 568, the test toggles
thread_local_options.cfg_opts.check_for_termination mid-test which can leak
modified state into later tests; wrap the change in a scope guard that saves the
previous thread_local_options (or at least the cfg_opts.check_for_termination
flag) on construction and restores it on destruction so the original option
value is reinstated when the test block exits (use an RAII-style guard or
explicit save/restore immediately before and after setting the flag).
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68c90da5b05c83299daec10a2b4c3022
Summary by CodeRabbit