Skip to content

update bpf abi to match Clang 23 - #161076

Open
folkertdev wants to merge 1 commit into
rust-lang:mainfrom
folkertdev:bpf-structs
Open

update bpf abi to match Clang 23#161076
folkertdev wants to merge 1 commit into
rust-lang:mainfrom
folkertdev:bpf-structs

Conversation

@folkertdev

@folkertdev folkertdev commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

mirror the behavior of llvm/llvm-project#206876

apparently we have no target maintainers for the bpf targets... fun.

@folkertdev folkertdev added the O-eBPF Target: I heard you liked code execution so I put some code execution in your code execution label Aug 13, 2026
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 13, 2026
@folkertdev folkertdev changed the title update bpf abi to match LLVM 23 update bpf abi to match Clang 23 Aug 13, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@folkertdev
folkertdev marked this pull request as ready for review August 14, 2026 09:29
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 14, 2026
@rustbot

rustbot commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 75 candidates
  • Random selection from 17 candidates

@amirHdev

amirHdev commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

I checked the C ABI path with extern "C" using the same aggregate cases as the Clang change
With Clang 23.1.0-rc1 from the LLVM revision used by this checkout bpfel and bpfeb produce the same return ABI as this PR. also checked the direct return call sites and those match as well

Since the new tests here are using the Rust ABI would it make sense to add a small extern "C" regression test too?

@folkertdev

folkertdev commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for taking a look!

Since the new tests here are using the Rust ABI would it make sense to add a small extern "C" regression test too?

Whoops, should have done this while more awake. In practice they are the same so that is why the CHECKs work but given that this tests ABI the tests should use extern "C".

@rustbot

rustbot commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

folkertdev
folkertdev commented Aug 15, 2026
View reviewed changes

@folkertdev folkertdev left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed, and I added some bpfeb revisions too. It looks like BE and LE are the same though (also with clang).

View changes since this review

Comment thread tests/codegen-llvm/bpf-abi/struct-return-regs.rs
Comment on lines +43 to +48
// CHECK-LABEL: define{{.*}} i8 @bar0(
// CHECK: ret i8
#[no_mangle]
extern "C" fn bar0(a: i8) -> Foo0 {
Foo0 { a }
}

@folkertdev folkertdev Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for BE this is a bit surprising to me, in many targets this would instead pass an i32 or i64 on BE so that the 8 actual bits are in the right spot. But, this is consistent with clang

https://godbolt.org/z/PvMj78bWs

vadorovsky
vadorovsky reviewed Aug 15, 2026
View reviewed changes

@vadorovsky vadorovsky left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, but I would like to make sure that this doesn't break any integration tests of https://github.com/aya-rs/aya. I will come back with an answer at the start of coming week.

View changes since this review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@vadorovsky vadorovsky vadorovsky left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@jieyouxu jieyouxu

Labels

O-eBPF Target: I heard you liked code execution so I put some code execution in your code execution S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@folkertdev @rust-log-analyzer @rustbot @amirHdev @vadorovsky @jieyouxu