Allow loading of LLVM plugins [when dynamically built rust] by wsmoses · Pull Request #82734 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

wsmoses

@wsmoses

@rust-highfive

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nikic

So is this intended to be used via -Cllvm-args=-load=plugin?

@wsmoses

Precisely! It currently, however, may require a shared library LLVM build of rustc.

As an example, this (along with shared library build) enables https://github.com/wsmoses/Enzyme to provide automatic differentiation of rust code.

rustc test.rs -C llvm-args="-load=`pwd`/../Enzyme/enzyme/buildrs/Enzyme/LLVMEnzyme-11.so" -C passes="enzyme"

// test.rs
extern { fn __enzyme_autodiff(_: usize, ...) -> f64; }

fn square(x : f64) -> f64 {
   return x * x;
}

fn main() {
   unsafe {
      println!("Hello, world {} {}!", square(3.0), __enzyme_autodiff(square as usize, 3.0));
   }
}

@nikic

Why is it necessary for LLVM to be built shared for this to work? We build a shared libLLVM on dist-x86_64-linux to use ThinLTO, but I believe most other targets link it statically.

@wsmoses

The reasons (at least for my test cases) that's needed is because of symbol resolution.

The header in this patch needs to be included when arguments are parsed (thus loading the plugin). Arguments are currently parsed on rustc_llvm in a custom C++ routine. However, the rustc_llvm library does not contain most of the llvm symbols that a plugin may use (for example _ZN4llvm2cl18GenericOptionValue6anchorEv). The only place this symbol appears to be defined (verified via nm) is the rustc_codegen_llvm library. As a consequence, when the plugin is loaded in a static link LLVM build, it can fail to load the plugin since the required functions may not be available.

@nagisa

My worry is that this being exposed as a stable interface (like, sadly, many other flags) really limit us in what we can do in the future in terms of changing how rust relates to LLVM. In this particular example, we wouldn't be able to link to LLVM statically without breaking people utilizing LLVM plugin loading interface, or even upgrade LLVM, as it does not provide stable APIs between releases.

(In retrospective -Cllvm-arg(s) should always have been an unstable flag)

@Mark-Simulacrum

I'll also note that we only ship dynamically linked LLVM on x86_64-unknown-linux-gnu today I think, so plugin support feels pretty limited too in that sense.

@wsmoses

I'm not as worried about the LLVM ABI problems, perhaps because I believe that any plugin should have to be built against the current rustc's packaged LLVM. Certainly this would mean that plugins using LLVM have to be updated upon an LLVM major release upgrade, but it shouldn't prevent rustc from upgrading LLVM.

How often does rust upgrade its LLVM? I assume its during a major (or perhaps) minor release?

@wsmoses

Another way we could handle plugins is to do something like this: wsmoses@0149bc4

In it we could define an unstable flag listing potential plugins to be loaded. Obviously it would still have the dynamic LLVM requirement, but perhaps the unstable flag would be better?

@varkor

I'm not familiar with the nuances of this decision, so I'm going to reassign.

r? @nagisa

@nagisa

I think a change like this warrants a RFC or at least a MCP.

@cuviper

We have wanted something like this for Fedora and RHEL in order to support annobin. Our rustc is using the system libLLVM.so, and we can easily build annobin to match, but I do see why we should be cautious about how we could support plugins in rustup-distributed builds.

@wsmoses

@nagisa nagisa added S-blocked

Status: Blocked on something else such as an RFC or other implementation work.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Mar 17, 2021

@nagisa

Given that MCP is not progressing much at all, I think this may have a better chance of progressing if loading of LLVM plugins was explicitly unstable (i.e. only usable via a -Z flag, and only on nightly).

@nagisa

I think the PR referenced above resolves most of my concerns surrounding stability, and being an unstable flag does not require the MCP process, which seems to have gotten stuck. I think we should proceed with that one.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jun 26, 2021

@bors

Allow loading of llvm plugins on nightly

Based on a discussion in rust-lang#82734 / with @wsmoses. Mainly moves this behind a -Z flag, so it can only be used on nightly, as requested by @nagisa in rust-lang#82734 (comment)

This change allows loading of llvm plugins like Enzyme. Right now it also requires a shared library LLVM build of rustc for symbol resolution.

// test.rs
extern { fn __enzyme_autodiff(_: usize, ...) -> f64; }

fn square(x : f64) -> f64 {
   return x * x;
}

fn main() {
   unsafe {
      println!("Hello, world {} {}!", square(3.0), __enzyme_autodiff(square as usize, 3.0));
   }
}
./rustc test.rs -Z llvm-plugins="./LLVMEnzyme-12.so" -C passes="enzyme"
./test
Hello, world 9 6!

I will try to figure out how to simplify the usage and get this into stable in a later iteration, but having this on nightly will already help testing further steps.

Labels

S-blocked

Status: Blocked on something else such as an RFC or other implementation work.