Add support for resolving helper functions by name instead of by id

[Alan-Jowett]

Resolves: #938

This pull request introduces support for external helper function calls in eBPF programs on Linux. The main changes involve adding logic to resolve external helper functions by name, updating platform interfaces to expose helper function lookup, and extending tests to cover this functionality.

Support for external helper function calls

• Added logic in ProgramReader::try_reloc (src/elf_loader.cpp) to detect calls to external helper functions (flagged as local calls with section index 0), resolve their index using the platform, and update the instruction accordingly.
• Implemented get_helper_index_linux in src/linux/gpl/spec_prototypes.cpp to look up helper function indices by name, throwing an error if the helper is not found.

Platform interface updates

• Added the get_helper_index function pointer to the ebpf_platform_t struct in src/platform.hpp, and updated the Linux platform implementation to provide this function. [1] [2] [3] [4]

Test coverage

• Added a new test section for an external function program in src/test/test_verify.cpp to verify the new functionality.

Submodule update

• Updated the ebpf-samples submodule to a newer commit, likely to pick up changes needed for external helper function testing.

Summary by CodeRabbit

• New Features

  • Relocation now resolves calls to external helpers into static helper calls when applicable.
  • Platform API exposes a helper-name → index lookup.

• Tests

  • Verification coverage extended to include external function scenarios and the test platform now exercises the new helper-index lookup.

[coderabbitai]

Walkthrough

Adds platform support for resolving helper functions by name, implements Linux name→index lookup, updates ELF loader to rewrite external-call relocations to static helper calls when resolvable, and extends tests to exercise the new path.

Changes

Cohort / File(s)	Summary
Platform interface src/platform.hpp	Add ebpf_get_helper_index_fn typedef and get_helper_index member to ebpf_platform_t.
Linux platform API & wiring src/linux/linux_platform.hpp, src/linux/linux_platform.cpp	Declare int32_t get_helper_index_linux(const std::string& name) and wire it into g_ebpf_platform_linux initializer.
Linux helper lookup implementation src/linux/gpl/spec_prototypes.cpp	Implement get_helper_index_linux() that matches helper names against prototypes (prefixing with "bpf_") and returns index or -1.
ELF loader relocation handling src/elf_loader.cpp	In ProgramReader::try_reloc, when an undefined external symbol (section index == 0) is the target of a local call, query platform get_helper_index and, if found, convert the instruction to INST_CALL_STATIC_HELPER with imm set to the helper index.
Tests / test harness src/test/test_verify.cpp, src/test/ebpf_yaml.cpp	Add TEST_SECTION("build", "externalfunction.o", ".text") and wire ebpf_get_helper_index_test into the test platform (g_platform_test) to exercise name-based helper resolution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Inspect ELF loader edit in ProgramReader::try_reloc for correct instruction encoding, branch/return behavior, and no regressions for non-helper relocations.
• Verify get_helper_index_linux string matching (prefixing with "bpf_"), return -1 semantics, and that it’s NULL-safe when platform callback absent.
• Confirm ebpf_platform_t ABI change is applied consistently where g_ebpf_platform_linux and test platform instances are built.
• Review added tests to ensure they actually exercise the new resolution and handle unresolved names.

Possibly related PRs

• Improve Linux helper support #926 — Changes to Linux helper prototypes and helper-lookup logic that overlap with name-to-index resolution and prototype handling.

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'Add support for resolving helper functions by name instead of by id' directly and clearly describes the primary change in the changeset. It is concise, specific, and accurately reflects the main objective of extending the platform interface to resolve helpers by name rather than numeric ID.
Linked Issues check	✅ Passed	The PR successfully implements all coding requirements from issue #938: it adds a get_helper_index platform interface function [src/platform.hpp, src/linux/linux_platform.hpp, src/linux/gpl/spec_prototypes.cpp], implements helper lookup by name [src/linux/gpl/spec_prototypes.cpp], patches instructions during ELF loading for external helper calls [src/elf_loader.cpp], and maintains backward compatibility by leaving non-matching cases unchanged. Test coverage has been added to verify the new functionality.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with the stated objectives. The modifications to src/platform.hpp, src/linux/linux_platform.hpp, src/linux/gpl/spec_prototypes.cpp, src/elf_loader.cpp, and src/test files are necessary to implement name-based helper resolution. The test addition in src/test/test_verify.cpp and src/test/ebpf_yaml.cpp are within scope for verifying the new feature.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fdb0343 and da04fe2.

