Recommendation around panic-handler and use of default-members

[thomaseizinger]

Instead of using default-members, I opted for the following in my eBPF code:

#[cfg(target_arch = "bpf")]
extern crate panic_halt; // Defines a no-op panic handler (we always compile with `immediate_abort` anyway).

This allows me to just run cargo check in my workspace without the compiler barking at me that I am defining two panic handlers (as the std one is part of the dependency tree via the shared crate and its optional std dependency).

This also means rust-analyzer runs cargo check for this crate as well and provides its lint output.

[tamird]

We intentionally don't want a no-op panic handler because that would potentially cause panicking programs to verify.

But I think you can just update the existing panic handlers we have in the examples to be conditional on target_arch and remove default-members.

Would you like to send a PR?

[thomaseizinger]

But I think you can just update the existing panic handlers we have in the examples to be conditional on target_arch and remove default-members.

Would you like to send a PR?

Yep can do! Was there any other reason why you use default-members?

We intentionally don't want a no-op panic handler because that would potentially cause panicking programs to verify.

The panic-halt crate is the same code though as used in the hello-xdp example: loop {}

https://github.com/korken89/panic-halt/blob/master/src/lib.rs

[tamird]

But I think you can just update the existing panic handlers we have in the examples to be conditional on target_arch and remove default-members. Would you like to send a PR?

Yep can do! Was there any other reason why you use default-members?

I honestly don't remember. CI will let us know.

We intentionally don't want a no-op panic handler because that would potentially cause panicking programs to verify.

The panic-halt crate is the same code though as used in the hello-xdp example: loop {}

https://github.com/korken89/panic-halt/blob/master/src/lib.rs

Ah, I hadn't looked. I just went by your description.

[thomaseizinger]

I called it "no-op" because that is essentially what it is compared to crates like panic-abort which call into intrinsics.

In my experience with eBPF, code that actually wants to panic doesn't compile in the first place. LLVM complains that abort is an undefined symbol or something. So really, this is just a place-holder.

To be fair, it is probably not needed to use a crate. I just used it because I couldn't figure out how to configure rust-analyzer to type-check my code without errors. Correctly feature-gateing (which I discovered now) works so we can go back to the inline panic-handler :)

[thomaseizinger]

So I sent a bunch of PRs. Weirdly enough, I couldn't replicate in your repo what I have working in mine in regards to removing default_members. I don't need it in our workspace and cargo test / cargo build across the workspace still works fine.

[thomaseizinger]

Aha!

What we can do is the following:

diff --git a/examples/aya-tool/myapp-ebpf/src/main.rs b/examples/aya-tool/myapp-ebpf/src/main.rs
index 1892359..31b4ace 100644
--- a/examples/aya-tool/myapp-ebpf/src/main.rs
+++ b/examples/aya-tool/myapp-ebpf/src/main.rs
@@ -1,5 +1,5 @@
-#![no_std]
-#![no_main]
+#![cfg_attr(target_arch = "bpf", no_std)]
+#![cfg_attr(target_arch = "bpf", no_main)]
 
 #[allow(clippy::all)]
 #[allow(dead_code)]
