Simplify/remove templates #776
Conversation
* Use linear_expression_t where both numbers and variables are needed. * tidy, remove unused methods Signed-off-by: Elazar Gershuni <[email protected]>
It helps with the "Mark directory as excluded" in CLion. Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
WalkthroughThe pull request introduces changes across multiple files, primarily focusing on code structure improvements, type simplifications, and adjustments to method signatures. Notable alterations include updates to file paths for the CLI11 library, enhancements to error handling, and modifications to method signatures to improve clarity and maintainability. The changes also involve the removal of unnecessary Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🔇 Additional comments (4)src/spec_type_descriptors.hpp (1)
The struct implementation ensures:
This makes it a robust key type for the cache map in Note: The improvement from tuple to struct and documentation suggestions were already covered in previous reviews. src/crab/add_bottom.hpp (1)
The change from template syntax to src/asm_files.cpp (2)
The addition of
While the const additions are good, let's verify there are no callers that expect to modify these parameters. ✅ Verification successfulLet me analyze the function declaration and its usage across the codebase to verify if there are any potential issues with the const changes. Let me try a different approach to check the implementation and usage. The const parameter changes are safe to apply The code analysis shows that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for all calls to create_map_crab to verify parameter usage
rg -l "create_map_crab\s*\(" | while read -r file; do
echo "=== Checking $file ==="
rg -A 2 "create_map_crab\s*\(" "$file"
done
Length of output: 1068 Script: #!/bin/bash
# Let's check the implementation details and parameter handling
ast-grep --pattern 'create_map_crab($$$) {
$$$
}'
Length of output: 53 Script: #!/bin/bash
# Let's check the full content of the relevant files
echo "=== Content of src/asm_files.cpp around create_map_crab ==="
rg -B 5 -A 10 "create_map_crab" src/asm_files.cpp
echo -e "\n=== Content of src/linux/linux_platform.cpp around create_map_crab ==="
rg -B 5 -A 5 "create_map_crab" src/linux/linux_platform.cpp
echo -e "\n=== Content of src/crab_verifier.hpp around create_map_crab ==="
rg -B 5 -A 5 "create_map_crab" src/crab_verifier.hpp
Length of output: 2462 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: 9
🧹 Outside diff range comments (6)
src/main/check.cpp (2)
Line range hint
154-158: Consider using platform-specific compilation instead of runtime check.The runtime check for Linux platform could be replaced with platform-specific compilation to make the code more maintainable and prevent dead code in non-Linux builds.
Consider using:
-#if !__linux__ - if (domain == "linux") { - std::cerr << "error: linux domain is unsupported on this machine\n"; - return 64; - } -#endif +#ifdef __linux__ + // Linux-specific code here +#else + if (domain == "linux") { + std::cerr << "error: linux domain is unsupported on this machine\n"; + return EXIT_UNSUPPORTED_PLATFORM; + } +#endif
Line range hint
71-267: Consider enhancing domain handling and error codes.A few suggestions to improve the code:
- Consider using an enum class for domains instead of strings to prevent typos and enable compile-time checks:
enum class VerifierDomain { ZONE_CRAB, LINUX, STATS, CFG };
- Consider defining named constants for exit codes to improve maintainability:
constexpr int EXIT_SUCCESS = 0; constexpr int EXIT_FAILURE = 1; constexpr int EXIT_INVALID_PROGRAM = 64;Would you like me to provide a complete implementation for these improvements?
src/asm_ostream.cpp (1)
Line range hint
187-358: Consider marking member functions as const.All member functions in
InstructionPrinterVisitorappear to be pure visitors that don't modify the struct's state. Consider marking them asconstto better express intent and enable compiler optimizations.Apply this change to the struct definition and its member functions:
struct InstructionPrinterVisitor { std::ostream& os_; - void visit(const auto& item) { std::visit(*this, item); } + void visit(const auto& item) const { std::visit(*this, item); } - void operator()(Undefined const& a) { os_ << "Undefined{" << a.opcode << "}"; } + void operator()(Undefined const& a) const { os_ << "Undefined{" << a.opcode << "}"; } - void operator()(LoadMapFd const& b) { os_ << b.dst << " = map_fd " << b.mapfd; } + void operator()(LoadMapFd const& b) const { os_ << b.dst << " = map_fd " << b.mapfd; } // ... Apply const to all other member functions ... };src/crab/ebpf_domain.cpp (3)
Line range hint
1949-1984: Consider Handling All Widths Consistently indo_store_stack.In the
do_store_stackfunction, the code provides specific handling whenwidth == 8, including assignments for various data kinds. For other widths, particularly when(width == 1 || width == 2 || width == 4) && type_inv.get_type(m_inv, val_type) == T_NUM, onlysvaluesanduvaluesare assigned, and other data kinds are havoced. Consider whether it is necessary to handle other data kinds (e.g.,ctx_offsets,map_fds, etc.) for widths other than 8 to ensure consistent behavior.
Line range hint
1949-1984: Refactor Suggestion: Reduce Code Duplication Betweendo_store_stackanddo_mem_store.Both
do_store_stackanddo_mem_storefunctions contain similar logic for handling memory store operations based onwidthandval_type. To enhance maintainability and reduce code duplication, consider extracting common code into a shared helper function.
Line range hint
1949-2080: Ensure Consistent Parameter Types and Names Betweendo_store_stackanddo_mem_store.The functions
do_store_stackanddo_mem_storehave similar parameters but slightly different in order and naming conventions. For example,do_store_stackhasNumAbsDomain& invas the first parameter, whereasdo_mem_storehas it implicitly throughthis->m_inv. Consider aligning the parameter lists of these functions to improve readability and consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (18)
scripts/.check-license.ignore(1 hunks)src/asm_cfg.cpp(2 hunks)src/asm_ostream.cpp(2 hunks)src/crab/add_bottom.hpp(1 hunks)src/crab/cfg.hpp(5 hunks)src/crab/ebpf_domain.cpp(2 hunks)src/crab/ebpf_domain.hpp(3 hunks)src/crab/type_domain.cpp(2 hunks)src/crab/type_domain.hpp(1 hunks)src/crab_utils/adapt_sgraph.hpp(2 hunks)src/crab_utils/debug.hpp(1 hunks)src/crab_utils/graph_ops.hpp(7 hunks)src/crab_verifier.cpp(3 hunks)src/main/check.cpp(1 hunks)src/main/run_yaml.cpp(1 hunks)src/main/utils.hpp(1 hunks)src/spec_type_descriptors.hpp(1 hunks)src/test/conformance_check.cpp(1 hunks)
🔇 Additional comments (33)
src/main/utils.hpp (1)
4-4: LGTM: Good addition of header guard.
The addition of #pragma once is a good practice for header files to prevent multiple inclusions.
src/main/run_yaml.cpp (1)
9-9: LGTM! Include path update aligns with PR objectives.
The change to use CLI11/CLI11.hpp is consistent with the PR's goal of facilitating CLion's "Mark directory as excluded" feature.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Include path update is consistently applied across the codebase
The verification confirms that:
- No files are using the old direct include path
CLI11.hpp - The new include path
CLI11/CLI11.hppis consistently used in all relevant files:- src/main/check.cpp
- src/main/run_yaml.cpp
- src/test/conformance_check.cpp
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CLI11.hpp include path consistency
# Test: Search for any remaining old-style includes. Expect: No matches for direct CLI11.hpp includes
rg -l 'include.*"CLI11\.hpp"'
# Test: Verify new include pattern is used consistently
rg -l 'include.*"CLI11/CLI11\.hpp"'
Length of output: 141
src/crab_utils/debug.hpp (1)
35-35: LGTM! Good cleanup of redundant inline specifier.
The removal of the inline specifier is appropriate here since template functions are implicitly inline. This change makes the code cleaner without affecting the behavior, as modern compilers make their own intelligent inlining decisions regardless of the inline keyword.
src/crab/type_domain.hpp (1)
43-43: LGTM: Enhanced type checking capabilities.
The change to accept linear_expression_t expands the method's capability to handle complex expressions while maintaining const-correctness.
Let's verify that this change is consistently applied across the codebase:
✅ Verification successful
The previous command might have failed because the pattern was too specific. Let's try a broader search to:
- Find all usages of get_type to verify the parameter types
- Look for any variable_t usage in the codebase to ensure complete migration
Based on the search results, I can now verify the change to get_type method. The search shows:
- The method has two overloads in type_domain.hpp:
type_encoding_t get_type(const NumAbsDomain& inv, const linear_expression_t& v) const;
type_encoding_t get_type(const NumAbsDomain& inv, const Reg& r) const;- The implementation in type_domain.cpp shows both overloads are used consistently:
type_encoding_t TypeDomain::get_type(const NumAbsDomain& inv, const linear_expression_t& v) const {
const auto res = inv.eval_interval(v).singleton();
if (!res) {
return T_TOP;
}
return static_cast<type_encoding_t>(res->cast_to<int>());
}
type_encoding_t TypeDomain::get_type(const NumAbsDomain& inv, const Reg& r) const {
const auto res = inv[reg_pack(r).type].singleton();
if (!res) {
return T_TOP;
}
return static_cast<type_encoding_t>(res->cast_to<int>());
}- All usages of
get_typein the codebase are consistent with these overloads, using eitherlinear_expression_torRegparameters.
LGTM: The get_type method change is consistently implemented.
The method signature change to accept linear_expression_t is properly implemented with both overloads maintained, and all usages in the codebase are consistent with this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct get_type calls with variable_t
# to ensure complete migration to linear_expression_t
ast-grep --pattern 'get_type($_, $_)' | grep -B2 -A2 'variable_t'
Length of output: 65
Script:
#!/bin/bash
# Let's try a more comprehensive search for get_type usage patterns
rg "get_type\(" -A 2 -B 2
# Also search for any remaining variable_t usage near get_type
rg "variable_t" -A 2 -B 2 "src/crab"
Length of output: 92125
src/test/conformance_check.cpp (2)
14-14: Verify build configuration updates.
Since the CLI11 header location has changed, ensure that build configurations (CMake, Make, etc.) are updated to include the new subfolder in the include paths.
#!/bin/bash
# Description: Check build files for include path configurations
# Expect: Updated include paths in build configurations
echo "Checking build configuration files for include paths:"
fd -e cmake -e txt -e make -e mk | xargs rg -l 'CLI11'14-14: LGTM! Verify consistent updates across codebase.
The include path change aligns with the PR objective of moving CLI11.hpp to a subfolder.
Let's verify that all CLI11 includes are updated consistently:
✅ Verification successful
The previous output showed the new includes but had some script errors. Let's run a cleaner verification to ensure we catch all possible include variations and confirm there are no remaining old-style includes.
Include path changes are consistent across the codebase
The verification confirms that all CLI11 includes have been updated to use the new path CLI11/CLI11.hpp. No instances of the old include style CLI11.hpp remain in the codebase. All three files using CLI11 (src/test/conformance_check.cpp, src/main/check.cpp, and src/main/run_yaml.cpp) are using the consistent new include path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old-style CLI11 includes
# Expect: No matches for the old include style
# Search for old include style
echo "Checking for old CLI11 includes:"
rg '#include\s+"CLI11\.hpp"'
# Search for new include style for reference
echo -e "\nConfirming new CLI11 includes:"
rg '#include\s+"CLI11/CLI11\.hpp"'
Length of output: 533
Script:
#!/bin/bash
# Let's verify there are no old-style includes remaining
# Using a more precise search for "#include" with CLI11.hpp
echo "Checking for any old-style CLI11 includes:"
rg -n '#include.*"CLI11\.hpp"' --type cpp
echo -e "\nVerifying all CLI11 includes are consistent:"
rg -n '#include.*CLI11' --type cpp
Length of output: 441
src/crab_verifier.cpp (3)
99-99: LGTM! Performance improvement in range-based for loop.
The addition of & in the structured binding avoids unnecessary copying of map elements, which is particularly beneficial when dealing with invariants that may contain complex state.
165-166: LGTM! Well-designed parameter passing strategy.
The function correctly takes program_info by value to enable moving it into the global state, while efficiently passing other parameters by const reference or pointer.
187-188: LGTM! Consistent parameter ordering and proper optional parameter handling.
The function signature is well-designed with:
- Consistent parameter ordering with
get_ebpf_report - Proper use of const references for input parameters
- Appropriate use of nullable pointers for optional parameters
src/crab/ebpf_domain.hpp (3)
169-169: LGTM: Simplified type references
The removal of the domains:: namespace prefix from NumAbsDomain improves code readability while maintaining the same functionality.
Also applies to: 172-173
203-203: LGTM: Consistent type simplification
The removal of the domains:: namespace prefix from the m_inv member variable maintains consistency with the method signature changes.
188-190: Verify callers of store methods
The transition to linear_expression_t for value parameters provides more flexibility and aligns with the PR objectives. However, we should verify that all callers have been updated accordingly.
Also applies to: 192-193
✅ Verification successful
Based on the search results, I can now generate the final response as all callers have been found and analyzed:
All callers properly updated to match new parameter types
The verification shows that all callers of both do_store_stack and do_mem_store have been correctly updated to use linear_expression_t parameters:
do_store_stackcalls inebpf_domain.cppuse proper linear expressions (e.g.,base_addr + offset,val_type,val_svalue,val_uvalue)do_mem_storecalls correctly pass linear expressions for type and values (e.g.,data_reg.type,data_reg.svalue,to_signed(imm))
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to do_store_stack and do_mem_store to verify parameter updates
echo "Checking do_store_stack calls:"
rg "do_store_stack\(" -A 2
echo -e "\nChecking do_mem_store calls:"
rg "do_mem_store\(" -A 2
Length of output: 2666
src/main/check.cpp (1)
18-18: LGTM: Include path update aligns with project objectives.
The change to use CLI11/CLI11.hpp instead of CLI11.hpp is consistent with the PR objective to relocate CLI11.hpp to a subfolder. This structural change improves IDE integration by facilitating the "Mark directory as excluded" feature in CLion.
Let's verify that this include path change is consistent across the codebase:
✅ Verification successful
Include path change is consistent across the codebase
The verification confirms that:
- All CLI11 includes in the codebase use the new path format
CLI11/CLI11.hpp - The header exists at the correct location
external/CLI11/CLI11.hpp - The change is consistently applied across all files that use CLI11:
- src/main/check.cpp
- src/test/conformance_check.cpp
- src/main/run_yaml.cpp
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old-style CLI11 includes that need updating
# and verify the new include path exists
# Check for any remaining old-style includes
echo "Checking for any remaining old-style CLI11.hpp includes:"
rg -l "include\s+[\"<]CLI11\.hpp[\">]"
# Verify the new include path exists
echo -e "\nVerifying new CLI11 directory structure:"
fd -t f "CLI11.hpp" -p "CLI11/"
Length of output: 382
Script:
#!/bin/bash
# Let's try a different approach to verify the CLI11 header location and usage
# First, let's find all CLI11.hpp files in the repository
echo "Finding all CLI11.hpp files:"
fd "CLI11.hpp"
# Then, let's check all include statements for CLI11
echo -e "\nChecking all CLI11 include patterns:"
rg -p "include.*CLI11" --type cpp --type hpp
# Let's also check the directory structure around any CLI11.hpp
echo -e "\nChecking parent directories of CLI11.hpp:"
fd "CLI11.hpp" -x dirname {}
Length of output: 417
Script:
#!/bin/bash
# Let's check all includes of CLI11 without file type restrictions
echo "Checking all CLI11 include patterns:"
rg -p "^#include.*CLI11"
# Let's also verify the content of src/main/check.cpp specifically
echo -e "\nChecking src/main/check.cpp content:"
cat src/main/check.cpp
Length of output: 11273
src/crab_utils/adapt_sgraph.hpp (1)
259-259: LGTM: Template simplification
The removal of the typename keyword aligns with modern C++ practices while maintaining the same functionality.
src/asm_cfg.cpp (2)
265-265: LGTM: Direct construction of Assume instruction
The change from templated insertion to direct construction aligns with the PR's goal of template simplification while maintaining the same functionality.
373-374: LGTM: Improved lambda formatting
The lambda formatting enhances readability while maintaining the same functionality of inserting loop counters at entry points.
src/asm_ostream.cpp (1)
191-191: LGTM! Template simplification improves readability.
The change from explicit template syntax to abbreviated function template syntax (auto) makes the code more concise while maintaining the same functionality.
src/crab/cfg.hpp (6)
46-49: LGTM: Iterator type definitions are correctly simplified.
The removal of unnecessary typename keywords aligns with modern C++ practices while maintaining the same functionality.
71-71: LGTM: Efficient constructor implementation.
The use of std::move optimizes the constructor by avoiding unnecessary copying of the label parameter.
168-168: LGTM: Consistent iterator type definitions.
The iterator type definitions are properly simplified and maintain consistency with the changes in basic_block_t.
Also applies to: 170-171
228-229: LGTM: Type definitions are properly simplified.
The type definitions are correctly simplified while maintaining proper const-correctness and consistency with the rest of the codebase.
Also applies to: 236-236
243-244: LGTM: Iterator types are properly defined.
The iterator type definitions are correctly simplified and maintain consistency with the container types.
480-480: LGTM: Type definitions are consistent and well-structured.
The type definitions properly maintain relationships with their base classes while following the simplified syntax pattern.
Also applies to: 483-483, 487-490
src/crab_utils/graph_ops.hpp (5)
107-107: LGTM: Type alias simplification
The removal of the typename keyword is safe as the dependent type is clearly an iterator type.
140-140: LGTM: Type alias simplification
The removal of the typename keyword is safe as edge_ref is clearly a dependent type within ItG.
166-166: LGTM: Type alias simplification
The removal of the typename keyword is safe as iterator is clearly a type within RG.
205-205: LGTM: Type alias simplification
The removal of the typename keyword is safe as iterator is clearly a type within RG.
498-502: LGTM: Multiple type alias simplifications
The removal of typename keywords from multiple type aliases is safe and consistent:
WeightfromGr::Weightvert_idfromgraph_t::vert_idmut_val_ref_tfromgraph_t::mut_val_ref_t
These changes align with modern C++ practices and improve code readability.
src/crab/type_domain.cpp (4)
224-225: Ensure correct usage of get_type overloads
The get_type method overload with the Reg& parameter remains. Verify that existing code uses the appropriate overloads as per the updated method signatures.
238-239: Verify all calls to has_type match the new method signature
The has_type method now accepts a linear_expression_t for the variable parameter. Ensure that all calls to has_type have been updated to match this signature.
Run the following script to find all calls to has_type that may need updating:
✅ Verification successful
Based on the ripgrep results, I can now generate the final response:
has_type method exists in two overloaded forms and all calls match the signatures
The codebase shows that TypeDomain::has_type exists in two valid overloaded forms:
has_type(const NumAbsDomain& inv, const linear_expression_t& v, type_encoding_t type)has_type(const NumAbsDomain& inv, const Reg& r, type_encoding_t type)
All calls to has_type in the codebase match one of these two signatures, with appropriate argument types. No updates are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `TypeDomain::has_type` that may need updating.
# Search for function calls to `has_type` with any number of arguments.
ast-grep --lang cpp --pattern 'has_type($_, $$$)' .
Length of output: 199
Script:
#!/bin/bash
# Search for function calls to has_type using ripgrep for better coverage
rg "has_type\(" --type cpp -A 2
# Also search for the TypeDomain class definition and its has_type method
rg "class TypeDomain" --type cpp -A 10
Length of output: 5599
216-217: Verify usage of get_type method with updated parameters
The get_type method now accepts a linear_expression_t parameter instead of previous types. Ensure all calls to get_type have been updated accordingly.
Run the following script to find all calls to get_type that may need updating:
✅ Verification successful
The previous ast-grep pattern had syntax issues. Let's try with ripgrep to find the usage patterns and class definition for better context.
No changes needed for get_type method calls
The codebase search reveals that the get_type method has two overloaded versions in type_domain.hpp:
get_type(const NumAbsDomain& inv, const linear_expression_t& v)get_type(const NumAbsDomain& inv, const Reg& r)
All existing calls to get_type in ebpf_domain.cpp are using the correct parameter types and don't need any updates:
get_type(inv, reg_pack(reg).type)- uses the linear_expression_t versionget_type(m_inv, val_type)- uses the linear_expression_t versionget_type(m_inv, std::get<Reg>(bin.v))- uses the Reg versionget_type(m_inv, dst.type)- uses the linear_expression_t version
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `TypeDomain::get_type` that may need updating.
# Search for function calls to `get_type` with any number of arguments.
ast-grep --lang cpp --pattern 'get_type($_, $$$)' .
Length of output: 199
Script:
#!/bin/bash
# Search for calls to get_type method in cpp files
rg -U "get_type\([^)]+\)" --type cpp -A 2
# Also search for TypeDomain class definition and its get_type method
rg -U "class TypeDomain" --type cpp -A 10
Length of output: 2038
206-207: Verify all calls to assign_type are updated to the new signature
The assign_type method now accepts const std::optional<variable_t> and const linear_expression_t& as parameters. Ensure that all calls to assign_type throughout the codebase have been updated to match this new signature.
Run the following script to locate all calls to assign_type that may need updating:
src/crab/ebpf_domain.cpp (1)
2049-2050: Approval: Correct Use of data_reg in do_mem_store Call.
The call to do_mem_store appropriately uses data_reg.type, data_reg.svalue, data_reg.uvalue, and data_reg, matching the expected parameter types. This use is correct and aligns with the updated function signature.
Signed-off-by: Elazar Gershuni <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Style
Tests