📒 Files selected for processing (2)

• src/elf_loader.cpp (1 hunks)
• src/test/ebpf_yaml.cpp (2 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{c,cc,cpp,cxx,h,hpp,hxx}

📄 CodeRabbit inference engine (AGENTS.md)

Run ./scripts/format-code --staged (clang-format) before committing; ensure formatting matches project baseline

Files:

• src/elf_loader.cpp
• src/test/ebpf_yaml.cpp

**/*.{c,cc,cpp,cxx}

📄 CodeRabbit inference engine (AGENTS.md)

Ensure new C/C++ source files include the standard SPDX license header; validate with ./scripts/check-license.sh

Files:

• src/elf_loader.cpp
• src/test/ebpf_yaml.cpp

src/test/**/*.{cc,cpp,cxx}

📄 CodeRabbit inference engine (AGENTS.md)

src/test/**/*.{cc,cpp,cxx}: Place Catch2 unit/integration tests under src/test and add focused tests when modifying verifier behaviour
When introducing or relying on a new invariant, add a targeted regression test covering the invariant

Files:

• src/test/ebpf_yaml.cpp

🧬 Code graph analysis (1)

src/test/ebpf_yaml.cpp (1)

src/platform.hpp (1)

• name (24-24)

🪛 Clang (14.0.6)

src/elf_loader.cpp

[warning] 957-957: variable 'helper_index' is not initialized

(cppcoreguidelines-init-variables)

src/test/ebpf_yaml.cpp

[warning] 43-43: use a trailing return type for this function

(modernize-use-trailing-return-type)

⏰ 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). (11)

• GitHub Check: Analyze (cpp)
• GitHub Check: build_windows (Debug, tests)
• GitHub Check: build_windows (Release, tests)
• GitHub Check: build_windows (Debug, library)
• GitHub Check: build_windows (Release, library)
• GitHub Check: build_ubuntu (Debug, tests)
• GitHub Check: build_ubuntu (Debug, library)
• GitHub Check: build_ubuntu (Release, tests)
• GitHub Check: build_ubuntu (Release, library)
• GitHub Check: build_ubuntu (Debug)
• GitHub Check: build_ubuntu (Release)

🔇 Additional comments (2)

src/test/ebpf_yaml.cpp (2)

63-63: LGTM!

The field initialization correctly wires the new helper function to the test platform structure, maintaining consistency with the existing initialization pattern.

---

43-46: Implementation is correct; no changes needed.

The Linux implementation of get_helper_index returns -1 for unknown helpers (line 2767 in src/linux/gpl/spec_prototypes.cpp), aligning with the PR objectives. The test function correctly delegates to this implementation and will handle unknown helper names appropriately.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/linux/linux_platform.hpp (1)

5-12: Avoid transitive includes for std::string

Include in this header to make it self-contained.

 #pragma once

+#include <string>
 #include "spec/function_prototypes.hpp"

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef861aa and 4227187.

📒 Files selected for processing (10)

• ebpf-samples (1 hunks)
• src/elf_loader.cpp (3 hunks)
• src/ir/unmarshal.cpp (4 hunks)
• src/linux/gpl/spec_prototypes.cpp (1 hunks)
• src/linux/linux_platform.cpp (1 hunks)
• src/linux/linux_platform.hpp (1 hunks)
• src/platform.hpp (2 hunks)
• src/spec/type_descriptors.hpp (1 hunks)
• src/test/test_marshal.cpp (1 hunks)
• src/test/test_verify.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (5)

**/*.{c,cc,cpp,cxx,h,hpp,hxx}

📄 CodeRabbit inference engine (AGENTS.md)

Run ./scripts/format-code --staged (clang-format) before committing; ensure formatting matches project baseline

Files:

• src/spec/type_descriptors.hpp
• src/linux/linux_platform.hpp
• src/linux/linux_platform.cpp
• src/platform.hpp
• src/test/test_verify.cpp
• src/test/test_marshal.cpp
• src/ir/unmarshal.cpp
• src/linux/gpl/spec_prototypes.cpp
• src/elf_loader.cpp

**/*.{h,hpp,hxx}

