Build librustc_llvm as a dylib, and install it as part of the private rustc crates.

[DiamondLovesYou]

I had to remove librustc_llvm's dependence on the libc crate as a part of
this. This seems reasonable as the global changes required were quite minimal.

Fix an FFI bug: Bool is signed.

[rust-highfive]

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

r? @alexcrichton

[alexcrichton]

We currently intentionally don't do this to cut down on the complexity of codegen backends and the number of files we're shipping, can you elaborate on why we might want to do this?

[bors]

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

[pietroalbini]

Ping from triage @DiamondLovesYou! It's been a while since we heard from you, will you have time to work on this again soon?

[DiamondLovesYou]

@alexcrichton The intent is to allow a runtime library to be able to use librustc_codegen_llvm and LLVM, somewhat independently, at the same time. Specifically, I need to read rustc metadata from elf files and insert the blobs into the cstore system at runtime, and then later use the same session to recodegen Things(TM). Due to the way rustc_driver::load_backend_from_dylib works, this makes using a separate LLVM at least difficult and possibly outright impossible as-is (AFAICT).

[alexcrichton]

Hm I'm not sure I really fully understand that use case? Changing our distribution strategy is a pretty major change for us, so if the other option is to refactor code here to make it more amenable to what you'd like to do I think we'd much prefer to go down a route like that.

[DiamondLovesYou]

Well, what's confusing? Maybe I'm not describing things as well as I could/should. The metadata is the normal Rust crate metadata, which a modified rustc leaves in compilation outputs. At runtime, we take a function def_id (retrieved at compiletime by another rustc modification) and retrans/recodegen that function def_id for another target, currently just AMDGPU. Yes, there are a lot of kinks to work out (like struct layout mismatch stuff), but I'm taking it one step at a time at this point.

This installs a new dylib into an existing folder which is already shipped, which already contains rustc private crates. This shouldn't be a big change to your distribution strategy. Additionally, LLVM is not small, I'd rather not have to duplicate it in my libraries.

[DiamondLovesYou]

@alexcrichton I looked into whether I could just have rustc_driver load the codegen crate without also making all of its symbols available globally. Unfortunately, rustc plugins are allowed to add their own passes to the set of LLVM passes and thus need to be able to resolve symbols in LLVM components when they are loaded. This works fine for plugins which are loaded after the codegen crate is loaded, but in my case the library needing to load these LLVM symbols is the one loading the codegen crate.

I'm open to suggestions.

[alexcrichton]

@DiamondLovesYou are you loading different codegen backends on the fly? Sorry I still don't understand what you're doing and what would necessitate a change such as this. For example why does todays' construction not work? What's the error message?

This is not a simple change because it's another quite large library for the dynamic loader to deal with at runtime. This means we need to actually ship this dynamic library (as you've found with the changes to the bootstrap system) and make sure it ends up in the right location and doesn't conflict with anything else. For example are we sure that the Emscripten LLVM doesn't conflict with the LLVM 6 LLVM? Additionally our runtime already took a pretty big hit just moving to a dlopen'd backend and I would be worried about taking more of a hit if more libraries are being processed.

I don't think a change like this is impossible but I would personally prefer to understand in more detail what it's going to be used for, why it should be in rustc itself and affect the official distribution, and also information like the performance impact of the change

[pietroalbini]

Ping from triage @DiamondLovesYou! The reviewer asked a question, will you have time to reply soon?

[DiamondLovesYou]

@pierzchalski Yes, will answer Soon(TM).

[pierzchalski]

Looks like "pie" is not a discriminating prefix, cc @pietroalbini

[DiamondLovesYou]

@pierzchalski Oh indeed! And I didn't check it like I should have. My bad.

[DiamondLovesYou]

@alexcrichton

@DiamondLovesYou are you loading different codegen backends on the fly? Sorry I still don't understand what you're doing and what would necessitate a change such as this. For example why does todays' construction not work? What's the error message?

I'm just using a single codegen backend (the main LLVM one), but I am loading it on the fly.

I have not tried using, eg, llvm-sys or llvm from crates.io, because I figured they wouldn't be able to coexist at runtime. AND I see ffi bindings to the LLVM api right here (this PR not withstanding), no need to get fickle with finding LLVM etc (not the first time I've just wanted to use these bindings)!

Anyway, consider this: when the LLVM codegen backend is loaded: existing, defined, symbols already present in the exe will override symbols loaded from the codegen backend object. To quote opengroup (emphasis mine):

Any object loaded by dlopen() that requires relocations against global symbols can reference the symbols in the original process image file, any objects loaded at program start-up, from the object itself as well as any other object included in the same dlopen() invocation, and any objects that were loaded in any dlopen() invocation and which specified the RTLD_GLOBAL flag.

