-
Notifications
You must be signed in to change notification settings - Fork 50
Lazy init constant_limits with LazyAllocator #970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduce thread-local lazy initialization for EbpfDomain constant limits via a thread-local LazyAllocator and accessor; add Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (4)src/**/*.{cpp,hpp,h}📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/{asm_*,cfg,arith,crab,linux}/**/*.{cpp,hpp,h}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{hpp,h}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{cpp,c,hpp,h}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-12-09T20:31:07.030ZApplied to files:
📚 Learning: 2025-12-09T20:31:07.030ZApplied to files:
📚 Learning: 2025-12-09T20:31:07.030ZApplied to files:
🧬 Code graph analysis (1)src/fwd_analyzer.cpp (1)
🪛 Clang (14.0.6)src/crab/ebpf_domain.cpp[warning] 163-163: variable 'constant_limits' is non-const and globally accessible, consider making it const (cppcoreguidelines-avoid-non-const-global-variables) [warning] 165-165: use a trailing return type for this function (modernize-use-trailing-return-type) [warning] 169-169: use a trailing return type for this function (modernize-use-trailing-return-type) [warning] 169-169: method 'widen' can be made static (readability-convert-member-functions-to-static) src/fwd_analyzer.cpp[error] 3-3: 'ranges' file not found (clang-diagnostic-error) 🪛 Cppcheck (2.19.0)src/crab/ebpf_domain.cpp[style] 164-164: The function 'urem' is never used. (unusedFunction) [style] 173-173: The function 'copy' is never used. (unusedFunction) src/fwd_analyzer.cpp[information] 3-3: Include file (missingIncludeSystem) [information] 30-30: Include file (missingIncludeSystem) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/crab/ebpf_domain.cpp:
- Around line 157-160: The LazyAllocator instance constant_limits caches results
from EbpfDomain::calculate_constant_limits which capture thread_local_options
and variable_registry; add a call to constant_limits.clear() inside
ebpf_verifier_clear_thread_local_state() (in the cleanup routine in
src/fwd_analyzer.cpp) so the LazyAllocator is invalidated when thread-local
state is reset, preventing stale cached limits across reused threads.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/crab/ebpf_domain.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{cpp,hpp,h}: Prove invariants when possible — encode the assumptions an analysis relies on (preconditions, lattice properties, monotonicity) directly in code comments, assertions, or type-level checks before trusting experiments.
Optimise for auditability — choose designs that are easy to step through and review by hand (even if marginally slower or more verbose) so future engineers can re-establish the soundness argument quickly.
Favour explicit error handling and early exits to surface problems instead of deferring to implicit behaviour.
Files:
src/crab/ebpf_domain.cpp
src/{asm_*,cfg,arith,crab,linux}/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Narrate the reasoning — any change that affects analysis results should spell out the argument for soundness: what inputs are assumed, what invariants are maintained, and how the change preserves them.
Files:
src/crab/ebpf_domain.cpp
**/*.{cpp,c,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,c,hpp,h}: Ensure new C/C++ sources include the standard SPDX license header; validate with ./scripts/check-license.sh.
Run ./scripts/format-code --staged before committing to enforce clang-format standards; this mirrors the installed Git hook.
Files:
src/crab/ebpf_domain.cpp
🧬 Code graph analysis (1)
src/crab/ebpf_domain.cpp (1)
src/crab/ebpf_domain.hpp (10)
other(59-59)other(60-60)other(61-61)other(62-62)other(63-63)other(64-64)other(65-65)other(66-66)other(67-67)other(68-68)
🪛 Clang (14.0.6)
src/crab/ebpf_domain.cpp
[warning] 158-158: variable 'constant_limits' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 160-160: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 162-162: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 162-162: method 'widen' can be made static
(readability-convert-member-functions-to-static)
🪛 Cppcheck (2.19.0)
src/crab/ebpf_domain.cpp
[style] 164-164: The function 'urem' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_ubuntu (Release, library)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: build_ubuntu (Debug)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (2)
src/crab/ebpf_domain.cpp (2)
17-17: LGTM!Required include for the
LazyAllocatortemplate.
162-169: LGTM!Usage of
get_constant_limits()correctly replaces the former static reference. The widening logic remains intact.(The static-analysis hint that
widencan be made static is a false positive—it accessesthis->rcpandstack.)
07e1a06 to
9f3f25f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/crab/ebpf_domain.cpp:
- Around line 157-160: Update the comment above the LazyAllocator instantiation
to explicitly document why clamping to constant limits via intersection is sound
and safe: state that LazyAllocator<EbpfDomain,
EbpfDomain::calculate_constant_limits> is used so thread_local_options is
populated first, note that LazyAllocator::get() has no reentrancy guard but
calculate_constant_limits does not call get_constant_limits() or
widen(to_constants=true) so recursion cannot occur, and explain that the
intersection operation only shrinks the domain (element-wise tightening of
constraints) and therefore preserves the over-approximation required for
soundness; reference LazyAllocator, EbpfDomain::calculate_constant_limits,
get_constant_limits, and widen(to_constants=true) in the comment.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/crab/ebpf_domain.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{cpp,hpp,h}: Prove invariants when possible — encode the assumptions an analysis relies on (preconditions, lattice properties, monotonicity) directly in code comments, assertions, or type-level checks before trusting experiments.
Optimise for auditability — choose designs that are easy to step through and review by hand (even if marginally slower or more verbose) so future engineers can re-establish the soundness argument quickly.
Favour explicit error handling and early exits to surface problems instead of deferring to implicit behaviour.
Files:
src/crab/ebpf_domain.cpp
src/{asm_*,cfg,arith,crab,linux}/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Narrate the reasoning — any change that affects analysis results should spell out the argument for soundness: what inputs are assumed, what invariants are maintained, and how the change preserves them.
Files:
src/crab/ebpf_domain.cpp
**/*.{cpp,c,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,c,hpp,h}: Ensure new C/C++ sources include the standard SPDX license header; validate with ./scripts/check-license.sh.
Run ./scripts/format-code --staged before committing to enforce clang-format standards; this mirrors the installed Git hook.
Files:
src/crab/ebpf_domain.cpp
🧬 Code graph analysis (1)
src/crab/ebpf_domain.cpp (1)
src/crab/ebpf_domain.hpp (10)
other(59-59)other(60-60)other(61-61)other(62-62)other(63-63)other(64-64)other(65-65)other(66-66)other(67-67)other(68-68)
🪛 Clang (14.0.6)
src/crab/ebpf_domain.cpp
[warning] 158-158: variable 'constant_limits' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 160-160: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 162-162: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 162-162: method 'widen' can be made static
(readability-convert-member-functions-to-static)
🪛 Cppcheck (2.19.0)
src/crab/ebpf_domain.cpp
[style] 164-164: The function 'urem' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Release, library)
- GitHub Check: build_ubuntu (Debug, library)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (2)
src/crab/ebpf_domain.cpp (2)
17-17: LGTM!Include is correctly placed and necessary for the
LazyAllocatortemplate.
162-169: LGTM!The widening implementation correctly uses
get_constant_limits()to clamp the result whento_constantsis true. The intersection operation preserves soundness by narrowing to the domain of representable constants.
|
✅ Review comment addressed. I've added:
This ensures the cached constant limits are invalidated when thread-local state is reset, preventing stale cached values when threads are reused with different configurations. |
Pull Request Test Coverage Report for Build 20967734412Details
💛 - Coveralls |
173f260 to
bdc575b
Compare
|
@Alan-Jowett Could you just update the sign-off to use your signature instead of Copilot's? |
|
Whoops. Will do. |
bdc575b to
3ef0772
Compare
Signed-off-by: Alan Jowett <alanjo@microsoft.com>
3ef0772 to
c73b885
Compare
Resolves: #972
Problem
The
constant_limitsstatic variable inebpf_domain.cppwas initialized at static initialization time, beforethread_local_optionsandvariable_registrywere properly set up. In release builds, this caused static initialization order issues whereEbpfDomain::calculate_constant_limits()would capture uninitialized or default state from these thread-local variables, leading to incorrect constraint limits being cached and reused throughout the program's lifetime.Additionally, even with lazy initialization, the cached limits could become stale when threads are reused with different configurations (e.g., in test suites where
thread_local_optionsis reassigned between test cases), as the cache was never invalidated during thread-local state cleanup.Solution
Lazy initialization: Replace static initialization with
LazyAllocator<EbpfDomain, EbpfDomain::calculate_constant_limits>to defer computation until first use, ensuringthread_local_optionsandvariable_registryare populated beforecalculate_constant_limits()captures their state.Cache invalidation: Add
EbpfDomain::clear_thread_local_state()method that clears theconstant_limitscache, and call it fromebpf_verifier_clear_thread_local_state()to prevent stale cached limits when threads are reused with different configurations.Soundness documentation: Expand comment to explicitly document reentrancy safety (no recursion possible since
calculate_constant_limitsdoesn't callget_constant_limits()orwiden(to_constants=true)) and soundness of intersection-based clamping (only shrinks domain, preserving over-approximation).Changes
constant_limitsfrom static initialization tothread_local LazyAllocatorget_constant_limits()accessor functionEbpfDomain::clear_thread_local_state()to invalidate the cachewiden()to useget_constant_limits()accessorWidening clamping behavior remains identical; this only fixes the initialization timing and cache lifetime issues.
Testing
Existing tests pass. The fix ensures correct behavior in release builds and when threads are reused across different verification contexts.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.