📄 CodeRabbit inference engine (AGENTS.md)

Prefer #pragma once in header files; follow existing patterns within each subdirectory

Files:

• src/spec/type_descriptors.hpp
• src/linux/linux_platform.hpp
• src/platform.hpp

src/{cfg,crab,linux,arith,asm_*}/**/*

📄 CodeRabbit inference engine (AGENTS.md)

Add new verifier logic under the matching subsystem directory (cfg/, crab/, linux/, arith/, asm_*) to keep separation of concerns

Files:

• src/linux/linux_platform.hpp
• src/linux/linux_platform.cpp
• src/linux/gpl/spec_prototypes.cpp

**/*.{c,cc,cpp,cxx}

📄 CodeRabbit inference engine (AGENTS.md)

Ensure new C/C++ source files include the standard SPDX license header; validate with ./scripts/check-license.sh

Files:

• src/linux/linux_platform.cpp
• src/test/test_verify.cpp
• src/test/test_marshal.cpp
• src/ir/unmarshal.cpp
• src/linux/gpl/spec_prototypes.cpp
• src/elf_loader.cpp

src/test/**/*.{cc,cpp,cxx}

📄 CodeRabbit inference engine (AGENTS.md)

src/test/**/*.{cc,cpp,cxx}: Place Catch2 unit/integration tests under src/test and add focused tests when modifying verifier behaviour
When introducing or relying on a new invariant, add a targeted regression test covering the invariant

Files:

• src/test/test_verify.cpp
• src/test/test_marshal.cpp

🧠 Learnings (2)

📚 Learning: 2025-10-15T13:34:52.142Z

Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/test/**/*.{cc,cpp,cxx} : Place Catch2 unit/integration tests under src/test and add focused tests when modifying verifier behaviour

Applied to files:

• src/test/test_verify.cpp

📚 Learning: 2025-10-15T13:34:52.142Z

Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/test/**/*.{cc,cpp,cxx} : When introducing or relying on a new invariant, add a targeted regression test covering the invariant

Applied to files:

• src/test/test_verify.cpp

🧬 Code graph analysis (6)

src/linux/linux_platform.hpp (1)

src/platform.hpp (4)

• name (21-21)
• name (24-24)
• n (20-20)
• n (23-23)

src/linux/linux_platform.cpp (2)

src/linux/gpl/spec_prototypes.cpp (6)

• get_helper_prototype_by_name_linux (2779-2794)
• get_helper_prototype_by_name_linux (2779-2779)
• is_helper_usable_linux (2740-2757)
• is_helper_usable_linux (2740-2740)
• is_helper_usable_by_name_linux (2759-2770)
• is_helper_usable_by_name_linux (2759-2759)

src/linux/linux_platform.hpp (3)

• get_helper_prototype_by_name_linux (9-9)
• is_helper_usable_linux (10-10)
• is_helper_usable_by_name_linux (11-11)

src/test/test_marshal.cpp (2)

src/ir/unmarshal.cpp (4)

• unmarshal (836-845)
• unmarshal (836-836)
• unmarshal (847-850)
• unmarshal (847-847)

src/ir/unmarshal.hpp (2)

• unmarshal (22-23)
• unmarshal (24-24)

src/ir/unmarshal.cpp (2)

src/elf_loader.cpp (3)

• name (663-663)
• name (665-665)
• name (692-692)

src/platform.hpp (2)

• name (21-21)
• name (24-24)

src/linux/gpl/spec_prototypes.cpp (2)

src/platform.hpp (4)

• name (21-21)
• name (24-24)
• n (20-20)
• n (23-23)

src/linux/linux_platform.hpp (3)

• is_helper_usable_by_name_linux (11-11)
• is_helper_usable_linux (10-10)
• get_helper_prototype_by_name_linux (9-9)

src/elf_loader.cpp (1)

src/platform.hpp (2)

• name (21-21)
• name (24-24)

🪛 Clang (14.0.6)

src/linux/linux_platform.hpp

[warning] 9-9: use a trailing return type for this function

(modernize-use-trailing-return-type)

---

[warning] 10-10: use a trailing return type for this function

(modernize-use-trailing-return-type)

---

[warning] 11-11: use a trailing return type for this function