That's a problem if the LLVM versions aren't the same. And we know they're not: Rust uses it's own fork!

In summary, I haven't encountered any error because I specifically sought to find a solution that avoids this can of worms.

Note this next quote is split:

This is not a simple change because it's another quite large library for the dynamic loader to deal with at runtime.
Additionally our runtime already took a pretty big hit just moving to a dlopen'd backend and I would be worried about taking more of a hit if more libraries are being processed.

The same is true of the codegen backend. First, the tax of searching is trivial: DT_NEEDED tells the linker exactly what needs to be found. Second, the symbols still have to be resolved: as I mentioned above, symbols already present in the image override new symbols loaded by dlopen(RTLD_GLOBAL | RTLD_NOW), thus all relocations must be looked up during load anyway.

A good question would be, "then why does chrome statically link, if not because it's faster?" Well because it is faster, but only when done en-mass. In chrome’s case, because they link everything together, the linker is allowed to assign relocation entries statically. In our case, the fact that the codegen backend module is dynamically linked means that optimization doesn't apply, even though LLVM is linked statically within the codegen backend. In other words, the codegen backend would need to be statically linked all the way up the tree for there to be any benefit.

As a sanity check, I’ve ran builds of both this PR and the commit it’s currently based on. I ran the builds for both 4 times. There was no significant difference between them. Dynamically linking averaged 33:45, while statically linking averaged 33:35. Or less than ½ of a percent.

This means we need to actually ship this dynamic library (as you've found with the changes to the bootstrap system) and make sure it ends up in the right location and doesn't conflict with anything else.

I can say from my own experience, due to a nasty hack I use to get cargo to recompile things after I’ve rebuilt part of the Rust toolchain (particularly librustc), that it does require adding the rustc private crates dir to LD_LIBRARY_PATH for build scripts. Specifically, this hack imports the relevant rustc_private crates into the build script for runtime (in rust-mir-hsa, this is the crate that uses the LLVM bindings and then loads the LLVM codegen backend), and then uses unix’ /proc/self/map_files to get the file paths of the rustc crates, which it feeds to cargo. Though the fact that I’m loading the rustc private crates at all in the build script makes this requirement not too surprising.

Regarding shipping: I’m not sure what, specifically, your concern is. The changes to that part of the build just ensure that the rustc_llvm shared object is placed next to the rest of the rustc private crates. Nothing more.

For example are we sure that the Emscripten LLVM doesn't conflict with the LLVM 6 LLVM?

I’ve run the tests with the Emscripten backend enabled, including generating dist packages. Emscripten's rustc_llvm gets a different hash. AFAICT, everything is in order. Though I'm sure the windows buildbots will have issues (because of course they will). I'll fix them once they're run.

[eddyb]

Note that this conflicts with the "merge librustc_llvm into librustc_codegen_llvm" item from #45274, which @irinagpopa has already started working on.
It's the only way I could come up with, to improve the safety and ergonomics of the LLVM bindings.

[bors]

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

[TimNN]

Ping from triage, @DiamondLovesYou, @alexcrichton, @eddyb: What is the status of this? Is this blocked by / made impossible by #45274?

[alexcrichton]

Sorry I was away on vacation and hadn't responded yet, but I'm back now!

@DiamondLovesYou I think I still don't fully understand why this change is necessary which is another source of my hesitation to land a PR like this. The OP has no information about why we would do a change like this. A later explanation sounds like you both want to not use the rustc LLVM and also use two LLVM instances at the same time? It then later sounds like you don't want to duplicate LLVM so you only want one LLVM, but the rustc LLVM?

Sorry I'm still lost in understanding what the purpose here is. I would personally love to continue to reduce the dynamic libraries associated with rustc as they're already way too much. If RTLD_GLOBAL is hindering what you're doing (I'm still not sure myself) then I think that's fixable at least. That flag was added purely to prevent regressing existing tests, but I think we should remove that flag entirely and the tests needed to fix it (those loading a custom LLVM pass). AFAIK it's pretty reasonable to require a custom LLVM build at that point instead of supporting it in our mainstream nightlies, it should just be an explicit step to do so rather than a side effect of a different PR

[DiamondLovesYou]

@alexcrichton No worries!

This can be closed, actually. The issue with involuntary loading multiple LLVM dynlib versions is still technically present (and I see that LLVM was just upgraded to the unreleased 7.0 version...), but it won't affect me now that I've discovered this wonderful crate. I'm disinclined to pursue.

[alexcrichton]

Ok!