Skip to content
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

Add support for bpf2bpf function calls #608

Merged
merged 4 commits into from
Sep 2, 2024
Merged

Conversation

dthaler
Copy link
Contributor

@dthaler dthaler commented Mar 22, 2024

Implemented approach discussed in meeting between Dave, Elazar, Arie, and others.
Each function is treated as a "macro" and CFG nodes are created as if the compiler had inlined the functions. The new nodes use variables prefixed with the conceptual call stack, so that nesting and multiple calls to the same function are both supported. E.g., "1/5/r6.svalue=0" means the svalue property of r6 in a macro called from label 5 which was in turn called from label 1.

As tested by the bpf_conformance suite, it turns out that an Exit instruction is responsible for restoring registers R6-R9 (as opposed to being restored by other instructions generated by the compiler). Hence the CallLocal instruction makes copies of R6-R9 state, and the Exit instruction restores R6-R9 from the copies, and cleans up the copied state.

This PR also:

  • fixes a use-after-free bug in wto.cpp where args2 state was being read after a pop() that could free that memory.
  • adds a "simplify" option for YAML tests that want to test behavior without no_simplify. Previously all YAML tests used no_simplify, but we needed to test CallLocal both with and without the simplify step in order to fully test the CFG changes.
  • updates the template matching code in test_marshal.cpp to correctly deal with fields with wildcards, to resolve an issue hit when testing this PR.

Fixes #451

@dthaler dthaler force-pushed the bpf2bpf branch 5 times, most recently from 385e867 to 5abb981 Compare March 22, 2024 23:17
@coveralls
Copy link

coveralls commented Mar 23, 2024

Coverage Status

coverage: 90.417% (-0.01%) from 90.431%
when pulling e2cf59a on dthaler:bpf2bpf
into cd399bb on vbpf:main.

@elazarg
Copy link
Collaborator

elazarg commented May 9, 2024

I don't think handling function calls as macros at the CFG level is the right approach, and I do not believe Arie and @caballa supported this approach. Inlining (as opposed to computing a summary) can be implemented by letting the abstract interpreter treat the code in same way a concrete interpreter would: jump to the start of the function with a copy of the abstract state, execute normally, then jump back with the resulting state.

In order to have the same behavior as in the macro approach you'd need to index the tracked states (instead of the instructions) with the same conceptual call stack, but I think this approach is more flexible in case we'd want to implement alternative approaches to inlining. For example, we can join on entry from all caller sites and thus converge faster, probably with a simpler debug output (also maybe slightly more useful proof-carrying-code in the future).

@dthaler
Copy link
Contributor Author

dthaler commented May 9, 2024

I don't think handling function calls as macros at the CFG level is the right approach, and I do not believe Arie and @caballa supported this approach.

This is the approach I understood on the call. Would like Arie and @caballa to weigh in.

Inlining (as opposed to computing a summary) can be implemented by letting the abstract interpreter treat the code in same way a concrete interpreter would: jump to the start of the function with a copy of the abstract state, execute normally, then jump back with the resulting state.

I don't follow what that means. What I do know is that this PR does work.
Do we need to have another Zoom call?

In order to have the same behavior as in the macro approach you'd need to index the tracked states (instead of the instructions) with the same conceptual call stack, but I think this approach is more flexible in case we'd want to implement alternative approaches to inlining. For example, we can join on entry from all caller sites and thus converge faster, probably with a simpler debug output (also maybe slightly more useful proof-carrying-code in the future).

@caballa
Copy link
Collaborator

caballa commented May 9, 2024

I read the slides given by @dthaler and asked Arie about the meeting you had and what I understand @dthaler has implemented is consistent with what I remember.

@elazarg suggests a "dynamic" inlining approach. By "dynamic", I mean that the abstract interpreter actually does the inlining as it goes. There are several advantages of doing that. Elazar mentioned some. Another important advantage is that a function call wouldn't be inlined if it's unreachable.

What @dthaler has implemented is more like a "static" inlining approach by doing some program transformation (I haven't looked at the PR but this is my guess).

Both approaches are compatible. We can start with Dave's approach since it's already implemented and tested and we can always implement what Elazar suggests if needed.

@dthaler dthaler force-pushed the bpf2bpf branch 2 times, most recently from 27a1a27 to fd3ec3b Compare May 15, 2024 13:20
@dthaler
Copy link
Contributor Author

dthaler commented May 15, 2024

Code coverage failure seems to be a transient failure due to coveralls backend, not specific to this PR.

Copy link
Collaborator

@elazarg elazarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry about the delay. I still have my reservations, but this approach does the job.

Below I added some comments and proposed additional tests. The most important comment is probably the handling of correlations in callee-saved registers. Let me know if you think it should be addressed now or later on.

src/crab/cfg.hpp Outdated Show resolved Hide resolved
src/asm_cfg.cpp Outdated Show resolved Hide resolved
src/asm_cfg.cpp Outdated Show resolved Hide resolved
src/asm_cfg.cpp Outdated Show resolved Hide resolved
src/asm_cfg.cpp Outdated Show resolved Hide resolved
src/asm_cfg.cpp Show resolved Hide resolved
src/asm_cfg.cpp Outdated Show resolved Hide resolved
src/asm_syntax.hpp Show resolved Hide resolved
src/crab/ebpf_domain.cpp Outdated Show resolved Hide resolved
src/crab/ebpf_domain.cpp Show resolved Hide resolved
@shankarseal
Copy link

@dthaler @elazarg the eBPF for Windows project would like to onboard bpf2bpf function call support to investigate if this can replace tail calls (among others) to improve performance of bpf programs in network datapath. Can you please continue this PR (which seems stalled) and resolve the remaining issues?

Signed-off-by: Elazar Gershuni <[email protected]>
Also - failed test for correlations between registers

Signed-off-by: Elazar Gershuni <[email protected]>
@elazarg elazarg merged commit 37bf146 into vbpf:main Sep 2, 2024
14 checks passed
@elazarg
Copy link
Collaborator

elazarg commented Sep 2, 2024

@shankarseal done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevail does not support function calls
5 participants