(modernize-use-trailing-return-type)

src/platform.hpp

[warning] 21-21: use 'using' instead of 'typedef'

(modernize-use-using)

---

[warning] 23-23: use 'using' instead of 'typedef'

(modernize-use-using)

---

[warning] 24-24: use 'using' instead of 'typedef'

(modernize-use-using)

---

[warning] 43-43: member variable 'get_helper_prototype_by_name' has public visibility

(misc-non-private-member-variables-in-classes)

---

[warning] 44-44: member variable 'is_helper_usable' has public visibility

(misc-non-private-member-variables-in-classes)

---

[warning] 45-45: member variable 'is_helper_usable_by_name' has public visibility

(misc-non-private-member-variables-in-classes)

src/test/test_verify.cpp

[warning] 544-544: variable 'TEST_CASE' is non-const and globally accessible, consider making it const

(cppcoreguidelines-avoid-non-const-global-variables)

src/ir/unmarshal.cpp

[warning] 86-86: member variable 'relocations' has public visibility

(misc-non-private-member-variables-in-classes)

---

[warning] 91-91: member variable '' has public visibility

(misc-non-private-member-variables-in-classes)

src/linux/gpl/spec_prototypes.cpp

[warning] 2759-2759: use a trailing return type for this function

(modernize-use-trailing-return-type)

---

[warning] 2759-2759: parameter 'name' is unused

(misc-unused-parameters)

---

[warning] 2772-2772: use a trailing return type for this function

(modernize-use-trailing-return-type)

---

[warning] 2772-2772: parameter 'n' is unused

(misc-unused-parameters)

---

[warning] 2779-2779: use a trailing return type for this function

(modernize-use-trailing-return-type)

---

[warning] 2779-2779: parameter 'name' is unused

(misc-unused-parameters)

---

[warning] 2779-2779: parameter 'out_id' is unused

(misc-unused-parameters)

⏰ 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). (11)

• GitHub Check: build_ubuntu (Debug)
• GitHub Check: build_ubuntu (Release)
• GitHub Check: build_ubuntu (Release, library)
• GitHub Check: build_windows (Release, library)
• GitHub Check: build_ubuntu (Debug, library)
• GitHub Check: build_windows (Debug, tests)
• GitHub Check: build_windows (Debug, library)
• GitHub Check: build_ubuntu (Release, tests)
• GitHub Check: build_windows (Release, tests)
• GitHub Check: build_ubuntu (Debug, tests)
• GitHub Check: Analyze (cpp)

🔇 Additional comments (11)

ebpf-samples (1)

1-1: Submodule update verified.

The new commit c5759d90e24d4241a488b4ac6a3904ccec87e0e2 exists in vbpf/ebpf-samples and includes the commit "Add test with an external helper function (test for resolve by name)", which directly supports the feature being implemented in this PR.

src/linux/linux_platform.cpp (1)

241-253: Wire-in by-name helper hooks

Initializer ordering matches ebpf_platform_t; no functional issues spotted.

src/test/test_marshal.cpp (1)

835-878: Unmarshal external helper by-name: coverage is correct

Exercises success and two failure paths; aligns with relocation-driven resolution.

src/linux/gpl/spec_prototypes.cpp (2)

2759-2770: Name-based usability check is consistent with numeric path

Prefix trim + linear lookup + existing context guard; OK.

---

2779-2794: By-name prototype lookup with out_id: OK

Returns prototype, sets id, and errors clearly on not-found/unusable.

src/elf_loader.cpp (1)

1069-1079: Per-program relocation capture and clear

Attaching relocations to RawProgram and clearing per iteration is correct.

src/platform.hpp (1)

21-25: By-name hook typedefs added

API surface looks fine.

src/ir/unmarshal.cpp (4)

86-86: LGTM!

The relocations map is properly declared and initialized. The public visibility flagged by static analysis is intentional for this design, enabling external function name resolution during unmarshalling.

Also applies to: 91-93

---

492-503: LGTM!

The variant approach cleanly supports both ID-based and name-based helper resolution. The implementation correctly captures the resolved func_id from the name-based path and uses it to populate the Call struct.

---

839-839: LGTM!

Correctly propagates relocations from RawProgram to enable name-based helper resolution during unmarshalling.

