Move checker and transformer to different cpp files #788
Conversation
Signed-off-by: Elazar Gershuni <[email protected]>
WalkthroughThis pull request introduces significant changes to the eBPF framework, primarily through the addition of an Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range comments (4)
test-data/packet.yaml (2)
Line range hint
32-256: Enhance legacy packet access test coverage.While the legacy packet access test cases cover different data sizes and access methods (immediate vs register), they could be more comprehensive:
- Consider varying the offset values instead of always using 23
- Add assertions for the actual values read from the packet
- Test boundary conditions (e.g., reading near packet size limits)
- Enhance pre/post conditions to specify expected value ranges for r0
Example improvement for the 2-byte case:
test-case: legacy 2 byte packet access boundary pre: ["r1.type=number", "r6.type=ctx", "r6.ctx_offset=0", "packet_size=65534"] # Add packet size code: <start>: | r0 = *(u16 *)skb[65532] # Test near boundary post: ["r0.type=number", "r0.uvalue=[0, 65535]", # Specify expected range "r6.type=ctx", "r6.ctx_offset=0"]
Line range hint
1-256: Consider reorganizing test cases for better maintainability.The test file would benefit from better organization:
- Group related test cases together (e.g., all unsigned read tests, all legacy access tests)
- Add comments explaining each group of tests
- Consider adding a table of contents at the top
Example structure:
# Test categories: # 1. Unsigned value reading tests # 2. Invalid access tests # 3. Legacy packet access tests # 3.1 Immediate offset tests # 3.2 Register offset testssrc/crab_verifier.cpp (1)
Line range hint
1-256: Well-executed architectural refactoring.The changes successfully achieve the PR objectives by:
- Moving checker/transformer functionality to separate files
- Reducing the interface exposed by ebpf_domain.hpp
- Maintaining existing behavior and error handling
- Improving code organization through better separation of concerns
The refactoring makes the codebase more maintainable by properly encapsulating the checker and transformer implementations.
test-data/jump.yaml (1)
Line range hint
763-1241: Consider adding test cases for non-zero immediate values.While the current test cases thoroughly cover comparisons with immediate value 0, it might be valuable to add similar test cases for non-zero immediate values to ensure consistent behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
src/crab/ebpf_checker.cpp(1 hunks)src/crab/ebpf_domain.hpp(2 hunks)src/crab/fwd_analyzer.cpp(2 hunks)src/crab_verifier.cpp(2 hunks)test-data/jump.yaml(21 hunks)test-data/packet.yaml(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/crab/ebpf_domain.hpp (2)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.hpp:152-152
Timestamp: 2024-10-11T21:13:34.961Z
Learning: In `src/crab/ebpf_domain.hpp`, methods in the `ebpf_domain_t` class typically do not have documentation comments, so adding documentation for new methods is not necessary.
Learnt from: elazarg
PR: vbpf/ebpf-verifier#692
File: src/crab/ebpf_domain.cpp:1366-1367
Timestamp: 2024-09-27T00:34:47.474Z
Learning: The project is compiled with C++20.
src/crab/fwd_analyzer.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#745
File: src/crab/fwd_analyzer.hpp:14-14
Timestamp: 2024-11-05T20:59:02.521Z
Learning: When reviewing changes, avoid suggesting documentation additions for pre-existing functions (e.g., `run_forward_analyzer` in `src/crab/fwd_analyzer.hpp`) if they are outside the scope of the current PR.
🪛 cppcheck
src/crab/ebpf_checker.cpp
[style] 106-106: The function 'ebpf_domain_assume' is never used.
(unusedFunction)
[style] 113-113: The function 'ebpf_domain_check' is never used.
(unusedFunction)
🔇 Additional comments (8)
src/crab/fwd_analyzer.cpp (1)
74-80: LGTM! Verify the assume-assert fix.
The changes correctly handle assertions by validating preconditions before transformation, which should fix the broken assume-assert issue mentioned in PR #787.
Let's verify the fix by checking for related test cases:
✅ Verification successful
Based on the evidence gathered, I can now provide a final response:
The assume-assert changes are well-tested and correctly implemented
The changes in fwd_analyzer.cpp are verified to be correct because:
- The
thread_local_options.assume_assertionsflag is consistently used across the codebase (ebpf_checker.cpp, ebpf_transformer.cpp) - The implementation is thoroughly tested with a comprehensive test suite in
test-data/jump.yamlcovering all comparison operations with assume_assertions enabled - The code follows the same pattern as other assertion handling in the codebase, where assertions are used to avoid redundant errors
- The changes maintain consistency with the existing error handling pattern seen in ebpf_checker.cpp where assertions are assumed to prevent duplicate warnings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for test cases that verify the assume-assert behavior
rg -g '*.yaml' -l 'assume|assert' tests/
Length of output: 89
Script:
#!/bin/bash
# Let's first find test files in the repository
fd -e yaml -e cpp -e c test
Length of output: 196
Script:
#!/bin/bash
# Search for assume/assert related test cases in all files
rg -l "assume.*assert|assert.*assume"
Length of output: 233
Script:
#!/bin/bash
# Look for the usage of thread_local_options.assume_assertions
rg "thread_local_options.assume_assertions"
Length of output: 397
Script:
#!/bin/bash
# Find test files that might be related to the analyzer
fd "test.*analyzer|analyzer.*test"
Length of output: 36
Script:
#!/bin/bash
# Let's check the test-data/jump.yaml content as it appears to have assume/assert references
cat test-data/jump.yaml
Length of output: 22932
Script:
#!/bin/bash
# Let's examine how assume_assertions is used in ebpf_checker.cpp for better context
rg -A 5 -B 5 "thread_local_options.assume_assertions" src/crab/ebpf_checker.cpp
Length of output: 687
Script:
#!/bin/bash
# Let's look at the ebpf_domain_assume implementation
ast-grep --pattern 'ebpf_domain_assume($$$)'
Length of output: 131
Script:
#!/bin/bash
# Let's look at the ebpf_domain_assume function definition
ast-grep --pattern 'void ebpf_domain_assume($$$)'
Length of output: 51
Script:
#!/bash
# Let's check if there are any test cases specifically for assume_assertions option
rg -A 3 -B 3 "options.*assume_assertions" test-data/jump.yaml
Length of output: 3404
Script:
#!/bin/bash
# Let's see how ebpf_domain_assume is used in other files
rg "ebpf_domain_assume" -A 2 -B 2
Length of output: 1006
src/crab_verifier.cpp (1)
159-159: LGTM! Good architectural improvement.
Moving setup_entry to ebpf_domain_t makes more sense architecturally as it's more closely related to domain initialization than transformation.
test-data/jump.yaml (1)
Line range hint 763-1241: LGTM! Comprehensive test coverage for pointer comparisons.
The added test cases thoroughly cover all jump instructions when comparing pointers with immediate value 0. The test cases consistently verify that:
- Pointer comparisons with immediate 0 are properly handled
- Appropriate error messages are generated for invalid operations
- Post-conditions are empty when operations are invalid
The test cases follow a systematic pattern covering:
- Regular jumps (JEQ, JNE, JSET, etc.)
- Signed jumps (JSLT, JSLE, JSGT, JSGE)
- 32-bit variants (JEQ32, JNE32, etc.)
src/crab/ebpf_domain.hpp (3)
18-25: Addition of constants for pointer arithmetic capping
The constants MAX_PACKET_SIZE and PTR_MAX are appropriately defined to cap pointer arithmetic to 32 bits, ensuring correct pointer operations within the eBPF domain.
29-31: Addition of domain manipulation function declarations
The declarations of ebpf_domain_transform, ebpf_domain_assume, and ebpf_domain_check provide necessary interfaces for domain manipulation and are correctly added.
70-72: Addition of new methods to ebpf_domain_t class
The methods setup_entry, from_constraints, and initialize_packet are appropriately added to the ebpf_domain_t class, enhancing its functionality.
src/crab/ebpf_checker.cpp (2)
113-120: Verify the usage of the function ebpf_domain_check.
Similarly, the function ebpf_domain_check is defined but may not be used within the codebase. Please confirm whether this function is invoked elsewhere. Removing unused functions can help reduce code complexity.
Run the following script to check for usages of ebpf_domain_check in the codebase:
✅ Verification successful
Function ebpf_domain_check is actively used in the codebase
The function is used in src/crab_verifier.cpp for checking assertions and collecting warnings. It's also properly declared in the header file src/crab/ebpf_domain.hpp. This is a core verification function that should not be removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of 'ebpf_domain_check' outside 'ebpf_checker.cpp'
rg 'ebpf_domain_check' --glob '!'src/crab/ebpf_checker.cpp''
Length of output: 311
🧰 Tools
🪛 cppcheck
[style] 113-113: The function 'ebpf_domain_check' is never used.
(unusedFunction)
106-112: Verify the usage of the function ebpf_domain_assume.
The function ebpf_domain_assume appears to be defined but not used within the provided code. Please verify if this function is utilized elsewhere in the codebase. If it's unused, consider removing it to keep the codebase clean and maintainable.
Run the following script to check for usages of ebpf_domain_assume in the codebase:
✅ Verification successful
The function ebpf_domain_assume is actively used in the codebase
The function is used in src/crab/fwd_analyzer.cpp and properly declared in the header file src/crab/ebpf_domain.hpp. It's an essential part of the codebase's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of 'ebpf_domain_assume' outside 'ebpf_checker.cpp'
rg 'ebpf_domain_assume' --glob '!'src/crab/ebpf_checker.cpp''
Length of output: 239
🧰 Tools
🪛 cppcheck
[style] 106-106: The function 'ebpf_domain_assume' is never used.
(unusedFunction)
Continue #787. Reduce the size of the interface exposed by ebpf_domain.hpp, so the both ebpf_transformer and ebpf_checker are not visible outside of their respective cpp files.
Two fixes that are not refactoring:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests