Fix ebpf_base.h usability in C #777
Conversation
Signed-off-by: Dave Thaler <[email protected]>
WalkthroughThe changes in this pull request involve modifying the Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant Preprocessor
participant Code
Compiler->>Preprocessor: Read source file
Preprocessor->>Code: Replace constants with values
Code->>Compiler: Compile code with replaced values
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/ebpf_base.h(1 hunks)
🔇 Additional comments (2)
src/ebpf_base.h (2)
50-50: LGTM! Proper use of parentheses in macro definition.
The change successfully addresses C compatibility while maintaining the correct calculation. The parentheses around the expression ensure proper evaluation order.
43-50: Verify macro usage and potential conflicts.
Let's verify that these changes resolve the C compatibility issues and don't introduce naming conflicts:
✅ Verification successful
Let me gather more information about the usage of these macros in the codebase.
Macros are used consistently and no conflicts found
The verification shows these macros are used appropriately throughout the codebase:
- The stack size constants are used consistently in stack-related operations in
src/crab/components - Proper bounds checking is implemented using these constants (e.g., in
src/assertions.cppandsrc/asm_unmarshal.cpp) - No naming conflicts found - these are the only stack-related macros besides an unrelated Linux spec prototype
- The constants are used in a type-safe manner with proper overflow checks
The changes appear to provide a robust and well-integrated stack frame implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any .c files that include this header
echo "Checking .c files that include ebpf_base.h:"
rg -l "#include.*ebpf_base.h" --type c
# Check for any potential macro naming conflicts
echo -e "\nChecking for potential macro naming conflicts:"
rg "#define (MAX_CALL_STACK_FRAMES|EBPF_SUBPROGRAM_STACK_SIZE|EBPF_TOTAL_STACK_SIZE)"
Length of output: 536
Script:
#!/bin/bash
# Search for usage of these macros in source files
echo "Checking usage of the macros:"
rg -A 2 "(MAX_CALL_STACK_FRAMES|EBPF_SUBPROGRAM_STACK_SIZE|EBPF_TOTAL_STACK_SIZE)" --type cpp --type c --type h
# Check if there are any other stack-related macros that might interact
echo -e "\nChecking other stack-related macros:"
rg "#define.*STACK" --type cpp --type c --type h
# Look for the referenced PR #721 to understand the context
echo -e "\nLooking for PR #721 references:"
rg "721" .github/
Length of output: 8483
|
Maybe we should add a trivial compilation step for the header files that should be compliant with C? |
ebpf_base.h says at the top:
However, PR #721 added some "constexpr int" lines at the end which
of course don't work with C files. This PR corrects that by using
#defineinstead.Summary by CodeRabbit