---

505-508: LGTM!

Error handling is comprehensive with clear messages for all failure paths: unresolved relocations, invalid helper names, invalid helper IDs, and unsupported functions.

Also applies to: 648-654, 657-659

[elazarg]

@Alan-Jowett merging ebpf-samples caused conflicts. Sorry!

[Alan-Jowett]

No worries. I think I have a simpler solution anyway. Will revise and resubmit

[Alan-Jowett]

I think I can actually constraint the change to the ELF parsing code and have it patch the instructions with the correct helper id and leave the unmarshalling unchanged.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4227187 and a3da20b.

📒 Files selected for processing (6)

• src/elf_loader.cpp (1 hunks)
• src/linux/gpl/spec_prototypes.cpp (1 hunks)
• src/linux/linux_platform.cpp (1 hunks)
• src/linux/linux_platform.hpp (1 hunks)
• src/platform.hpp (2 hunks)
• src/test/test_verify.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (5)

**/*.{c,cc,cpp,cxx,h,hpp,hxx}

📄 CodeRabbit inference engine (AGENTS.md)

Run ./scripts/format-code --staged (clang-format) before committing; ensure formatting matches project baseline

Files:

• src/platform.hpp
• src/elf_loader.cpp
• src/linux/linux_platform.hpp
• src/linux/linux_platform.cpp
• src/linux/gpl/spec_prototypes.cpp
• src/test/test_verify.cpp

**/*.{h,hpp,hxx}

📄 CodeRabbit inference engine (AGENTS.md)

Prefer #pragma once in header files; follow existing patterns within each subdirectory

Files:

• src/platform.hpp
• src/linux/linux_platform.hpp

**/*.{c,cc,cpp,cxx}

📄 CodeRabbit inference engine (AGENTS.md)

Ensure new C/C++ source files include the standard SPDX license header; validate with ./scripts/check-license.sh

Files:

• src/elf_loader.cpp
• src/linux/linux_platform.cpp
• src/linux/gpl/spec_prototypes.cpp
• src/test/test_verify.cpp

src/{cfg,crab,linux,arith,asm_*}/**/*

📄 CodeRabbit inference engine (AGENTS.md)

Add new verifier logic under the matching subsystem directory (cfg/, crab/, linux/, arith/, asm_*) to keep separation of concerns

Files:

• src/linux/linux_platform.hpp
• src/linux/linux_platform.cpp
• src/linux/gpl/spec_prototypes.cpp

src/test/**/*.{cc,cpp,cxx}

📄 CodeRabbit inference engine (AGENTS.md)

src/test/**/*.{cc,cpp,cxx}: Place Catch2 unit/integration tests under src/test and add focused tests when modifying verifier behaviour
When introducing or relying on a new invariant, add a targeted regression test covering the invariant

Files:

• src/test/test_verify.cpp

🧠 Learnings (4)

📚 Learning: 2025-10-15T13:34:52.142Z

Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Before finishing a change, manually review modified control-flow and data-flow to ensure no UB or unchecked user input paths were introduced

Applied to files:

• src/elf_loader.cpp

📚 Learning: 2025-10-15T13:34:52.142Z

Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/test/**/*.{cc,cpp,cxx} : Place Catch2 unit/integration tests under src/test and add focused tests when modifying verifier behaviour

Applied to files:

• src/test/test_verify.cpp

📚 Learning: 2025-10-15T13:34:52.142Z

Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/test/**/*.{cc,cpp,cxx} : When introducing or relying on a new invariant, add a targeted regression test covering the invariant

Applied to files:

• src/test/test_verify.cpp

📚 Learning: 2025-10-15T13:34:52.142Z

Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/{cfg,crab,linux,arith,asm_*}/**/* : Add new verifier logic under the matching subsystem directory (cfg/, crab/, linux/, arith/, asm_*) to keep separation of concerns

Applied to files:

• src/test/test_verify.cpp

🧬 Code graph analysis (3)

src/linux/linux_platform.hpp (1)

src/platform.hpp (1)

• name (24-24)

src/linux/linux_platform.cpp (2)

src/linux/gpl/spec_prototypes.cpp (2)

• get_helper_index_linux (2759-2768)
• get_helper_index_linux (2759-2759)

src/linux/linux_platform.hpp (1)

