Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Support LLVM14 #316

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Support LLVM14 #316

wants to merge 4 commits into from

Conversation

nbaksalyar
Copy link
Contributor

This is WIP until llvm-sys is updated to version 140.

Fixes #312.

README.md Outdated Show resolved Hide resolved
@nbaksalyar nbaksalyar force-pushed the llvm14 branch 2 times, most recently from 4e25fc5 to 28fd922 Compare April 17, 2022 03:36
@nbaksalyar nbaksalyar marked this pull request as ready for review April 17, 2022 03:36
@nbaksalyar
Copy link
Contributor Author

llvm-sys v140 has been published now, so this PR is ready to be reviewed.

README.md Outdated
It is needed to compile BPF bytecode.
- **LLVM 14 or 13**
It is needed to compile BPF bytecode. If you're using Rust 1.60 or later, LLVM 14 is required.
If you need to use LLVM 13, use the feature `llvm13-0`.
Copy link
Collaborator

@rsdy rsdy Apr 17, 2022

Choose a reason for hiding this comment

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

You seem to have reverted this again to llvm13-0?

Copy link
Contributor Author

@nbaksalyar nbaksalyar Apr 17, 2022

Choose a reason for hiding this comment

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

Ah, that's awkward. Made this change on another machine and forgot to synchronise. Sorry about that!

CI configuration needs to be updated too as it seems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI configuration needs to be updated too as it seems.

@rsdy I think there are several ways make it work - which one would you recommend to go for?

  1. Update the Docker images (needs to be done at some point anyway but now it's a bit more complicated - e.g., llvm14 isn't packaged for Fedora 35 so you have to use F36 beta).
  2. Change build commands in the CI workflows to cargo build --features=llvm13.
  3. Make llvm13 the default version/feature for now (I'm favouring this one).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nbaksalyar Sorry for the slow response on this, I've been away from the Internet. Currently bumping images for 1.

README.md Outdated
@@ -235,7 +242,8 @@ There are two LLVM versions involved in compiling BPF programs:

| Rust version | LLVM version of the rustc | Valid LLVM version of system |
|:-------------|:-------------------------:|:-----------------------------|
| 1.56 ~ | LLVM 13 | LLVM 13 and newer |
| 1.56 ~ 1.60 | LLVM 13 | LLVM 13 and newer |
Copy link

@SikoJobs SikoJobs Apr 25, 2022

Choose a reason for hiding this comment

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

Shouldn't it be "1.56 ~ 1.59"?
I've reproduced #315 using Rust 1.60 + system LLVM 13.0.1, and Rust 1.59 + system LLVM 13.0.1 works well.

Signed-off-by: Nikita Baksalyar <[email protected]>
* Use latest Docker images for builds.
* Re-enable Alpine build because it now supports llvm13.

Signed-off-by: Nikita Baksalyar <[email protected]>
rq_disk was removed from the request struct.
Use request.q.disk instead.

Signed-off-by: Nikita Baksalyar <[email protected]>
@DerekStrickland
Copy link

Hi @rsdy. I've made some progress this weekend on this issue, but there's more to do. Do you want incremental PRs along the way to get a some improvements earlier rather than later, or do you want one big PR that handles it all? Also, wanted to inform you in advance it will require a PR to build-images to get merged first and for those images to be updated. Looking at previous comments I suspect you expected that though 😄

@rsdy
Copy link
Collaborator

rsdy commented Sep 26, 2022

@DerekStrickland Thanks for picking this up! I'm happy to merge PRs as they come in assuming that they keep the repo in a consistent state, e.g. if it breaks main "temporarily" I'd much rather merge those PRs together before landing them in main.

I think the best approach might be for you to re-open a different PR with this branch, targeting main, so I can close this and move work over to your fork. Does that make sense?

@DerekStrickland
Copy link

Yes it does. I've got a little bit of clean up to do and the work week is back upon us. I'll get some incremental updates over to you as soon as I can starting with the build-images changes that will be required.

@AlexGatz
Copy link

Excited about this pr! Thanks for all of the work on it.

@DerekStrickland
Copy link

Thanks @AlexGatz. The build-images PR got merged yesterday but failed in the foniod CI while passing in my fork. I'm investigating what is up with that before proceeding to updating RedBPF itself. I'll keep you all posted as I progress.

@DerekStrickland
Copy link

@rsdy @AlexGatz New PR staged for build images. tl;dr bindgen published a release that separates the bindgen-cli from the bindgen library via workspace 3 days ago that required a change to the cargo install command.

@AlexGatz
Copy link

@rsdy @AlexGatz New PR staged for build images. tl;dr bindgen published a release that separates the bindgen-cli from the bindgen library via workspace 3 days ago that required a change to the cargo install command.

Fantastic. Thank you.

@yasin-cs-ko-ak
Copy link

LLVM 15 is released!

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

Successfully merging this pull request may close these issues.

Add support for LLVM 14 (Rust 1.60)
6 participants