Uniform printing of simplified vs non-simplified #805
Conversation
Do not print entry and exit. Signed-off-by: Elazar Gershuni <[email protected]>
WalkthroughThe pull request introduces a new method Changes
Possibly related PRs
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
src/asm_cfg.cpp(1 hunks)src/crab/cfg.hpp(1 hunks)src/crab_verifier.cpp(1 hunks)src/crab_verifier.hpp(1 hunks)src/main/check.cpp(1 hunks)
🔇 Additional comments (8)
src/crab_verifier.cpp (2)
53-66: Implementation aligns with PR objectives
The code achieves uniform printing by:
- Using LineInfoPrinter consistently for line information
- Collecting basic blocks with the new simplify parameter
- Following a clear printing sequence
52-52: Verify callers of print_invariants
The method signature has been updated to include the simplify parameter.
✅ Verification successful
All callers of print_invariants have been updated correctly
The method is called only once in src/main/check.cpp and the call site correctly passes the simplify parameter from verbosity.simplify. The method declaration in the header file also matches the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of print_invariants to ensure they've been updated
# with the new parameter
rg "print_invariants\s*\([^)]*\)" --type cpp
Length of output: 373
src/main/check.cpp (3)
251-252: LGTM: Good refactoring
Extracting verbosity options into a local variable reduces redundancy.
253-253: Verify print_invariants implementation
Ensure the simplify parameter is properly handled in the print_invariants method.
✅ Verification successful
Simplify parameter is correctly implemented in print_invariants
The simplify parameter is properly handled in the print_invariants implementation in src/crab_verifier.cpp. It's passed directly to collect_basic_blocks() which uses it to control the simplification of basic blocks during printing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for print_invariants implementation and its usage of the simplify parameter
ast-grep --pattern 'print_invariants($_,$_,$_) {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Search for print_invariants method with ripgrep for better coverage
rg -U "print_invariants.*\{[\s\S]*?\}" -A 10
# Also search for any print_invariants declarations or calls
rg "print_invariants" -A 2
Length of output: 4765
257-265: Check for duplicate termination messages
The termination message will be printed if either print_failures or print_invariants is true. This could result in duplicate output if both flags are enabled.
src/crab/cfg.hpp (1)
388-388: Document the simplify parameter and verify all call sites.
The signature change looks good but needs:
- Documentation for the
simplifyparameter to explain its behavior - Updates to all call sites due to the breaking change
Run the following script to verify all call sites:
✅ Verification successful
All call sites are updated with the new parameter
The codebase search shows:
- Implementation in
src/asm_cfg.cppcorrectly uses the newsimplifyparameter - Single call site in
src/crab_verifier.cppalready passes thesimplifyparameter - No other call sites found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to collect_basic_blocks to ensure they're updated with the new parameter
# Search for method calls
rg -A 2 "collect_basic_blocks\("
# Search for method references
ast-grep --pattern 'collect_basic_blocks'
Length of output: 1038
src/asm_cfg.cpp (2)
253-262: LGTM: Non-simplify path correctly implements PR objectives
The implementation correctly excludes entry and exit points from collection, aligning with PR requirements.
288-292: Verify behavior when entry instruction is undefined
When the first label is the entry label, we clear m_ts but continue processing. This might lead to unexpected behavior.
✅ Verification successful
Clearing m_ts for entry label is correct behavior
The code is handling a special case correctly. For the entry label, m_ts is cleared before pushing the next label because:
- The entry instruction is undefined by design
- m_ts represents a sequence of labels, and starting with a clean slate (0) for entry block is the intended behavior
- The subsequent push_back ensures proper initialization with the next label
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests or code paths that exercise the entry instruction undefined case
rg -l "entry.*undefined" --type cpp
ast-grep --pattern 'if ($x == cfg.entry_label()) { $$$}'
Length of output: 96
Script:
#!/bin/bash
# Search for tests related to entry label and m_ts
rg -A 5 "entry_label" --type cpp
# Search for m_ts usage in BasicBlock class
ast-grep --pattern 'class BasicBlock {
$$$
m_ts $$$
$$$
}'
# Search for test cases involving BasicBlock and m_ts
rg -A 5 "BasicBlock.*m_ts" --type cpp
Length of output: 4993
Script:
#!/bin/bash
# Look for BasicBlock class definition and m_ts member
rg -A 10 "class BasicBlock" --type cpp --type hpp
# Look for m_ts initialization or usage
rg -A 3 "m_ts\." --type cpp --type hpp
rg -A 3 "m_ts\[" --type cpp --type hpp
# Look for tests involving entry label and basic blocks
rg -A 5 "TEST.*BasicBlock.*entry" --type cpp
Length of output: 264
Script:
#!/bin/bash
# Look for BasicBlock class definition and m_ts member
rg -A 10 "class BasicBlock"
# Look for m_ts initialization or usage
rg -A 3 "m_ts\."
rg -A 3 "m_ts\["
# Look for any tests or code that handles the entry block case
rg -B 3 -A 3 "first_label.*entry"
Length of output: 1331
Signed-off-by: Elazar Gershuni <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
src/asm_cfg.cpp(1 hunks)src/asm_ostream.cpp(3 hunks)src/crab/cfg.hpp(2 hunks)src/crab_verifier.cpp(0 hunks)src/crab_verifier.hpp(2 hunks)src/main/check.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/crab_verifier.cpp
🧰 Additional context used
📓 Learnings (1)
src/asm_cfg.cpp (1)
Learnt from: elazarg
PR: vbpf/ebpf-verifier#805
File: src/asm_cfg.cpp:264-267
Timestamp: 2024-11-24T12:00:20.806Z
Learning: In `collect_basic_blocks`, removing `next_label` requires `std::set` for `worklist`, so replacing it with `std::queue` is not appropriate.
🔇 Additional comments (6)
src/crab_verifier.hpp (1)
70-70: Verify friend function implementation.
Ensure the corresponding implementation in the cpp file properly handles the simplify parameter and maintains const correctness.
✅ Verification successful
Implementation matches declaration and maintains const correctness
The implementation in src/asm_ostream.cpp correctly:
- Preserves const correctness for both
cfgandinvariantsparameters - Matches the parameter order and types from the declaration
- Properly handles the
simplifyparameter by forwarding it toprint_cfg
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation to verify const correctness and parameter handling
rg -A 5 "void print_invariants.*simplify.*Invariants&"
Length of output: 891
src/main/check.cpp (2)
241-244: LGTM: Good refactoring to reduce structure access.
The introduction of the local verbosity variable improves readability by eliminating repeated access to the nested structure.
251-252: LGTM: Consistent with the uniform printing objective.
The print_invariants call correctly uses the simplify flag to maintain uniform printing behavior.
src/crab/cfg.hpp (2)
388-388: Verify implementation of collect_basic_blocks
The method signature change aligns with PR objectives for controlled simplification.
✅ Verification successful
Implementation matches declaration and callers are updated
The collect_basic_blocks implementation in src/asm_cfg.cpp correctly matches the declaration with both cfg and simplify parameters. The only caller in src/asm_ostream.cpp passes both required arguments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify implementation matches declaration
# and all callers are updated
# Find implementation
ast-grep --pattern 'std::set<basic_block_t> collect_basic_blocks(const cfg_t& $_, bool $_) {
$$$
}'
# Find callers
rg -l "collect_basic_blocks" --type cpp
Length of output: 196
Script:
#!/bin/bash
# Find implementation and callers with context
rg "collect_basic_blocks" -B2 -A2 --type cpp
# Find any potential operator<< that might need updating
rg "operator<<.*cfg_t" --type cpp
Length of output: 1149
422-422: Verify callers of removed operator<<
The new print_cfg function replaces the previous stream operator.
✅ Verification successful
Removal of operator<< is safe
Based on the codebase analysis:
- No existing usages of
operator<<withcfg_tfound - The new
print_cfgfunction is properly defined and implemented insrc/asm_ostream.cpp - No stream operations using
cfgobjects were found in files that includecfg.hpp
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and verify all callers are updated
# Find potential callers still using operator<<
rg "operator<<.*cfg_t"
# Find new print_cfg usage
rg "print_cfg.*cfg_t"
Length of output: 405
Script:
#!/bin/bash
# Search for any usage of operator<< with cfg_t in the git history
git log -p | rg "operator<<.*cfg_t"
# Find all usages of cfg_t with stream operations
rg "<<.*cfg" -A 2
Length of output: 19
Script:
#!/bin/bash
# Find all files that include cfg.hpp
rg -l "cfg\.hpp"
# Find any stream operations with cfg in these files
rg "os\s*<<.*cfg|cout\s*<<.*cfg|cerr\s*<<.*cfg" -A 2
Length of output: 350
src/asm_ostream.cpp (1)
208-209: Ensure Consistent Use of Simplify Parameter
In print_invariants, the simplify parameter is passed to print_cfg. Verify that this aligns with the intended behavior and that all calls to print_invariants provide the correct simplify value.
Fix small issues in simplification
Summary by CodeRabbit
New Features
Bug Fixes
Refactor