• get_helper_index_linux (10-10)

src/linux/gpl/spec_prototypes.cpp (1)

src/platform.hpp (1)

• name (24-24)

🪛 Clang (14.0.6)

src/platform.hpp

[warning] 24-24: use 'using' instead of 'typedef'

(modernize-use-using)

---

[warning] 44-44: member variable 'get_helper_index' has public visibility

(misc-non-private-member-variables-in-classes)

src/linux/linux_platform.hpp

[warning] 10-10: use a trailing return type for this function

(modernize-use-trailing-return-type)

src/linux/gpl/spec_prototypes.cpp

[warning] 2759-2759: use a trailing return type for this function

(modernize-use-trailing-return-type)

src/test/test_verify.cpp

[warning] 544-544: variable 'TEST_CASE' is non-const and globally accessible, consider making it const

(cppcoreguidelines-avoid-non-const-global-variables)

⏰ 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 (Release, library)
• GitHub Check: build_ubuntu (Debug, tests)
• GitHub Check: build_windows (Debug, tests)
• GitHub Check: build_ubuntu (Release, tests)
• GitHub Check: build_windows (Release, tests)
• GitHub Check: build_windows (Debug, library)
• GitHub Check: build_ubuntu (Release, library)
• GitHub Check: build_ubuntu (Release)
• GitHub Check: build_ubuntu (Debug)
• GitHub Check: Analyze (cpp)

🔇 Additional comments (6)

src/linux/linux_platform.hpp (1)

10-10: LGTM!

Declaration is consistent with the platform interface typedef.

src/linux/linux_platform.cpp (1)

245-245: Callback wired correctly.

The platform initializer now includes the helper index lookup function in the proper position.

src/platform.hpp (1)

24-24: Typedef added for helper index callback.

Follows the existing pattern for other platform callbacks.

src/linux/gpl/spec_prototypes.cpp (1)

2759-2768: Helper index lookup implemented correctly.

Linear search prepends "bpf_" prefix to each prototype name and returns the matching index. Error message is clear when helper is not found.

src/elf_loader.cpp (1)

954-960: External helper resolution implemented correctly.

The code identifies undefined-symbol local calls (section_index == 0), resolves the helper index via the platform callback, and directly patches the instruction. This approach resolves helpers during ELF load, avoiding the need for deferred resolution during unmarshal.

Note: Past review suggested setting imm = -1 as a sentinel, but that pattern applies to deferred resolution. The current implementation resolves immediately, so the helper index is directly written to imm. This aligns with the PR comment's stated approach: "patch instructions with the correct helper ID during ELF load."

src/test/test_verify.cpp (1)

544-544: Artifact not found in current environment.

The test at line 544 references externalfunction.o, but the ebpf-samples directory is empty and the artifact is missing. The artifact is only referenced in this single test location with no other references in the codebase.

Verify manually that:

1. The ebpf-samples submodule is properly checked out with all artifacts
2. externalfunction.o exists in the ebpf-samples directory after pulling the PR branch

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a3da20b and fdb0343.

📒 Files selected for processing (2)

• src/elf_loader.cpp (1 hunks)
• src/linux/gpl/spec_prototypes.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{c,cc,cpp,cxx,h,hpp,hxx}

📄 CodeRabbit inference engine (AGENTS.md)

Run ./scripts/format-code --staged (clang-format) before committing; ensure formatting matches project baseline

Files:

• src/linux/gpl/spec_prototypes.cpp
• src/elf_loader.cpp

**/*.{c,cc,cpp,cxx}

📄 CodeRabbit inference engine (AGENTS.md)

Ensure new C/C++ source files include the standard SPDX license header; validate with ./scripts/check-license.sh

Files:

• src/linux/gpl/spec_prototypes.cpp
• src/elf_loader.cpp

src/{cfg,crab,linux,arith,asm_*}/**/*

📄 CodeRabbit inference engine (AGENTS.md)

Add new verifier logic under the matching subsystem directory (cfg/, crab/, linux/, arith/, asm_*) to keep separation of concerns

Files:

• src/linux/gpl/spec_prototypes.cpp

🧠 Learnings (1)

📚 Learning: 2025-10-15T13:34:52.142Z

Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Before finishing a change, manually review modified control-flow and data-flow to ensure no UB or unchecked user input paths were introduced

