Remove thread_local_program_info; make Program self-contained#1091
Merged
Remove thread_local_program_info; make Program self-contained#1091
Conversation
Replace thread_local_program_info reads in platform helper functions with explicit const EbpfProgramType& parameters on is_helper_usable, get_helper_prototype, make_call, and parse_instruction. This removes the implicit dependency on ambient thread-local state from the platform layer and instruction parsing. Change Program::from_sequence to take ProgramInfo& (non-const) so callback targets are computed directly on the caller's ProgramInfo rather than through the thread-local copy. Move the map equivalence cache from ProgramInfo into a local variable in parse_maps_section_linux, passed explicitly through create_map_linux and create_map_crab. This fixes a pre-existing test-ordering bug where stale cache entries from a previous test caused FD number mismatches. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
Add const ProgramInfo& parameter to ebpf_get_map_descriptor_fn and make it return const EbpfMapDescriptor&. Update find_map_descriptor, get_map_descriptor_linux, and the test platform implementation. Change EbpfDomain map query methods (get_map_type, get_map_key_size, get_map_value_size, get_map_inner_map_fd, get_map_max_entries) to take const AnalysisContext& instead of const ebpf_platform_t&, since they now need both platform and program_info for map descriptor lookup. This eliminates thread_local_program_info from elf_loader.cpp entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
Program now owns its ProgramInfo, populated by from_sequence. The analyze(prog) shims construct AnalysisContext from prog.info() and thread_local_options, eliminating all reads of thread_local_program_info. Add explicit AnalysisContext parameter to AnalysisResult methods (is_valid_after, check_observation_at_label, compute_failure_slices) and const Program& to print_error for line-info lookup. Remove thread_local_program_info variable, its extern declaration, the LazyAllocator include, thread_local_analysis_context(), and the dead 3-arg ebpf_domain_check overload. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThread-local program state is eliminated by storing Changes
Possibly related issues
Possibly related PRs
Suggested labels
🚥 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🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db3c7a095f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/printing.cpp (1)
106-127: 🧹 Nitpick | 🔵 TrivialAvoid copying the entire line-info map per printer.
LineInfoPrinterstoresline_info_mapby value, so everyDetailedPrinterandprint_errorconstruction copies all BTF line info. Store a const reference;progoutlives these short-lived printers.Proposed change
struct LineInfoPrinter { std::ostream& os; - const std::map<size_t, btf_line_info_t> line_info_map; + const std::map<size_t, btf_line_info_t>& line_info_map; bool print_line_info_enabled{}; std::string previous_source_line;Also applies to: 253-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/printing.cpp` around lines 106 - 127, LineInfoPrinter currently stores line_info_map by value causing expensive copies; change its member to const std::map<size_t, btf_line_info_t>& line_info_map and update its constructor to accept that const reference, then update all call sites (e.g., DetailedPrinter constructor and any print_error constructions) to pass prog.info().line_info by reference instead of letting it be copied; keep other members (print_line_info_enabled, previous_source_line) unchanged so behavior is identical while avoiding the map copy.src/ir/parse.cpp (1)
156-170: 🧹 Nitpick | 🔵 TrivialPlatform hardcoding in
parse_instructionlimits flexibility but does not impact current usage.
parse_instructionat line 170 hardcodesg_ebpf_platform_linuxwhen callingmake_call, despite acceptingprogram_typeas a parameter. This API inconsistency is documented in test code (ebpf_yaml.cpp:31–34), which explicitly works around it. The only real caller is test code that passes an emptyprogram_typeanyway, and the call at line 306 is inside the unusedparse_programfunction (marked[[maybe_unused]]). If future code paths require platform flexibility during parsing, threadebpf_platform_tthrough the signature; until then, this is design debt rather than a blocking issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ir/parse.cpp` around lines 156 - 170, The code in parse_instruction hardcodes g_ebpf_platform_linux when calling make_call; update the call in the parse_instruction function to use the platform information carried by the EbpfProgramType parameter (program_type) instead of g_ebpf_platform_linux so make_call(func, /*platform from program_type*/, program_type) is used; locate the make_call invocation in parse_instruction and thread the program_type's platform field (or otherwise expose an ebpf platform from program_type) into that argument to remove the hardcoded g_ebpf_platform_linux.src/main.cpp (1)
240-256:⚠️ Potential issue | 🟠 MajorSynchronize verifier options before analysis.
compute_failure_slicesreceivesebpf_verifier_options, but theAnalysisResultis produced earlier byanalyze(prog). Keep the analysis and slicing contexts on the same options; otherwise--failure-slicecan request deps after analysis already ran without collecting them.Proposed fix
- auto result = analyze(prog); + thread_local_options = ebpf_verifier_options; + auto result = analyze(prog); @@ - const AnalysisContext context{prog.info(), ebpf_verifier_options, *prog.info().platform}; + const AnalysisContext context{prog.info(), thread_local_options, *prog.info().platform};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 240 - 256, The analysis result is produced by analyze(prog) before failure-slice uses ebpf_verifier_options, causing options drift; ensure the verifier options are synchronized by running analysis with the same options used for slicing: change the analyze call to accept/use ebpf_verifier_options (e.g., analyze(prog, ebpf_verifier_options) or update prog/info to reflect ebpf_verifier_options before calling analyze), so that the AnalysisResult, compute_failure_slices, and AnalysisContext all operate with the same ebpf_verifier_options values.src/ir/cfg_builder.cpp (1)
647-721:⚠️ Potential issue | 🟠 MajorAccept
ProgramInfoby value instead of lvalue reference to make ownership transfer explicit.The function signature accepts
ProgramInfo&but moves from it (line 720). While no current call sites reuse the moved-from object, the API allows undefined behavior if a caller passes a reusable lvalue and accesses it afterward. Taking the parameter by value makes ownership semantics clear without requiring callers to writestd::move.Proposed change
-Program Program::from_sequence(const InstructionSeq& inst_seq, ProgramInfo& info, +Program Program::from_sequence(const InstructionSeq& inst_seq, ProgramInfo info, const ebpf_verifier_options_t& options) {Update both the declaration in
src/ir/program.hpp:58and the definition insrc/ir/cfg_builder.cpp:647.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ir/cfg_builder.cpp` around lines 647 - 721, Change Program::from_sequence to take ProgramInfo by value (ProgramInfo info) instead of ProgramInfo& so ownership is explicit; update the declaration in program.hpp and the definition in cfg_builder.cpp accordingly, then keep the existing move into builder.prog.m_info (std::move(info)) as the final transfer. Ensure all call sites that currently pass an lvalue continue to compile (no extra std::move required) and adjust any forwarding or validation that relied on a reference (e.g., options.validate() and validate_instruction_feature_support(inst_seq, info, ...) now receive the by-value info).src/result.cpp (1)
619-643:⚠️ Potential issue | 🟠 MajorAdd a guard to bind the slice context to the Program being analyzed.
The function accepts separate
progandcontextparameters. Whileprogsupplies the assertions and CFG,ebpf_domain_check(..., context)usescontext.program_info. If a caller passes a context built from a different program's ProgramInfo, the failure-slice seeding replays checks against the wrong semantic environment. Add an assertion at the function entry to enforce thatcontext.program_infomatchesprog.info():Suggested fix
std::vector<FailureSlice> AnalysisResult::compute_failure_slices(const Program& prog, const SliceParams params, const AnalysisContext& context) const { + assert(&context.program_info == &prog.info()); const auto max_slices = params.max_slices;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/result.cpp` around lines 619 - 643, Add a guard in AnalysisResult::compute_failure_slices to ensure the provided AnalysisContext is bound to the same Program as the prog parameter: at the start of compute_failure_slices check that context.program_info equals prog.info() (or equivalent identity/comparison used by ProgramInfo) and assert or throw if they differ; this prevents ebpf_domain_check and the assertion replay from using a mismatched ProgramInfo when seeding failure slices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/crab/ebpf_domain.cpp`:
- Around line 266-341: These helpers (get_map_type, get_map_inner_map_fd,
get_map_key_size, get_map_value_size, get_map_max_entries) currently call
context.platform.get_map_descriptor (which may throw if an fd is missing) while
iterating an abstract fd range; change them to detect missing descriptors and
return conservative fallbacks instead of allowing exceptions: for
get_map_type/get_map_inner_map_fd return std::nullopt when any fd in the range
has no descriptor, and for
get_map_key_size/get_map_value_size/get_map_max_entries return Interval::top()
when any descriptor is absent; implement this by replacing the direct call with
a safe lookup/check (or try/catch around
get_map_descriptor/get_map_descriptor_linux) and on failure immediately return
the conservative value.
In `@src/fwd_analyzer.cpp`:
- Around line 164-171: The two analyze(...) overloads currently build
AnalysisContext from thread_local_options, which makes a prepared Program
non-self-contained; change them to use the options stored on the Program itself
(e.g. use a getter like prog.prepared_options() or prog.preparation_options())
when constructing AnalysisContext (i.e., replace thread_local_options with the
Program's stored preparation options), or if no such field exists remove these
convenience overloads and require callers to call analyze(prog, entry_invariant,
AnalysisContext{...}) with an explicit AnalysisContext; update
constructors/usages around AnalysisContext and the Program API accordingly and
adjust callers/tests to pass the correct context if you remove the overloads.
In `@src/io/elf_loader.hpp`:
- Around line 25-26: The header declares create_map_crab(...) using
std::map<EquivalenceKey,int>& but does not include <map>, creating a
transitive-include dependency; open src/io/elf_loader.hpp and add `#include` <map>
to the top-of-file includes so std::map is defined for the create_map_crab
declaration (and any other uses of std::map in this header), keeping
EquivalenceKey and other includes unchanged.
In `@src/ir/program.hpp`:
- Around line 58-59: The declaration of Program::from_sequence currently takes
ProgramInfo& but the implementation in cfg_builder.cpp moves from the info
parameter (consumes it); change the API to take ProgramInfo by value or as
ProgramInfo&& to reflect consumption semantics, update the declaration of static
Program from_sequence(const InstructionSeq& inst_seq, ProgramInfo info, const
ebpf_verifier_options_t& options) (or use ProgramInfo&&) to match the
implementation, and update all call sites to pass std::move(raw_prog.info) (or
forward rvalues) so that callers explicitly transfer ownership; ensure the
implementation signature and all declarations match the chosen by-value or
rvalue-reference form.
In `@src/result.hpp`:
- Around line 156-162: The public overload of compute_failure_slices can receive
an AnalysisContext from a different Program which lets
context.options.total_stack_size() and other platform facts diverge from
prog.info(); fix by validating or deriving the program info: at the start of
compute_failure_slices(const Program& prog, const AnalysisContext& context) add
a runtime assertion that the context was built from the same program (e.g.,
assert(context.program_info == prog.info()) or equivalent accessor), and update
the header comment to state the precondition; alternatively, replace usage of
context.program_info inside the function by deriving ProgramInfo from
prog.info() and constructing/using a local AnalysisContext tied to prog to
eliminate the mismatch risk.
In `@src/test/ebpf_yaml.cpp`:
- Around line 420-421: The test constructs an explicit AnalysisContext named
context but then calls analyze(prog, test_case.assumed_pre_invariant), which
uses the thread-local options; change the call to use the overload that accepts
the context so analysis runs with that context (i.e., call analyze(prog,
context, test_case.assumed_pre_invariant) or the appropriate analyze(prog,
context, ...) overload) ensuring AnalysisContext context is passed into the
analyze invocation so result and subsequent observation checks use the same
environment.
- Line 297: The test is calling parse_instruction with an empty program type
(passing `{}`), which yields a null context_descriptor and causes
program-type-required helpers to be rejected; update raw_cfg_to_instruction_seq
to accept and thread an EbpfProgramType parameter and then pass that
EbpfProgramType through to parse_instruction so parse_instruction receives the
correct program type (not `{}`) when constructing Instruction instances; adjust
raw_cfg_to_instruction_seq signature/usages and the call site that currently
does const Instruction& ins = parse_instruction(line, label_name_to_label, {});
to supply the threaded EbpfProgramType so helpers resolve correctly.
In `@src/test/test_failure_slice.cpp`:
- Around line 256-260: The test uses elf.get_programs("xdp").back() without
asserting that the returned vector is non-empty, which can cause a crash outside
Catch2; update the test to first capture the vector (e.g., auto progs =
elf.get_programs("xdp")), assert its non-emptiness with REQUIRE(progs.size() >
0) (or REQUIRE_FALSE(progs.empty())), then use progs.back() to obtain raw_prog
before calling unmarshal/Program::from_sequence; ensure the check happens before
accessing raw_prog and before any use of InstructionSeq.
- Around line 30-33: Set the intended thread-local analysis options before
running analysis: assign or install the local options (the one with
collect_instruction_deps enabled) into thread_local_options prior to calling
analyze(prog) so analyze builds results with instruction dependencies; then
construct the AnalysisContext from options and call
result.compute_failure_slices(prog, context) as before (affects the block around
analyze(prog), the const AnalysisContext{...} construction, and
compute_failure_slices calls).
---
Outside diff comments:
In `@src/ir/cfg_builder.cpp`:
- Around line 647-721: Change Program::from_sequence to take ProgramInfo by
value (ProgramInfo info) instead of ProgramInfo& so ownership is explicit;
update the declaration in program.hpp and the definition in cfg_builder.cpp
accordingly, then keep the existing move into builder.prog.m_info
(std::move(info)) as the final transfer. Ensure all call sites that currently
pass an lvalue continue to compile (no extra std::move required) and adjust any
forwarding or validation that relied on a reference (e.g., options.validate()
and validate_instruction_feature_support(inst_seq, info, ...) now receive the
by-value info).
In `@src/ir/parse.cpp`:
- Around line 156-170: The code in parse_instruction hardcodes
g_ebpf_platform_linux when calling make_call; update the call in the
parse_instruction function to use the platform information carried by the
EbpfProgramType parameter (program_type) instead of g_ebpf_platform_linux so
make_call(func, /*platform from program_type*/, program_type) is used; locate
the make_call invocation in parse_instruction and thread the program_type's
platform field (or otherwise expose an ebpf platform from program_type) into
that argument to remove the hardcoded g_ebpf_platform_linux.
In `@src/main.cpp`:
- Around line 240-256: The analysis result is produced by analyze(prog) before
failure-slice uses ebpf_verifier_options, causing options drift; ensure the
verifier options are synchronized by running analysis with the same options used
for slicing: change the analyze call to accept/use ebpf_verifier_options (e.g.,
analyze(prog, ebpf_verifier_options) or update prog/info to reflect
ebpf_verifier_options before calling analyze), so that the AnalysisResult,
compute_failure_slices, and AnalysisContext all operate with the same
ebpf_verifier_options values.
In `@src/printing.cpp`:
- Around line 106-127: LineInfoPrinter currently stores line_info_map by value
causing expensive copies; change its member to const std::map<size_t,
btf_line_info_t>& line_info_map and update its constructor to accept that const
reference, then update all call sites (e.g., DetailedPrinter constructor and any
print_error constructions) to pass prog.info().line_info by reference instead of
letting it be copied; keep other members (print_line_info_enabled,
previous_source_line) unchanged so behavior is identical while avoiding the map
copy.
In `@src/result.cpp`:
- Around line 619-643: Add a guard in AnalysisResult::compute_failure_slices to
ensure the provided AnalysisContext is bound to the same Program as the prog
parameter: at the start of compute_failure_slices check that
context.program_info equals prog.info() (or equivalent identity/comparison used
by ProgramInfo) and assert or throw if they differ; this prevents
ebpf_domain_check and the assertion replay from using a mismatched ProgramInfo
when seeding failure slices.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a9807b4-bd5c-415d-a026-e115bce15491
📒 Files selected for processing (28)
src/analysis_context.hppsrc/crab/ebpf_checker.cppsrc/crab/ebpf_domain.cppsrc/crab/ebpf_domain.hppsrc/crab/ebpf_transformer.cppsrc/fwd_analyzer.cppsrc/io/elf_loader.cppsrc/io/elf_loader.hppsrc/ir/cfg_builder.cppsrc/ir/parse.cppsrc/ir/parse.hppsrc/ir/program.hppsrc/ir/unmarshal.cppsrc/ir/unmarshal.hppsrc/linux/gpl/spec_prototypes.cppsrc/linux/linux_platform.cppsrc/linux/linux_platform.hppsrc/main.cppsrc/platform.hppsrc/printing.cppsrc/result.cppsrc/result.hppsrc/spec/type_descriptors.hppsrc/test/ebpf_yaml.cppsrc/test/test_failure_slice.cppsrc/test/test_platform_tables.cppsrc/test/test_verify.hppsrc/test/test_verify_multithreading.cpp
💤 Files with no reviewable changes (2)
- src/analysis_context.hpp
- src/spec/type_descriptors.hpp
Replace per-section mock map cache with allocate_mock_map_fd, a pure function that deduplicates against the accumulated map_descriptors vector. This fixes FD collisions when an ELF has multiple legacy map sections, since each parse_maps_section call now sees all previously assigned FDs. Remove create_map_crab (now unused). Restore const ProgramInfo& on Program::from_sequence to avoid silently consuming the caller's ProgramInfo via move. The function copies into a local, mutates callback targets there, and moves into Program. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
Add analyze(prog, options) and verify(prog, options) so callers can pass options explicitly without constructing AnalysisContext. The thread_local_options shims delegate to these. Fix test_failure_slice and ebpf_yaml to pass options explicitly to analyze instead of relying on thread_local_options, ensuring collect_instruction_deps and other test-specific options take effect. Add missing REQUIRE size check in test_failure_slice before .back(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EbpfProgramTypeexplicitly through platform helper functions (is_helper_usable,get_helper_prototype), removing implicitthread_local_program_inforeads from the platform layerProgramInfothroughget_map_descriptorplatform function, eliminatingthread_local_program_infofromelf_loader.cppProgramInfointo a local variable inparse_maps_section_linux, fixing a pre-existing test-ordering bug where stale cache entries caused FD mismatchesProgramInfoinProgram(resolves Make prepared programs self-contained #1082), making prepared programs self-containedthread_local_program_infoentirely — variable, extern declaration,LazyAllocatorinclude,thread_local_analysis_context()bridge, and deadebpf_domain_checkoverloadContinues #1088.
thread_local_optionsand the domain-internal thread-locals (array_map,SplitDBM::scratch_) remain for a future PR.Test plan
🤖 Generated with Claude Code