Add EbpfTransformer Catch2 test suite #911
Conversation
Signed-off-by: Prevail AI <prevail-ai@example.com>
WalkthroughAdds a new eBPF transformer test suite, wires it into the build when tests are enabled, and exposes internal state of EbpfDomain to tests via a forward declaration and friend class. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
elazarg
changed the title
Pull Request Test Coverage Report for Build 17776994019Details
💛 - 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
203-224: Define PREVAIL_TESTING for test builds to enable the gated friend.Propagate a test-only compile definition to both the library and tests.
Apply this diff:
if (VERIFIER_ENABLE_TESTS) + # Enable test-only access (gated friend) in headers when building tests + target_compile_definitions(prevail PUBLIC PREVAIL_TESTING) target_compile_options(check PRIVATE ${COMMON_FLAGS}) @@ target_link_libraries(tests PRIVATE Threads::Threads) target_link_libraries(tests PRIVATE Catch2::Catch2WithMain) + target_compile_definitions(tests PRIVATE PREVAIL_TESTING)
📜 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_transformer.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_transformer.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_transformer.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_transformer.cpp
🧠 Learnings (2)
📚 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_transformer.cpp
📚 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 CMakeLists.txt : Maintain C++20 build configuration for the core verifier (e.g., CMAKE_CXX_STANDARD 20) in the top-level CMakeLists.txt
Applied to files:
CMakeLists.txt
🧬 Code graph analysis (1)
src/test/test_ebpf_transformer.cpp (2)
src/platform.hpp (1)
map_fd(36-36)src/crab/ebpf_domain.hpp (2)
ebpf_domain_transform(26-26)ebpf_domain_initialize_loop_counter(31-31)
🔇 Additional comments (1)
src/crab/ebpf_domain.hpp (1)
33-34: Gate test-only inspector behind PREVAIL_TESTINGWrap the forward declaration and friend with a test-only macro so test-only internals aren't exposed to production.
Location: src/crab/ebpf_domain.hpp (forward declaration + friend)
- class EbpfDomainInspector; + #ifdef PREVAIL_TESTING + class EbpfDomainInspector; + #endif @@ class EbpfDomain final { friend class EbpfChecker; friend class EbpfTransformer; - friend class EbpfDomainInspector; + #ifdef PREVAIL_TESTING + friend class EbpfDomainInspector; + #endifVerify there are no non-test references to EbpfDomainInspector before applying (search in the sandbox was inconclusive).
| template <typename T> | ||
| void apply(EbpfDomain& dom, T instruction) { | ||
| ebpf_domain_transform(dom, Instruction{std::move(instruction)}); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Use perfect forwarding to avoid extra copies in apply().
Minor tidy-up; reduces copies of Instruction wrappers.
Apply this diff:
-template <typename T>
-void apply(EbpfDomain& dom, T instruction) {
- ebpf_domain_transform(dom, Instruction{std::move(instruction)});
-}
+template <typename T>
+void apply(EbpfDomain& dom, T&& instruction) {
+ ebpf_domain_transform(dom, Instruction{std::forward<T>(instruction)});
+}📝 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.
| template <typename T> | |
| void apply(EbpfDomain& dom, T instruction) { | |
| ebpf_domain_transform(dom, Instruction{std::move(instruction)}); | |
| } | |
| template <typename T> | |
| void apply(EbpfDomain& dom, T&& instruction) { | |
| ebpf_domain_transform(dom, Instruction{std::forward<T>(instruction)}); | |
| } |
🤖 Prompt for AI Agents
In src/test/test_ebpf_transformer.cpp around lines 71-75, the apply() template
takes its instruction by value causing an extra copy when constructing the
Instruction wrapper; change the signature to take T&& and forward the argument
into the Instruction construction using std::forward<T>(instruction) so the
wrapper is constructed in-place without unnecessary copies (i.e., make apply a
forwarding reference and use std::forward when passing into
ebpf_domain_transform).
| void set_scalar(EbpfDomain& dom, const Reg reg, const int64_t value) { | ||
| const uint64_t magnitude = value < 0 ? static_cast<uint64_t>(-value) : static_cast<uint64_t>(value); | ||
| apply(dom, Bin{.op = Bin::Op::MOV, .dst = reg, .v = Imm{magnitude}, .is64 = true, .lddw = false}); | ||
| if (value < 0) { | ||
| apply(dom, Un{.op = Un::Op::NEG, .dst = reg, .is64 = true}); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Guard against INT64_MIN overflow in set_scalar().
Unary minus on INT64_MIN is UB; also allow 64-bit immediates via lddw.
Apply this diff:
void set_scalar(EbpfDomain& dom, const Reg reg, const int64_t value) {
- const uint64_t magnitude = value < 0 ? static_cast<uint64_t>(-value) : static_cast<uint64_t>(value);
- apply(dom, Bin{.op = Bin::Op::MOV, .dst = reg, .v = Imm{magnitude}, .is64 = true, .lddw = false});
- if (value < 0) {
- apply(dom, Un{.op = Un::Op::NEG, .dst = reg, .is64 = true});
- }
+ if (value == std::numeric_limits<int64_t>::min()) {
+ // Load raw 0x8000... and rely on the domain's signed view to interpret as -2^63.
+ apply(dom, Bin{.op = Bin::Op::MOV, .dst = reg, .v = Imm{0x8000000000000000ULL}, .is64 = true, .lddw = true});
+ return;
+ }
+ const uint64_t magnitude = value < 0 ? static_cast<uint64_t>(-(value)) : static_cast<uint64_t>(value);
+ const bool needs_lddw = magnitude > std::numeric_limits<uint32_t>::max();
+ apply(dom, Bin{.op = Bin::Op::MOV, .dst = reg, .v = Imm{magnitude}, .is64 = true, .lddw = needs_lddw});
+ if (value < 0) apply(dom, Un{.op = Un::Op::NEG, .dst = reg, .is64 = true});
}📝 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.
| void set_scalar(EbpfDomain& dom, const Reg reg, const int64_t value) { | |
| const uint64_t magnitude = value < 0 ? static_cast<uint64_t>(-value) : static_cast<uint64_t>(value); | |
| apply(dom, Bin{.op = Bin::Op::MOV, .dst = reg, .v = Imm{magnitude}, .is64 = true, .lddw = false}); | |
| if (value < 0) { | |
| apply(dom, Un{.op = Un::Op::NEG, .dst = reg, .is64 = true}); | |
| } | |
| } | |
| void set_scalar(EbpfDomain& dom, const Reg reg, const int64_t value) { | |
| if (value == std::numeric_limits<int64_t>::min()) { | |
| // Load raw 0x8000... and rely on the domain's signed view to interpret as -2^63. | |
| apply(dom, Bin{.op = Bin::Op::MOV, .dst = reg, .v = Imm{0x8000000000000000ULL}, .is64 = true, .lddw = true}); | |
| return; | |
| } | |
| const uint64_t magnitude = value < 0 ? static_cast<uint64_t>(-(value)) : static_cast<uint64_t>(value); | |
| const bool needs_lddw = magnitude > std::numeric_limits<uint32_t>::max(); | |
| apply(dom, Bin{.op = Bin::Op::MOV, .dst = reg, .v = Imm{magnitude}, .is64 = true, .lddw = needs_lddw}); | |
| if (value < 0) apply(dom, Un{.op = Un::Op::NEG, .dst = reg, .is64 = true}); | |
| } |
🤖 Prompt for AI Agents
In src/test/test_ebpf_transformer.cpp around lines 96-102, set_scalar()
currently negates int64_t value which is undefined for INT64_MIN and doesn't use
lddw for full 64-bit immediates; change the logic to detect value == INT64_MIN
and handle it without performing unary minus: for INT64_MIN load the unsigned
64-bit immediate (uint64_t) representing INT64_MIN using a MOV with lddw = true
and do not emit a subsequent NEG; for other negative values compute magnitude as
static_cast<uint64_t>(-value) and for any immediate that requires full 64-bit
width set lddw = true on the MOV (otherwise keep lddw = false), and only emit
the NEG when value is negative and not INT64_MIN.
| descriptor.type = 1; // Regular data map. | ||
|
|
||
| apply(dom, LoadMapFd{.dst = r(1), .mapfd = map_fd}); | ||
|
|
||
| const auto pack = reg_pack(r(1)); | ||
| CHECK(reg_type(dom, r(1)) == T_MAP); | ||
| CHECK(interval(dom, pack.map_fd) == Interval(map_fd)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Replace magic map-type numbers with named constants.
Improves readability and reduces coupling to UAPI numeric values.
Apply this diff to add local constants:
namespace {
+// Minimal named aliases to avoid magic numbers in tests.
+constexpr int BPF_MAP_TYPE_ARRAY = 1;
+constexpr int BPF_MAP_TYPE_PROG_ARRAY = 3;
+constexpr int BPF_MAP_TYPE_ARRAY_OF_MAPS = 12;Then update usages:
- descriptor.type = 1; // Regular data map.
+ descriptor.type = BPF_MAP_TYPE_ARRAY; // Regular data map.
@@
- descriptor.type = 3; // BPF_MAP_TYPE_PROG_ARRAY -> program maps.
+ descriptor.type = BPF_MAP_TYPE_PROG_ARRAY; // Program maps.
@@
- descriptor.type = 1;
+ descriptor.type = BPF_MAP_TYPE_ARRAY;
@@
- descriptor.type = 12; // ARRAY_OF_MAPS -> returns another map fd.
+ descriptor.type = BPF_MAP_TYPE_ARRAY_OF_MAPS; // Returns inner map fd.Also applies to: 552-560, 566-574, 622-624
🤖 Prompt for AI Agents
In src/test/test_ebpf_transformer.cpp around lines 538-545 (and also update
occurrences at 552-560, 566-574, 622-624), replace hard-coded numeric map-type
literals (e.g. "descriptor.type = 1") with a named local constant (e.g.
constexpr int MAP_TYPE_REGULAR = <uapi value>), declare the constant near the
top of the test file or inside the test scope, and then update all usages in the
specified ranges to use the named constant instead of the magic number so the
code reads descriptively and avoids coupling to raw UAPI integers.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68c897a330288329bd41b7fed0dbb44a
Summary by CodeRabbit