Applied to files:

• src/elf_loader.cpp

🧬 Code graph analysis (1)

src/linux/gpl/spec_prototypes.cpp (1)

src/platform.hpp (1)

• name (24-24)

🪛 Clang (14.0.6)

src/linux/gpl/spec_prototypes.cpp

[warning] 2759-2759: use a trailing return type for this function

(modernize-use-trailing-return-type)

---

[warning] 2759-2759: parameter 'name' is unused

(misc-unused-parameters)

src/elf_loader.cpp

[warning] 957-957: variable 'helper_index' is not initialized

(cppcoreguidelines-init-variables)

⏰ 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). (11)

• GitHub Check: build_ubuntu (Release)
• GitHub Check: build_ubuntu (Debug)
• GitHub Check: build_ubuntu (Release, library)
• GitHub Check: build_windows (Release, tests)
• GitHub Check: build_ubuntu (Debug, tests)
• GitHub Check: build_windows (Release, library)
• GitHub Check: build_ubuntu (Debug, library)
• GitHub Check: build_ubuntu (Release, tests)
• GitHub Check: build_windows (Debug, tests)
• GitHub Check: build_windows (Debug, library)
• GitHub Check: Analyze (cpp)

🔇 Additional comments (1)

src/linux/gpl/spec_prototypes.cpp (1)

2759-2768: Implementation is correct.

The linear search with prefix matching correctly resolves helper names to indices. The -1 sentinel for "not found" aligns with the platform interface contract.

[coveralls]

Pull Request Test Coverage Report for Build 19040038659

Details

• 15 of 17 (88.24%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.003%) to 86.939%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/test/ebpf_yaml.cpp	0	2	0.0%

Totals
Change from base Build 19018342762:	0.003%
Covered Lines:	9059
Relevant Lines:	10420

---

💛 - Coveralls

[Alan-Jowett]

@dthaler should this be rethought and instead be BTF based?

[dthaler]

@dthaler should this be rethought and instead be BTF based?

I haven't reviewed this PR yet and I'm in IETF meetings now, but based on the PR title: yes :)
Helper IDs are arguably deprecated in favor of BTF IDs (used with a different BPF instruction).

[Alan-Jowett]

@dthaler should this be rethought and instead be BTF based?

I haven't reviewed this PR yet and I'm in IETF meetings now, but based on the PR title: yes :) Helper IDs are arguably deprecated in favor of BTF IDs (used with a different BPF instruction).

No rush, this can wait until after IETF. This is indeed part of the implementation of microsoft/ebpf-for-windows#4795

My only concern with taking a dependency on BTF is that this is currently a tool chain specific feature. We would need to figure out how to generate BTF data for helper functions when the provider is compiled using MSVC tool chains.

[Alan-Jowett]

@elazarg should we hold off merging this in light of @dthaler feedback?

Given that call by helper ID is deprecated, should we revise this change to do the following:

1. Have the elf_loader.cpp patch the instruction with source register = 2 and imm = BTF ID of the function
2. Have unmarshall.cpp lookup the BTF data for the function by BTF id, pass it to a platform function, and get back the EbpfHelperPrototype.
3. Unmarshall.cpp the constructs a Call object in the same way it does for helper function by id path.

[dthaler]

@elazarg should we hold off merging this in light of @dthaler feedback?

Given that call by helper ID is deprecated, should we revise this change to do the following:

1. Have the elf_loader.cpp patch the instruction with source register = 2 and imm = BTF ID of the function
2. Have unmarshall.cpp lookup the BTF data for the function by BTF id, pass it to a platform function, and get back the EbpfHelperPrototype.
3. Unmarshall.cpp the constructs a Call object in the same way it does for helper function by id path.

My opinion: I think that would be the best approach.

[elazarg]

Maybe better merge this as a quick fix and then do it the right way?

[dthaler]

Maybe better merge this as a quick fix and then do it the right way?

I'd ask Alan whether a "quick fix" is actually needed. It sounds like he's saying this can wait. After reviewing microsoft/ebpf-for-windows#4795 more, I question whether this actually meets the requirements (the requirements listed in that PR are self-contradictory I now believe, and my proposed resolution would mean this PR would likely not meet the requirements).