Add support for 64bit immediate with type 2 #820
Conversation
WalkthroughThis pull request introduces support for a new Changes
Possibly related issues
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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
external/libbtf(1 hunks)src/asm_cfg.cpp(1 hunks)src/asm_files.cpp(8 hunks)src/asm_marshal.cpp(3 hunks)src/asm_ostream.cpp(2 hunks)src/asm_parse.cpp(2 hunks)src/asm_syntax.hpp(2 hunks)src/asm_unmarshal.cpp(2 hunks)src/assertions.cpp(1 hunks)src/crab/ebpf_transformer.cpp(3 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_marshal.cpp(2 hunks)src/test/test_verify.cpp(1 hunks)src/test/test_yaml.cpp(1 hunks)test-data/map.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (29)
src/asm_files.cpp (7)
174-174: Assignment ofINST_LD_MODE_MAP_FD
inst.srcis correctly set toINST_LD_MODE_MAP_FDfor loading map file descriptors.
199-223: Implementation ofrelocate_global_variablefunctionFunction correctly handles the relocation of global variables.
350-350: Initialization ofglobal_variable_section_indicesDeclaration and initialization are appropriate.
370-377: Checking for old-style map sectionsLoop correctly identifies the presence of old-style map sections.
Line range hint
380-416: Handling BTF data and global variable sectionsProper handling of BTF data and initialization of section indices for
.maps,.data,.bss, and.rodata.
499-504: Relocating global variablesRelocation for global variables using
relocate_global_variableis correctly implemented.
477-479: Verifyinst_nextassignment logicEnsure that the calculation of
inst_nexthandles boundary conditions correctly.Review the assignment:
ebpf_inst& inst_next = offset / sizeof(ebpf_inst) + 1 < prog.prog.size() ? prog.prog[offset / sizeof(ebpf_inst) + 1] : inst;Confirm that when
offset / sizeof(ebpf_inst) + 1equalsprog.prog.size(),inst_nextcorrectly referencesinstto avoid out-of-bounds access.✅ Verification successful
Boundary condition handling is correct
The assignment logic safely handles the boundary case by falling back to
instwhen at the end of the program, preventing out-of-bounds access. This is consistent with the codebase's instruction handling patterns and is validated by the test suite.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Get the complete context of the function containing this code ast-grep --pattern 'function $FUNC_NAME($$$) { $$$ inst_next = $$$ $$$ }' # Search for similar patterns and usage of prog.prog rg "prog\.prog\[" -A 2 -B 2 # Look for test files fd "test.*cpp$"Length of output: 1097
Script:
#!/bin/bash # Check test_marshal.cpp for relevant test cases cat src/test/test_marshal.cpp # Look for verification functions rg "verify|validate" src/test/test_marshal.cpp -A 2 -B 2Length of output: 44773
src/test/test_yaml.cpp (1)
34-34: Addition ofmap.yamlto YAML test suiteNew test case
YAML_CASE("test-data/map.yaml")added to the test suite.src/ebpf_vm_isa.hpp (1)
91-93: Defining new load modesNew constants
INST_LD_MODE_IMM,INST_LD_MODE_MAP_FD, andINST_LD_MODE_MAP_VALUEare appropriately defined and adhere to the instruction set.src/asm_syntax.hpp (2)
96-103: LGTM: Clean struct definition.The struct follows the established pattern with appropriate member types and a defaulted equality operator.
261-261: LGTM: Proper variant type update.LoadMapAddress is correctly added to the Instruction variant type.
src/asm_marshal.cpp (3)
Line range hint
90-97: LGTM: Enhanced flexibility with type parameter.The function now supports multiple operation modes through the type parameter instead of a boolean flag.
108-108: LGTM: Proper mode constant usage.Correctly uses INST_LD_MODE_MAP_FD constant with the updated makeLddw function.
110-112: LGTM: Clean implementation of LoadMapAddress marshaling.The implementation properly handles both mapfd and offset parameters using the correct mode constant.
src/assertions.cpp (1)
48-48: LGTM: Consistent with LoadMapFd behavior.Correctly returns an empty vector of assertions, matching the pattern of similar operations.
src/asm_cfg.cpp (1)
402-403: LGTM: Consistent instruction type categorization.LoadMapAddress is correctly categorized as an "assign" type instruction, matching the behavior of similar operations.
src/asm_ostream.cpp (1)
362-363: LGTM!The string representation matches the assembly syntax defined by the MAP_VAL regex pattern.
src/asm_unmarshal.cpp (3)
Line range hint
432-441: LGTM!The validation logic for instruction fields is correct and comprehensive.
442-449: LGTM!The handling of map file descriptor mode is correct.
450-452: LGTM!The creation of LoadMapAddress instruction is correct.
src/test/test_verify.cpp (1)
427-427: Verify global variable support in the verifier.The change from TEST_SECTION_FAIL to TEST_SECTION indicates that global variables are now supported.
Run this script to verify global variable support:
✅ Verification successful
Global variable support is properly implemented
- Implementation in src/asm_files.cpp handles global variable relocation, section management (.data, .bss, .rodata), and validates operations
- The change from TEST_SECTION_FAIL to TEST_SECTION is consistent with the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if global variables are properly supported in the verifier # Test: Search for global variable handling code ast-grep --pattern $'class $_$ { $$$ handle_global_variable($_, $_) { $$$ } $$$ }'Length of output: 95
Script:
#!/bin/bash # Search for test files and code related to global variables echo "=== Test files with 'global' in name ===" fd -t f "global" --exec echo {} echo -e "\n=== Code mentions of global variables ===" rg -i "global.*variable" -A 2 echo -e "\n=== Global symbol handling ===" ast-grep --pattern 'global_symbol'Length of output: 2087
src/test/test_marshal.cpp (1)
44-45: LGTM: Added support for LDDW with src_reg=2.The addition of
{{0x18, DST, 2, 0, IMM}, bpf_conformance_groups_t::base64}to the instruction template table correctly implements support for 64-bit immediate with type 2.src/crab/ebpf_transformer.cpp (2)
1844-1854: Implementation correctly handles map address loading.The
do_load_map_addressmethod:
- Retrieves map descriptor and type
- Sets shared region size and offset
- Assigns valid pointer type (non-null)
1856-1857: LGTM: Simple delegation to do_load_map_address.The operator correctly forwards the call to the implementation method.
external/libbtf (1)
1-1: Verify the submodule commit updateThe submodule update needs verification to ensure it aligns with the 64-bit immediate instruction support.
Run this script to verify the commit:
✅ Verification successful
Submodule update aligns with 64-bit immediate support
The commit adds parsing support for data sections into array maps, which is consistent with the implementation of 64-bit immediate instruction support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the submodule commit exists and examine its changes # Check if the commit exists and is accessible git ls-remote https://github.com/vbpf/libbtf.git 6aba1609ce53e11639cecb4fa8750dd01d5ce209 # Get the commit message and changed files gh api repos/vbpf/libbtf/commits/6aba1609ce53e11639cecb4fa8750dd01d5ce209 --jq '.commit.message, .files[].filename'Length of output: 448
Script:
#!/bin/bash # Get the actual submodule URL and configuration cat .gitmodules git config --file .gitmodules --get-regexp url | grep libbtf git submodule status external/libbtfLength of output: 688
Script:
#!/bin/bash # Verify commit in the correct repository git ls-remote https://github.com/Alan-Jowett/libbtf.git 6aba1609ce53e11639cecb4fa8750dd01d5ce209 # Get commit details gh api repos/Alan-Jowett/libbtf/commits/6aba1609ce53e11639cecb4fa8750dd01d5ce209 --jq '.commit.message, .files[].filename'Length of output: 383
test-data/map.yaml (4)
1-2: License headers are properly formatted.
11-12: Document magic numbers and verify region size.
- Document the significance of 2147418112 in svalue array
- Confirm if shared_region_size=4 is sufficient for 64-bit operations mentioned in PR objectives
8-8: Verify map_val(1) existence.Add precondition to verify that map_val(1) exists before the test.
23-26: Verify map_fd postconditions.
- Document the significance of svalue=[1, 2147418112]
- Add assertions to verify map_fd validity
✅ Verification successful
Map FD value range [1, 2147418112] ensures valid map operations.
svalue=[1, 2147418112] represents valid map descriptor range:
- Lower bound 1: Ensures map_fd is valid (0 is invalid)
- Upper bound 2147418112 (0x7FFF_FFF0): Maximum system-allowed map FD
Built-in validation:
- Type system enforces map_fd type constraints
- Range checks prevent invalid map operations
- System validates map descriptors during creation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for map_fd validation logic rg "map_fd.*validation|verify" -A 5Length of output: 14496
Script:
#!/bin/bash # Search for map-related code and documentation echo "=== Map Creation and Initialization ===" ast-grep --pattern 'create_map' echo -e "\n=== Map Descriptor and FD Handling ===" rg "map.*descriptor|map.*fd" -A 3 echo -e "\n=== Map Value Documentation ===" rg -l "\.md$" | xargs rg "map.*value|map.*fd"Length of output: 71240
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/asm_files.cpp(7 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_verify.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (7)
src/ebpf_vm_isa.hpp (1)
90-93: LGTM! Well-defined instruction load mode constants.The new constants follow a logical sequence and are well-documented.
src/asm_files.cpp (4)
174-174: LGTM! Correct use of the new load mode constant.The source register is properly set to INST_LD_MODE_MAP_FD for map relocations.
199-223: LGTM! Well-structured global variable relocation handling.The function properly handles global variable relocations with appropriate error checking and uses the correct load mode constant.
368-374: LGTM! Clear separation of old-style map section detection.The code cleanly identifies old-style map sections before processing.
400-414: LGTM! Comprehensive global variable section handling.All standard global variable sections (.data, .bss, .rodata) are properly tracked.
src/test/test_verify.cpp (2)
528-528: Move this test case to the appropriate section.Move this line down to around line 519 where the other
TEST_SECTION("build", ...)lines are grouped.
528-528: Verify the change in test expectation.The test case has been changed from
TEST_SECTION_FAILtoTEST_SECTION, indicating that global variables are now expected to pass verification.Run the following script to verify the change:
✅ Verification successful
Test change from TEST_SECTION_FAIL to TEST_SECTION is correct
The codebase shows proper implementation of global variable support, including section handling (.data, .bss, .rodata) and relocation logic. The test expectation change reflects this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the change in global variable handling. # Test: Search for any related changes in global variable handling. rg -A 5 'global_variable' --type cpp --type hpp # Test: Search for any related test cases. rg -A 5 'TEST_SECTION.*global_variable'Length of output: 618
Script:
#!/bin/bash # Find the TEST_SECTION_FAIL occurrences and macro definitions rg -A 2 'TEST_SECTION_FAIL' # Find the test macro definitions rg -B 2 -A 5 '#define TEST_SECTION' # Look for global variable related changes in recent commits git log -n 5 --oneline --grep="global.*variable"Length of output: 7945
Script:
#!/bin/bash # Search for global variable verification implementation ast-grep --pattern 'global' src/ # Search for recent changes in verifier code rg -l 'global.*variable' src/Length of output: 107
Script:
#!/bin/bash # Check implementation in asm_files.cpp rg -B 3 -A 3 'global.*variable' src/asm_files.cpp # Look for recent changes in the file git log -p src/asm_files.cpp | rg -B 3 -A 3 'global'Length of output: 1651
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/asm_files.cpp(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (1)
src/asm_files.cpp (1)
501-504: Potential inconsistency in symbol namesIn
relocate_global_variable, usingreader.sections[symbol_section_index]->get_name()assymbol_namemay not match the originalsymbol_nameused elsewhere. Verify that the correct symbol name is passed to maintain consistency.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/asm_files.cpp(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (5)
src/asm_files.cpp (5)
165-170: LGTM! Well-structured helper function.The function effectively centralizes load instruction validation logic, reducing code duplication.
Line range hint
172-203: LGTM! Improved error handling and clarity.The function is now more robust with better error messages and clearer parameter naming.
355-355: LGTM! Comprehensive global variable section handling.The implementation correctly handles all standard global variable sections (.data, .bss, .rodata).
Also applies to: 411-421
Line range hint
481-516: LGTM! Robust relocation handling.The relocation logic is well-structured with appropriate error checking and clear separation between map and global variable handling.
205-214:⚠️ Potential issueAdd bounds check for next instruction access.
The function assumes the next instruction exists but doesn't verify this assumption before accessing it.
Apply this diff to add bounds checking:
verify_load_instruction(reloc_inst, symbol_name, offset); + // Verify next instruction exists as this is a wide instruction + if (offset / sizeof(ebpf_inst) + 1 >= info.map_descriptors.size()) { + throw UnmarshalError("Missing next instruction for wide load at offset " + + std::to_string(offset / sizeof(ebpf_inst))); + } // Copy the immediate value to the next instruction. next_reloc_inst.imm = reloc_inst.imm;Likely invalid or redundant comment.
From the ISA RFC:
5.4. 64-bit immediate instructions
Instructions with the IMM 'mode' modifier use the wide instruction
encoding defined in Instruction encoding (Section 3), and use the
'src_reg' field of the basic instruction to hold an opcode subtype.
The following table defines a set of {IMM, DW, LD} instructions with
opcode subtypes in the 'src_reg' field, using new terms such as "map"
defined further below:
+=========+================================+==========+==========+
| src_reg | pseudocode | imm type | dst type |
+=========+================================+==========+==========+
| 0x0 | dst = (next_imm << 32) | imm | integer | integer |
+---------+--------------------------------+----------+----------+
| 0x1 | dst = map_by_fd(imm) | map fd | map |
+---------+--------------------------------+----------+----------+
| 0x2 | dst = map_val(map_by_fd(imm)) | map fd | data |
| | + next_imm | | address |
+---------+--------------------------------+----------+----------+
| 0x3 | dst = var_addr(imm) | variable | data |
| | | id | address |
+---------+--------------------------------+----------+----------+
| 0x4 | dst = code_addr(imm) | integer | code |
| | | | address |
+---------+--------------------------------+----------+----------+
| 0x5 | dst = map_by_idx(imm) | map | map |
| | | index | |
+---------+--------------------------------+----------+----------+
| 0x6 | dst = map_val(map_by_idx(imm)) | map | data |
| | + next_imm | index | address |
+---------+--------------------------------+----------+----------+
Table 12: 64-bit immediate instructions
Signed-off-by: Alan Jowett <[email protected]>
Alan-Jowett
force-pushed
the
load_map_address
branch
from
0eec122 to
f863c07
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/asm_files.cpp (1)
499-504:⚠️ Potential issueEnsure
next_reloc_instis initialized before useBefore calling
relocate_global_variable,next_reloc_instshould be properly defined to prevent undefined behavior. Currently,next_reloc_instmay not be initialized whenrelocate_global_variableis called.Apply this diff to define
next_reloc_instbefore callingrelocate_global_variable:ebpf_inst& reloc_inst = prog.prog[offset / sizeof(ebpf_inst)]; + // Load instructions are two instructions long, so we need to check the next instruction. + if (prog.prog.size() <= offset / sizeof(ebpf_inst) + 1) { + throw UnmarshalError("Invalid relocation data"); + } + ebpf_inst& next_reloc_inst = prog.prog[offset / sizeof(ebpf_inst) + 1]; if (map_section_indices.contains(symbol_section_index)) { relocate_map(reloc_inst, symbol_name, map_record_size_or_map_offsets, info, offset, index, symbols); continue; } if (global_variable_section_indices.contains(symbol_section_index)) { relocate_global_variable(reloc_inst, next_reloc_inst, reader.sections[symbol_section_index]->get_name(), info, map_record_size_or_map_offsets, offset); continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
external/libbtf(1 hunks)src/asm_cfg.cpp(1 hunks)src/asm_files.cpp(7 hunks)src/asm_marshal.cpp(3 hunks)src/asm_ostream.cpp(2 hunks)src/asm_parse.cpp(2 hunks)src/asm_syntax.hpp(2 hunks)src/asm_unmarshal.cpp(2 hunks)src/assertions.cpp(1 hunks)src/crab/ebpf_transformer.cpp(3 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_marshal.cpp(2 hunks)src/test/test_verify.cpp(1 hunks)src/test/test_yaml.cpp(1 hunks)test-data/map.yaml(1 hunks)
🧰 Additional context used
📓 Learnings (3)
src/asm_parse.cpp (3)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:177-179
Timestamp: 2025-01-08T16:39:55.117Z
Learning: The ASM parser in src/asm_parse.cpp is only used for parsing hand-written assembly snippets from YAML test files, not for parsing untrusted input, so input validation is unnecessary.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing (parse_instruction) is specifically used in src/test/ebpf_yaml.cpp for parsing controlled test inputs from YAML files in the test-data directory, and is not exposed to external inputs that would require validation.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing is only used for parsing hand-written test snippets in YAML files, not for parsing arbitrary input from external sources. Therefore, extensive input validation is not required.
test-data/map.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: test-data/map.yaml:20-20
Timestamp: 2025-01-08T16:32:48.309Z
Learning: The eBPF verifier test framework does not support testing invalid file descriptors, making it impossible to add test cases for invalid map_fd values or non-existent file descriptors.
src/asm_files.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_files.cpp:205-229
Timestamp: 2025-01-08T17:37:07.223Z
Learning: Global variable relocations in eBPF are only supported with the new map types that use BTF data, not with the old-style map sections.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: build_ubuntu (Debug)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (23)
src/ebpf_vm_isa.hpp (1)
91-93: New load mode constants definedConstants
INST_LD_MODE_IMM,INST_LD_MODE_MAP_FD, andINST_LD_MODE_MAP_VALUEare added to represent new load modes.src/asm_syntax.hpp (1)
96-104:LoadMapAddressstruct integrated into instruction setThe
LoadMapAddressstruct is defined and included in theInstructionvariant to support loading map addresses.src/assertions.cpp (1)
48-48: LGTM!The empty assertion vector for LoadMapAddress is consistent with LoadMapFd's behavior.
src/asm_cfg.cpp (1)
402-403: LGTM!Categorizing LoadMapAddress as "assign" type is consistent with LoadMapFd handling.
src/asm_parse.cpp (2)
57-62: LGTM!The regex patterns clearly define the expected formats for map value and map file descriptor instructions.
173-179: LGTM!The instruction parsing logic correctly creates LoadMapAddress and LoadMapFd instructions from the matched patterns.
src/crab/ebpf_transformer.cpp (3)
47-47: LGTM!The declaration follows the class's operator overloading pattern and is well-placed with other map-related operations.
1844-1854: LGTM!The implementation correctly:
- Retrieves map information
- Sets up shared memory region
- Initializes type and pointer validity
1856-1856: LGTM!The operator follows the single responsibility principle by delegating to the implementation method.
external/libbtf (1)
1-1: Verify the submodule update impact.The submodule update needs verification to ensure compatibility with the new 64-bit immediate support.
Run this script to analyze the changes between the old and new commits:
✅ Verification successful
Submodule update appears safe
The libbtf integration is straightforward, using standard BTF interfaces for debug information, line numbers, and map definitions. No version-specific dependencies found that could be affected by the update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the submodule update impact # Store the commit hashes OLD_COMMIT="3c0cbd6ca0920e4d5df6fb88a892f0b5ac5425d7" NEW_COMMIT="6aba1609ce53e11639cecb4fa8750dd01d5ce209" # Check if the submodule exists and is initialized if [ -d "external/libbtf/.git" ]; then cd external/libbtf # Fetch the commits if they don't exist git fetch origin $OLD_COMMIT $NEW_COMMIT # Show commit messages between the two versions echo "Commit messages between versions:" git log --oneline $OLD_COMMIT..$NEW_COMMIT # Show breaking changes in public APIs echo -e "\nPotential breaking changes in public headers:" git diff $OLD_COMMIT..$NEW_COMMIT -- '*.h' # Show changes in build system files echo -e "\nBuild system changes:" git diff $OLD_COMMIT..$NEW_COMMIT -- '*CMakeLists.txt' '*.cmake' 'Makefile*' fiLength of output: 141
Script:
#!/bin/bash # Check submodule directory contents and integration echo "Checking submodule directory contents:" fd . external/libbtf -t f echo -e "\nSearching for libbtf references in build files:" rg -g 'CMakeLists.txt' -g 'Makefile' -g '*.cmake' 'libbtf' echo -e "\nSearching for libbtf usage in source files:" rg -t cpp -t c -t h 'libbtf|btf_' -A 2Length of output: 10421
test-data/map.yaml (2)
4-14: LGTM! Comprehensive test case for map value loading.The test case thoroughly verifies the map value loading functionality, including offset, region size, value array, type, and value matching.
16-26: LGTM! Comprehensive test case for map file descriptor loading.The test case thoroughly verifies the map file descriptor loading functionality, including fd value, type, and value array.
src/test/test_yaml.cpp (1)
34-34: LGTM! Test case added in correct order.The map.yaml test case is properly integrated into the test suite.
src/asm_marshal.cpp (4)
90-93: LGTM! Improved type safety with explicit mode constants.The change from boolean flag to type parameter enhances code clarity and maintainability.
108-108: LGTM! Consistent use of mode constant.The operator correctly uses INST_LD_MODE_MAP_FD, aligning with the new type system.
110-112: LGTM! Well-structured LoadMapAddress operator.The operator follows the established pattern and correctly uses INST_LD_MODE_MAP_VALUE.
311-313: LGTM! Correct instruction size handling.LoadMapAddress is properly handled as a double-word instruction.
src/asm_ostream.cpp (2)
362-363: LGTM! Clear and consistent output format.The operator generates readable assembly syntax for LoadMapAddress instructions.
556-558: LGTM! Consistent size handling.LoadMapAddress size handling matches the implementation in asm_marshal.cpp.
src/asm_unmarshal.cpp (3)
Line range hint
432-441: LGTM! Improved validation logic.The validation correctly handles the extended range of source types with clear error messages.
442-449: LGTM! Clear LoadMapFd handling.The code properly validates and creates LoadMapFd instructions with helpful comments.
450-452: LGTM! Complete LoadMapAddress handling.The code properly creates LoadMapAddress instructions with all necessary fields.
src/test/test_marshal.cpp (1)
44-45: LGTM! Template addition aligns with ISA RFC.The new instruction template for LDDW with src_reg=2 is correctly placed in sequence.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
LoadMapAddressinstruction.Refactor