Skip to content

Move all LLVM externs into the rustc_llvm crate #142897

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Jun 23, 2025

#17795 expresses a desire to test the LLVM bindings with ctest. A pre-requisite for that is to have all the LLVM externs in once place, rather than spread across multiple crates.

Actually testing with ctest is blocked because the current version of ctest uses a long out-of-date parser which cannot cope with eg. extern blocks among other things. However there appears to be active work to build a new version of ctest (see https://github.com/rust-lang/libc/tree/main/ctest-next)

This PR does not intend to change any behaviour, only to move all FFI code into the rustc_llvm crate so that it may be tested at some future time.

@rustbot
Copy link
Collaborator

rustbot commented Jun 23, 2025

r? @cuviper

rustbot has assigned @cuviper.
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

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-meta Area: Issues & PRs about the rust-lang/rust repository itself F-autodiff `#![feature(autodiff)]` 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. labels Jun 23, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 23, 2025

triagebot.toml has been modified, there may have been changes to the review queue.

cc @davidtwco, @wesleywiser

Some changes occurred in compiler/rustc_codegen_llvm/src/builder/autodiff.rs

cc @ZuseZ4

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in compiler/rustc_codegen_llvm/src/llvm/enzyme_ffi.rs

cc @ZuseZ4

Some changes occurred in coverage instrumentation.

cc @Zalathar

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Jun 26, 2025

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

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

I've only skimmed this so far, but I think there are a few things we can do that will reduce some of the churn.

(The smaller we can make this, the more likely I'll be able to review it properly!)

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2025
@petrochenkov
Copy link
Contributor

Doing something like this was in my todo list for quite some time, but I expected to turn rustc_llvm into a crate with just bindings, a sort of typical llvm-sys.
So the dependencies on

rustc_codegen_ssa = { path = "../rustc_codegen_ssa" }
rustc_macros = { path = "../rustc_macros" }
rustc_middle = { path = "../rustc_middle" }
rustc_session = { path = "../rustc_session" }
rustc_target = { path = "../rustc_target" }

look unexpected.

@Diggsey
Copy link
Contributor Author

Diggsey commented Jul 5, 2025

@petrochenkov Types from those crates are used as part of inherent method implementations on FFI types. I could move them into extension methods or standalone functions in rustc_codegen_llvm but it would be more churn. Maybe something to address in a future PR?

@Diggsey Diggsey force-pushed the db-split-llvm-bindings branch from e865bbb to f092208 Compare July 5, 2025 00:58
@rust-log-analyzer

This comment has been minimized.

@Diggsey Diggsey force-pushed the db-split-llvm-bindings branch from f092208 to bb74561 Compare July 5, 2025 01:10
@rust-log-analyzer

This comment has been minimized.

@Diggsey
Copy link
Contributor Author

Diggsey commented Jul 5, 2025

I believe I've made the changes requested.

I could use some input on two things:

  1. What to do regarding @petrochenkov 's comment about dependencies of rustc_llvm.
  2. Any insight into this linker error I'm seeing. I can't seem to find any difference between how the two crates are built that would explain the problem?

edit:
Fixed the linker error.

@Diggsey Diggsey force-pushed the db-split-llvm-bindings branch from bb74561 to 39b9861 Compare July 5, 2025 12:44
@Diggsey Diggsey requested review from cuviper and bjorn3 July 5, 2025 14:06
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-meta Area: Issues & PRs about the rust-lang/rust repository itself F-autodiff `#![feature(autodiff)]` 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
Development

Successfully merging this pull request may close these issues.

7 participants