Dont internalize extern definitions and add proper section info - #317
Conversation
vadorovsky
left a comment
There was a problem hiding this comment.
Nice work and thanks for adding the compiler test! Just couple of nits and after that I think it's good to go. 🙂
@vadorovsky reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions
-- commits line 2 at r1:
nit: s/Dont/Don't/
src/llvm/mod.rs line 270 at r1 (raw file):
if num_blocks == 0 { unsafe { LLVMSetSection(value, c".ksyms".as_ptr()) }; info!(
In DISanitizer (https://github.com/aya-rs/bpf-linker/blob/main/src/llvm/di.rs) we seem to use mostly trace! log level for fixups, let's use it here too instead of info!.
src/llvm/mod.rs line 277 at r1 (raw file):
} } if unsafe { !LLVMIsAGlobalVariable(value).is_null() } {
As clippy suggests, you can merge these two if stamements into one.
|
let's not merge this until we have kfuncs working e2e in aya since this might not be enough |
eebd6c6 to
af91924
Compare
af91924 to
2444bda
Compare
|
Hi! I've recently tried to use kfuncs from a Rust ebpf program via C shims and this PR did come up very handy as bpf-linker did indeed internalize the For reference, here is relevant bit from linker dump: Before After |
|
Hey y'all, hope y'all are doing well. I was wondering what the status of this change is. The last CI run is from February and its logs have expired. The full test suite passes locally with this commit cherry-picked onto current main (with llvm-23), including the new extern_linkage.rs case. Other than the nits, additional test cases, and failed CI builds. what is blocking it from being merged? As @domwst mentioned, this change does fix the issues concerning calling conventions as arguments to functions declared I compiled the following code against Rust nightly, kernel v6.18, and linked it with bpf-linker excluding/including the change in this PR. I generated the LLVM-IR by running LLVM-IR diff: The object dump diff also indicates that the BPF argument register This change also ensures the linker acts correctly when we link in raw LLVM IR bitcode. Using a C shim and the following Rust program we get the following when compiling with and no output when running With the fix we see and respectively. This change does not get us full E2E support for kfunc calling through Rust+aya as rustc still doesn't emit the requisite debug info for foreign functions. However, ksyms support was recently merged into Aya (aya-rs/aya#1372) and there are open PRs that improve on kfunc support (aya-rs/aya#1495). However, these changes still depend on this bpf-linker change as even if the proper logic to resolve kfunc calls is added to aya (the loader) exclusively, loading the program would still fail as the linker has failed to provide the correct signatures and function preludes for the kfuncs that require a non-zero number of arguments. While these changes are being worked on, this change would still unblock kfunc calling through Rust+aya if C shims are also utilized. If any help is needed with getting this change ready to be merged I am more than willing to assist. |
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
The stated calling-convention preservation behavior lacks a regression case using an explicit calling convention.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Preserves external declarations during LLVM optimization and debug-info sanitization, enabling correct BTF .ksyms generation.
Changes:
- Skip internalization and debug-linkage rewriting for declarations.
- Add linkage and BTF regression tests.
- Add Bazel fixtures for external C bitcode.
File summaries
| File | Description |
|---|---|
src/llvm/mod.rs |
Preserves declaration linkage. |
src/llvm/di.rs |
Preserves declaration debug linkage. |
tests/extern_linkage.rs |
Tests external and weak linkage. |
tests/c/extern.c |
Provides a .ksyms fixture. |
tests/btf/assembly/extern.rs |
Verifies external-function BTF output. |
tests/BUILD.bazel |
Registers the new tests and fixture. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5e63764 to
d7a04ca
Compare
Internalizing unresolved functions lets LLVM treat them as local definitions and drop arguments from calls. Leave function and global declarations unchanged, including their visibility and weak linkage. Exclude declarations from debug-info linkage rewriting as well. Marking a C kfunc declaration as a local definition removes its BTF FUNC record and .ksyms entry, preventing the loader from resolving it. Use one compiler fixture with optimized-IR and BTF revisions to check strong and weak declarations and external-function BTF metadata. Detect ELF output before dumping BTF so both revisions can share the source. Supply declaration debug info through fixed LLVM IR: Clang 18 omits it at -O0, while newer Clang bitcode can be unreadable by older supported LLVM. Correct the existing export test's conditional directives so its cdylib and bin revisions select their intended crate types. Share their identical assertions using the common CHECK prefix. Co-authored-by: altug bozkurt <altug.bozkurt09@gmail.com>
vadorovsky
left a comment
There was a problem hiding this comment.
One (old) comment, after that let's ship it.
@vadorovsky partially reviewed 7 files, made 2 comments, and resolved 1 discussion.
Reviewable status: 4 of 8 files reviewed, 3 unresolved discussions (waiting on altugbozkurt07 and tamird).
src/llvm/mod.rs line 270 at r1 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
In
DISanitizer(https://github.com/aya-rs/bpf-linker/blob/main/src/llvm/di.rs) we seem to use mostlytrace!log level for fixups, let's use it here too instead ofinfo!.
@tamird This is still not addressed. 🙂 info! is too noisy for this log IMO.
tamird
left a comment
There was a problem hiding this comment.
@tamird made 1 comment.
Reviewable status: 4 of 8 files reviewed, 2 unresolved discussions (waiting on altugbozkurt07, tamird, and vadorovsky).
src/llvm/mod.rs line 270 at r1 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
@tamird This is still not addressed. 🙂
info!is too noisy for this log IMO.
there's no info here...?
|
Previously, tamird (Tamir Duberstein) wrote…
OK, yeah, it's just reviewable hallucinating |
|
I just realized that we can use Rust with C shims as a workaround for kfuncs or CO-RE. Could we document this, perhaps in the aya-book? I can open a PR to do that. |
|
Sure. |

As per our discussion with @alessandrod , i added a small fix to make sure extern declarations preserve linkage/calling-convention and add the proper section info to hint the llvm backend for btf datasec generation for ksyms.
This change is