@@ -51,3 +51,8 @@ unsafe fn try_task_alloc(ctx: LsmContext) -> Result<i32, i32> {
 fn panic(_info: &core::panic::PanicInfo) -> ! {
     loop {}
 }
+
+#[cfg(not(target_arch = "bpf"))]
+fn main() {
+    panic!("This program is meant to be compiled as an eBPF program.");
+}

We can conditionally opt-in to no-std and no-main only if we compile for the eBPF architecture. It means we need to add an fn main() stub though. With this however, cargo build, cargo clippy etc run all just fine on the host system, even for the eBPF crate.

Thoughts?

[tamird]

How does this build for you, without this stub?

[thomaseizinger]

What do you mean how does it build for me?

Without the main stub, it doesn't build unless you build with --target bpfel-unknown-none. That is the issue I'd like to solve (and is what necessitates default-members as far as I understand).

The idea was, if we feature-gate no_std and no_main on the correct architecture, then you can actually build (but not run) the eBPF kernel and thus tools like rust-analyzer and cargo check in the workspace just work.

[tamird]

I meant in your repo, with which your experience motivated this line of PRs.

[tamird]

Clarifying further: you said at the top of this issue that you opted for that panic_halt crate and removed default-members. What I'm asking is: how does that work, given it doesn't work here in the aya repo?

[thomaseizinger]

Clarifying further: you said at the top of this issue that you opted for that panic_halt crate and removed default-members. What I'm asking is: how does that work, given it doesn't work here in the aya repo?

Ah, I wasn't clear enough then sorry about that. The important part isn't the external panic handler, the important part is using the correct cfg: target_arch = "bpf" instead of cfg(test)!

I've actually gone back to an inline panic handler in my repo to remove the external dependency again. The only reason why I introduced it was because rust-analyzer (which I have configured to run with --all-targets --all-features) barked at me for defining two panic handlers. And indeed, if you run cargo check --all-targets --all-features without defining default-members, that will fail. Moving the panic handler to another crate solved that at least for rust-analyzer.

I opened the PRs because I think using the correct cfg makes the default experience much better and shouldn't be very controversial.

I do think there is more room for improving the experience here. If we cfg the no_std and no_main attributes and add a main() stub, we can also get rid of default-members because cargo check --workspace will just work. However, one could argue that because the eBPF could should never be run on any other architecture, it also shouldn't compile in the first place. Plus, users may get confused as to why this boilerplate with cfg is needed.

What do you think? Personally, I'd say getting rid of default-members would be worth it as it is a pain to maintain once your repo gets bigger (we have 20+ crates in our workspace). IMO, tuning code the code such that it has good defaults (and works with the defaults of other tools) is more important than the keeping the eBPF code as lean and "ideal" as possible. For example, in a larger workspace, not every team-member may need to touch the eBPF code, hence it would be good if e.g. cargo test --workspace --all-features just works for them.

[thomaseizinger]

Thinking more about it, I think those changes can't actually be meaningfully separated as they are too intertwined:

• Feature-gating the panic_handler on target_arch = "bpf" only really makes sense if we also feature-gate no_std on the same. Those two are actually what go together semantically: If you opt out of std, you need to define a panic handler.
• In order to remove default-members, we also need to ensure the eBPF code compiles for the host architecture. That doesn't work unless you define a main function so we also need to feature-gate no_main on the target architecture and provide a stub main() when compiling for the host.

Doing both of test means cargo build and cargo test just work for the eBPF kernel and there is no more need to define default-members because all cargo commands just also work for the eBPF crate.

If you agree to this, I can go ahead and update all the PRs to make those changes.

What we can do in a follow-up IMO is reducing some of that boilerplate.

• For example, the panic_handler definition could be moved to aya_ebpf (https://github.com/aya-rs/aya/issues/1230).
• We could introduce a macro for the main() stub so people don't have to define their own.

[tamird]

These super-verbose replies aren't helpful, I think. My question is very simple: you wrote at the top of this issue that removing default-members works in your workspace, but you don't have the main stub, right? How does that work?

[thomaseizinger]

These super-verbose replies aren't helpful, I think. My question is very simple: you wrote at the top of this issue that removing default-members works in your workspace, but you don't have the main stub, right? How does that work?

I said that running cargo check works. If we want to have cargo build and cargo test work as well, we need to add the stub.

Weirdly enough, I couldn't replicate in your repo what I have working in mine in regards to removing default_members. I don't need it in our workspace and cargo test / cargo build across the workspace still works fine.

This was a fail-guided conclusion. The only thing that works without the stub is cargo check.

[tamird]

Got it. OK, I think we're gonna need to update:

• aya-rs/aya
• aya-rs/aya-template
• aya-rs/book

Let's start with the main repo. The goal is to remove default-members. Can you update https://github.com/aya-rs/aya/pull/1229?

[skewballfox]

What we can do is the following:

diff --git a/examples/aya-tool/myapp-ebpf/src/main.rs b/examples/aya-tool/myapp-ebpf/src/main.rs index 1892359..31b4ace 100644 --- a/examples/aya-tool/myapp-ebpf/src/main.rs +++ b/examples/aya-tool/myapp-ebpf/src/main.rs @@ -1,5 +1,5 @@ -#![no_std] -#![no_main] +#![cfg_attr(target_arch = "bpf", no_std)] +#![cfg_attr(target_arch = "bpf", no_main)]

#[allow(clippy::all)] #[allow(dead_code)] @@ -51,3 +51,8 @@ unsafe fn try_task_alloc(ctx: LsmContext) -> Result<i32, i32> { fn panic(_info: &core::panic::PanicInfo) -> ! { loop {} } + +#[cfg(not(target_arch = "bpf"))] +fn main() {

• panic!("This program is meant to be compiled as an eBPF program."); +} We can conditionally opt-in to no-std and no-main only if we compile for the eBPF architecture. It means we need to add an fn main() stub though. With this however, cargo build, cargo clippy etc run all just fine on the host system, even for the eBPF crate.

I just spent a few days trying to get test and ci checks to still work when an ebpf module is added to my project's workspace, and this just fixed the issue. thanks!

[thomaseizinger]

I am glad it is helpful!

It would be nice to have this in the book in some form so it is more discoverable. In my experience, feature-gating the attributes is the best trade-off in terms of ergonomics and correctness.

[tamird]

@thomaseizinger can you please summarize where we landed on this?

[thomaseizinger]

Sure! As far as I remember from https://github.com/aya-rs/aya/pull/1229, the summary is that the aya maintainers considered it better / more correct to:

• Ensure an eBPF kernel crate only ever compiles when targeting a bpf target (i.e. it should not compile with stubs on a host architecture such as x86)
• Use cargo features such as default-members to exclude eBPF kernel crates from commands such as cargo check or cargo clippy when run on the entire workspace

My last comment (https://github.com/aya-rs/aya/pull/1229#issuecomment-2777368144) contains some more information as to why I disagree with that direction and led to the PR being closed. Unless something has changed in your position regarding that, we can also close this issue.