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

BPF target support #79608

Merged
merged 14 commits into from
Jun 6, 2021
Merged

BPF target support #79608

merged 14 commits into from
Jun 6, 2021

Conversation

alessandrod
Copy link
Contributor

This adds bpfel-unknown-none and bpfeb-unknown-none, two new no_std targets that generate little and big endian BPF. The approach taken is very similar to the cuda target, where TargetOptions::obj_is_bitcode is enabled and code generation is done by the linker.

I added the targets to dist-various-2. There are some tests in bpf-linker and I'm planning to add more. Those are currently not ran as part of rust CI.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2020
@matthewjasper
Copy link
Contributor

cc @rust-lang/compiler @rust-lang/infra

src/bootstrap/native.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

I am uncertain that we should be expanding our set of targets right now, I'd really like to get a policy accepted before we do so (cc @joshtriplett). I am inclined to not land this PR at this time.

@Mark-Simulacrum
Copy link
Member

I have asked for feedback on Zulip, but I am still inclined that we not merge these targets at tier 2 status for now. I think I could be sold on tier 3, but someone else would need to approve the target definitions (I don't know much about BPF).

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2020
@alessandrod
Copy link
Contributor Author

I'm obviously happy for this to be merged at tier 3 (with a little caveat, see below), but here's my rationale for proposing it at tier 2.

The targets meet the criteria explained in https://doc.rust-lang.org/nightly/rustc/platform-support.html#tier-2. Target/TargetOptions have seen some light refactoring recently but other than that the PR doesn't add anything that would likely cause build breakage at any point. The underlying LLVM BPF target is quite small so it doesn't contribute significantly to build times. It's stable and has been enabled in clang for a long time.

The "emit bitcode, codegen in the linker" approach is identical to the cuda target, which is tier 2. There are a couple of reasons for emitting bitcode. Like NVPTX, BPF supports a subset of Rust (eg, arguments can only be passed in registers, up to a maximum of 5), so it makes sense to push codegen after linking has happened to avoid potentially hitting codegen failures on unused code. Also when targeting older kernels, bpf-linker needs to run more aggressive inlining/loop unrolling optimizations.

The PR doesn't add new tests. I didn't integrate bpf-linker tests in the rust CI because reviewing the NVPTX PRs I saw that people had some concerns about those, and the NVPTX tests were ultimately disabled. bpf-linker tests are compiletest based tho so they can be moved over easily.

The only reason for wanting this at tier 2 sooner rather than later is that while bpf-linker is already usable with rust stable today with -C linker-flavor=wasm-ld -C linker=bpf-linker, deployment is a bit of a nightmare. For things to work rustc and bpf-linker need to use the same LLVM. Rust has been tracking the latest LLVM version for a while now, but that's almost never available in linux distributions. With this in tier 2 I could use rustc's libLLVM from bpf-linker, which would simplify things a lot.

All that said, I wasn't aware of rust-lang/rfcs#2803 when I created the PR, and again I'm happy to wait until that lands.

@joshtriplett
Copy link
Member

@Mark-Simulacrum Working on that policy now.

@apiraino apiraino removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 5, 2021
@apiraino
Copy link
Contributor

apiraino commented Jan 5, 2021

Confirming the decision during the team compiler meeting to wait for the policy to be merged before proceeding.

@nagisa nagisa added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jan 7, 2021
@bors
Copy link
Contributor

bors commented Jan 12, 2021

☔ The latest upstream changes (presumably #80928) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 1, 2021

☔ The latest upstream changes (presumably #84786) made this pull request unmergeable. Please resolve the merge conflicts.

@dvc94ch
Copy link
Contributor

dvc94ch commented May 11, 2021

So now that rust-lang/rfcs#2803 was merged, what does that mean for this target?

@alessandrod
Copy link
Contributor Author

So now that rust-lang/rfcs#2803 was merged, what does that mean for this target?

Working on it! A couple of weeks ago I did a rebase and added inline assembly support. As soon as https://reviews.llvm.org/D102118 gets merged I'll do one more rebase and try to get the conversation going.

@joshtriplett
Copy link
Member

I'd be happy to help review any drafts of BPF's answers to the target tier requirements. (I'd suggest full-quoting the tier 3 requirements and responding to each one, which will make it easy to check them off and confirm that BPF is meeting them.)

@bors
Copy link
Contributor

bors commented Jun 5, 2021

📌 Commit 0adb933 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 5, 2021
BPF target support

This adds `bpfel-unknown-none` and `bpfeb-unknown-none`, two new no_std targets that generate little and big endian BPF. The approach taken is very similar to the cuda target, where `TargetOptions::obj_is_bitcode` is enabled and code generation is done by the linker.

I added the targets to `dist-various-2`. There are [some tests](https://github.com/alessandrod/bpf-linker/tree/main/tests/assembly) in bpf-linker and I'm planning to add more. Those are currently not ran as part of rust CI.
@bors
Copy link
Contributor

bors commented Jun 6, 2021

⌛ Testing commit 0adb933 with merge f434217...

@bors
Copy link
Contributor

bors commented Jun 6, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing f434217 to master...

@alessandrod
Copy link
Contributor Author

Thanks everyone who helped with this 🙏

@dvc94ch
Copy link
Contributor

dvc94ch commented Jun 6, 2021

Thank you for persevering for 6 months.

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jul 7, 2021
BPF target support

This adds `bpfel-unknown-none` and `bpfeb-unknown-none`, two new no_std targets that generate little and big endian BPF. The approach taken is very similar to the cuda target, where `TargetOptions::obj_is_bitcode` is enabled and code generation is done by the linker.

I added the targets to `dist-various-2`. There are [some tests](https://github.com/alessandrod/bpf-linker/tree/main/tests/assembly) in bpf-linker and I'm planning to add more. Those are currently not ran as part of rust CI.
@danobi
Copy link
Contributor

danobi commented Jul 11, 2021

Sorry if this is the wrong place to ask (please redirect me if appropriate).

I'd like to better understand the rationale behind having rustc emit bitcode and the linker doing codegen. There's already prior art in the bpf ecosystem to link ELF objects together ( https://github.com/torvalds/linux/blob/de5540965853e514a85d3b775e9049deb85a2ff3/tools/bpf/bpftool/Documentation/bpftool-gen.rst?plain=1#L28-L31 ).

Admittedly, I don't think it's much of an issue if bpf-linker gets merged into upstream rust.

There are a couple of reasons for emitting bitcode. Like NVPTX, BPF supports a subset of Rust (eg, arguments can only be passed in registers, up to a maximum of 5), so it makes sense to push codegen after linking has happened to avoid potentially hitting codegen failures on unused code.

Could you clarify this point? I'm ignorant as to how this would be different w.r.t. clang and bpf progs written in C.

Also when targeting older kernels, bpf-linker needs to run more aggressive inlining/loop unrolling optimizations.

Could you also clarify this too? I'm not sure how separating out codegen into another binary helps with this issue. It seems more reliable to solve this issue by letter the user annotate their code (always_unroll, never_unroll, etc).

@bjorn3
Copy link
Member

bjorn3 commented Jul 11, 2021

Could you clarify this point? I'm ignorant as to how this would be different w.r.t. clang and bpf progs written in C.

C can be used without a standard library that may use unsupported constructs. Rust requires at least libcore to be compiled. Without it, even something as simple as defining a variable complains about a missing Sized lang item. Or adding two numbers complains about a eg missing impl Add for u8 in case of u8s.

Could you also clarify this too? I'm not sure how separating out codegen into another binary helps with this issue. It seems more reliable to solve this issue by letter the user annotate their code (always_unroll, never_unroll, etc).

Libcore is pre-compiled for a target independent of which kernel is used. This pre-compiled code may not have unrolled loops/inlined functions enough for the running kernel. Moving the codegen into the linker allows the linker to codegen eBPF code accepted by the specific kernel it is going to run on.

@danobi
Copy link
Contributor

danobi commented Jul 12, 2021

Could you clarify this point? I'm ignorant as to how this would be different w.r.t. clang and bpf progs written in C.

C can be used without a standard library that may use unsupported constructs. Rust requires at least libcore to be compiled. Without it, even something as simple as defining a variable complains about a missing Sized lang item. Or adding two numbers complains about a eg missing impl Add for u8 in case of u8s.

Thanks for the explanation. That makes sense to me.

Could you also clarify this too? I'm not sure how separating out codegen into another binary helps with this issue. It seems more reliable to solve this issue by letter the user annotate their code (always_unroll, never_unroll, etc).

Libcore is pre-compiled for a target independent of which kernel is used. This pre-compiled code may not have unrolled loops/inlined functions enough for the running kernel. Moving the codegen into the linker allows the linker to codegen eBPF code accepted by the specific kernel it is going to run on.

So is the idea w/ bpf-linker in a separate binary (rather than embedded in rustc) to let bpf-linker move faster and support more kernels than at rustc release pace? Alternatively, would it ever make sense to embed bpf-linker inside rustc?

@bjorn3
Copy link
Member

bjorn3 commented Jul 12, 2021

So is the idea w/ bpf-linker in a separate binary (rather than embedded in rustc) to let bpf-linker move faster and support more kernels than at rustc release pace?

Not necessarily. The main point as far as I understand is pushing all the codegen to the compilation of the bpf program when the user can choose which kernel to target instead of having part of the codegen happen on rust's CI where it is not known which kernel will be targeted by the user.

Alternatively, would it ever make sense to embed bpf-linker inside rustc?

Maybe?

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Aug 12, 2021
Pkgsrc changes:
 * Bump bootstrap requirements to 1.53.0.
 * Adjust patches, adapt to upstream changes, adjust cargo checksums
 * If using an external llvm, require >= 10.0

Upsteream changes:

Version 1.54.0 (2021-07-29)
============================

Language
-----------------------

- [You can now use macros for values in built-in attribute macros.][83366]
  While a seemingly minor addition on its own, this enables a lot of
  powerful functionality when combined correctly. Most notably you can
  now include external documentation in your crate by writing the following.
  ```rust
  #![doc = include_str!("README.md")]
  ```
  You can also use this to include auto-generated modules:
  ```rust
  #[path = concat!(env!("OUT_DIR"), "/generated.rs")]
  mod generated;
  ```

- [You can now cast between unsized slice types (and types which contain
  unsized slices) in `const fn`.][85078]
- [You can now use multiple generic lifetimes with `impl Trait` where the
   lifetimes don't explicitly outlive another.][84701] In code this means
   that you can now have `impl Trait<'a, 'b>` where as before you could
   only have `impl Trait<'a, 'b> where 'b: 'a`.

Compiler
-----------------------

- [Rustc will now search for custom JSON targets in
  `/lib/rustlib/<target-triple>/target.json` where `/` is the "sysroot"
  directory.][83800] You can find your sysroot directory by running
  `rustc --print sysroot`.
- [Added `wasm` as a `target_family` for WebAssembly platforms.][84072]
- [You can now use `#[target_feature]` on safe functions when targeting
  WebAssembly platforms.][84988]
- [Improved debugger output for enums on Windows MSVC platforms.][85292]
- [Added tier 3\* support for `bpfel-unknown-none`
   and `bpfeb-unknown-none`.][79608]

\* Refer to Rust's [platform support page][platform-support-doc] for more
   information on Rust's tiered platform support.

Libraries
-----------------------

- [`panic::panic_any` will now `#[track_caller]`.][85745]
- [Added `OutOfMemory` as a variant of `io::ErrorKind`.][84744]
- [ `proc_macro::Literal` now implements `FromStr`.][84717]
- [The implementations of vendor intrinsics in core::arch have been
   significantly refactored.][83278] The main user-visible changes are
   a 50% reduction in the size of libcore.rlib and stricter validation
   of constant operands passed to intrinsics. The latter is technically
   a breaking change, but allows Rust to more closely match the C vendor
   intrinsics API.

Stabilized APIs
---------------

- [`BTreeMap::into_keys`]
- [`BTreeMap::into_values`]
- [`HashMap::into_keys`]
- [`HashMap::into_values`]
- [`arch::wasm32`]
- [`VecDeque::binary_search`]
- [`VecDeque::binary_search_by`]
- [`VecDeque::binary_search_by_key`]
- [`VecDeque::partition_point`]

Cargo
-----

- [Added the `--prune <spec>` option to `cargo-tree` to remove a package from
  the dependency graph.][cargo/9520]
- [Added the `--depth` option to `cargo-tree` to print only to a certain depth
  in the tree ][cargo/9499]
- [Added the `no-proc-macro` value to `cargo-tree --edges` to hide procedural
  macro dependencies.][cargo/9488]
- [A new environment variable named `CARGO_TARGET_TMPDIR` is
  available.][cargo/9375]
  This variable points to a directory that integration tests and
  benches can use as a "scratchpad" for testing filesystem operations.

Compatibility Notes
-------------------
- [Mixing Option and Result via `?` is no longer permitted in
  closures for inferred types.][86831]
- [Previously unsound code is no longer permitted where different
  constructors in branches could require different lifetimes.][85574]
- As previously mentioned the [`std::arch` instrinsics now uses
  stricter const checking][83278] than before and may reject some
  previously accepted code.
- [`i128` multiplication on Cortex M0+ platforms currently
  unconditionally causes overflow when compiled with `codegen-units
  = 1`.][86063]

[85574]: rust-lang/rust#85574
[86831]: rust-lang/rust#86831
[86063]: rust-lang/rust#86063
[86831]: rust-lang/rust#86831
[79608]: rust-lang/rust#79608
[84988]: rust-lang/rust#84988
[84701]: rust-lang/rust#84701
[84072]: rust-lang/rust#84072
[85745]: rust-lang/rust#85745
[84744]: rust-lang/rust#84744
[85078]: rust-lang/rust#85078
[84717]: rust-lang/rust#84717
[83800]: rust-lang/rust#83800
[83366]: rust-lang/rust#83366
[83278]: rust-lang/rust#83278
[85292]: rust-lang/rust#85292
[cargo/9520]: rust-lang/cargo#9520
[cargo/9499]: rust-lang/cargo#9499
[cargo/9488]: rust-lang/cargo#9488
[cargo/9375]: rust-lang/cargo#9375
[`BTreeMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_keys
[`BTreeMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_values
[`HashMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_keys
[`HashMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_values
[`arch::wasm32`]: https://doc.rust-lang.org/core/arch/wasm32/index.html
[`VecDeque::binary_search`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search
[`VecDeque::binary_search_by`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by
[`VecDeque::binary_search_by_key`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by_key
[`VecDeque::partition_point`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.partition_point
@workingjubilee workingjubilee added the O-eBPF Target: I heard you liked code execution so I put some code execution in your code execution label Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-eBPF Target: I heard you liked code execution so I put some code execution in your code execution S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.