rustc: Load the `rustc_trans` crate at runtime #47671

alexcrichton · Jan 23, 2018

Building on the work of #45684 this commit updates the compiler to
unconditionally load the rustc_trans crate at runtime instead of linking to it
at compile time. The end goal of this work is to implement #46819 where rustc
will have multiple backends available to it to load.

This commit starts off by removing the extern crate rustc_trans from the
driver. This involved moving some miscellaneous functionality into the
TransCrate trait and also required an implementation of how to locate and load
the trans backend. This ended up being a little tricky because the sysroot isn't
always the right location (for example --sysroot arguments) so some extra code
was added as well to probe a directory relative to the current dll (the
rustc_driver dll).

Rustbuild has been updated accordingly as well to have a separate compilation
invocation for the rustc_trans crate and assembly it accordingly into the
sysroot. Finally, the distribution logic for the rustc package was also
updated to slurp up the trans backends folder.

A number of assorted fallout changes were included here as well to ensure tests
pass and such, and they should all be commented inline.

rust-highfive · Jan 23, 2018

r? @petrochenkov

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

alexcrichton · Jan 23, 2018

r? @Mark-Simulacrum

cc @michaelwoerister, @eddyb, @bjorn3

alexcrichton · Jan 23, 2018

The next plans for #46819 are to add rustbuild configuration and support (as well as another submodule) for Emscripten's backend, integrate that for Emscripten, and then we should be good to upgrade to LLVM 5!

eddyb · Jan 23, 2018

eddyb reviewed Jan 23, 2018

View changes

eddyb · Jan 23, 2018

src/librustc_driver/lib.rs

+    let sysroot = sysroot_candidates.iter()
+        .map(|sysroot| {
+            let libdir = filesearch::relative_target_lib_path(&sysroot, &target);
+            sysroot.join(&libdir).join("trans-backends")


Just a nitpick, but could you use "codegen backend" instead of "trans backend" for everything external to the compiler itself? It can be changed later as part of #45274, so it doesn't really matter as long as we don't stabilize anything based on it.

eddyb · Jan 23, 2018

eddyb reviewed Jan 23, 2018

View changes

eddyb · Jan 23, 2018

src/librustc_trans/lib.rs

        box LlvmTransCrate(())
    }
 }

 impl TransCrate for LlvmTransCrate {
+    fn init(&self, sess: &Session) {
+        llvm_util::init(sess); // Make sure llvm is inited


I think this is fine for now but in the future we might want to have a Session created by the time we create this trait object (we'll need at least the target spec loaded).

Maybe let __rustc_codegen_backend take a Option<&Session> as argument and pass None when we are only using it for printing version info and stuff like that.

No, I think we can always create a Session, there shouldn't really be a significant cost to it since we're already doing the argument parsing and we have to load the target specs anyway.

Ah yeah so the two locations this was necessary (not taking a Session on construction) was when we print the version and when we print the LLVM passes. There's also one where we need to construct a list of all error codes but that's commented out for now...

I it'd be possible to have a Session here, but it'd be a "dummy session" in some cases because I don't think we want to generate an error for failure to parse command line arguments before we print the version. Either way I'd be fine though!

We don't fully parse arguments before processing the version? Is it a separate codepath?

EDIT: note that you can't load the backend without also parsing --target / -Z codegen-backend at least.

Well this PR doesn't use target specs yet, so it's not necessary for now. But I am curious, because I was hoping for a straight-forward implementation, is the "printing the version skips checking the rest of the CLI arguments" guaranteed/relied upon?

EDIT: I guess one could argue that -Z codegen-backend should be taken into account.

I think it basically would just require more refactoring. We'd have to, for example, initialize the Session in stages, first with a bunch of known options, then do some processing, then aftewards actually validate everything. AFAIK it's not really set up to do that yet.

Can't we validate everything early and buffer the errors or something?

I guess? Again though I didn't get the impression that the argument parsing was at all ready for this sort of refactoring, in the sense that it seemed outside the scope of this PR

Yeah, I don't want to block this PR, this is more for future reference.

Mark-Simulacrum · Jan 24, 2018

Mark-Simulacrum requested changes Jan 24, 2018

View changes

Some general nits. Overall approach looks okay to me, though I'm not super happy about having to add another step to compile.rs... seems okay though.

Mark-Simulacrum · Jan 24, 2018

src/librustc_driver/lib.rs

+    let lib = match DynamicLibrary::open_global_now(path) {
+        Ok(lib) => lib,
+        Err(err) => {
+            let err = format!("Couldnt load codegen backend {:?}: {:?}",


Couldn't here and elsewhere. Minor nit.

Mark-Simulacrum · Jan 24, 2018

src/librustc_driver/lib.rs

+    static mut LOAD: fn() -> Box<TransCrate> = || unreachable!();
+
+    INIT.call_once(|| {
+        let trans_name = sess.opts.debugging_opts.codegen_backend.clone();


Why do we need to clone this when we call as_ref on it in the next line? This seems.. odd.

Mark-Simulacrum · Jan 24, 2018

src/librustc_driver/lib.rs

+    });
+    let backend = unsafe { LOAD() };
+    backend.init(sess);
+    return backend


nit: backend

Mark-Simulacrum · Jan 24, 2018

src/librustc_driver/lib.rs

+    // but there's a few manual calls to this function in this file we protect
+    // against.
+    static LOADED: AtomicBool = ATOMIC_BOOL_INIT;
+    assert!(!LOADED.swap(true, Ordering::SeqCst),


Is swap the right thing here? I guess maybe... I'd sort of expect fetch_or perhaps?

Mark-Simulacrum · Jan 24, 2018

src/librustc_driver/lib.rs

+    assert!(!LOADED.swap(true, Ordering::SeqCst),
+            "cannot load the default trans backend twice");
+
+    if cfg!(test) {


I would like a comment explaining this, as it seems odd -- why would this be the case?

Certainly! I've added a comment

Mark-Simulacrum · Jan 24, 2018

src/librustc_driver/lib.rs

+            Some(s) => s,
+            None => continue,
+        };
+        if !filename.starts_with(DLL_PREFIX) || !filename.ends_with(DLL_SUFFIX) {


AFAICT, this condition is equivalent to !(starts_with && ends_with) which seems... odd? Why are we checking that it doesn't start with or end with the DLL_PREFIX? Aren't we looking for a DLL?

Specifically, given that we cut off the suffix and prefix below, shouldn't the condition be filename.starts_with(DLL_PREFIX) && filename.ends_with(DLL_SUFFIX)?

Hm yeah that's the condition I'm going for, but I think that's what's here as well? I'm looking for

if !(correct_prefix && correct_suffix) { continue }

which should be the same as

if !correct_prefix || !correct_suffix { continue }

Hm, yeah. Maybe clearer to write it that way (edit: so !(a && b)) though? Probably a minor point and not entirely relevant anyway. I was confused; but I don't think there's anyway to make this nonconfusing really.

Mark-Simulacrum · Jan 24, 2018

src/librustc_driver/lib.rs

+            let addr = current_dll_path as usize as *mut _;
+            let mut info = mem::zeroed();
+            if libc::dladdr(addr, &mut info) == 0 {
+                info!("dladdr failed: {}", io::Error::last_os_error());


Are you sure this is the right thing to call? I would sort of expect us to call dlerror here... which might be the same thing, I'm not actually sure.

Turns out, no!

Mark-Simulacrum · Jan 24, 2018

src/librustc_driver/lib.rs

+                info!("dladdr failed: {}", io::Error::last_os_error());
+                return None
+            }
+            if info.dli_fname.is_null() {


If no symbol matching addr could be found, then dli_sname and dli_saddr are set to NULL is what I see in my documentation, which seems to make me believe this is the wrong check?

I also see the following, which is also concerning. This might all be irrelevant and the existing code is "good enough" though.

Sometimes, the function pointers you pass to dladdr() may surprise you. On some architectures (notably i386 and x86_64), dli_fname and dli_fbase may end up pointing back at the object from which you called dladdr(), even if the function used as an argument should come from a dynamically linked library. The problem is that the function pointer will still be resolved at compile time, but merely point to the plt (Procedure Linkage Table) section of the original object (which dispatches the call after asking the dynamic linker to resolve the symbol). To work around this, you can try to com- pile the code to be position-independent: then, the compiler cannot prepare the pointer at compile time any more and gcc(1) will generate code that just loads the final symbol address from the got (Global Offset Table) at run time before passing it to dladdr().

I think that's ok for us? I haven't used this sort of dynamic loading much before so I suspect that we'll probably find a bug or two along the way, but I think this specific situation may fall under the category of "turns out ok given our usage" maybe?

So I guess what I'm asking is why do we check dli_fname and not dli_sname? Judging by the documentation, what we check today isn't going to determine whether the symbol was found or not, which I believe we do want to determine.

Agreed though that I'm not too worried about hashing this out now, if it works in your usage then seems good enough.

Mark-Simulacrum · Jan 24, 2018

src/librustc_driver/lib.rs

-    #[cfg(feature="llvm")]
-    all_errors.extend_from_slice(&rustc_trans::DIAGNOSTICS);
+    // FIXME: need to figure out a way to get these back in here
+    // all_errors.extend_from_slice(get_trans(sess).diagnostics());


This might be a function that should lie on the trait -- it seems like it should anyway, or maybe the trait could return a vec of errors that need to be added.

Heh it actually is! Unfortunately though there's not a great way to load it here, so I figured that for the one extended error in librustc_trans we could fudge it for now and figure it out later.

Could we just load all of the diagnostics, independent of backend, and then just some of them would be unused?

Unfortunately that's also gonna be difficult I think. I believe that with this PR as-is we could do that, but in the future I think we'll only want to dlopen one version of LLVM into the current process, and at this point in time we don't actually know the trans backend we'd otherwise be using. That means that with the Emscripten target we'd load the normal LLVM backend to load diagnostics and then load the Emscripten backend to actually do trans. I think that it would work ok? The "global" nature of the dlopen though required here though may throw a wrench into that..

In essence though I don't think there's a quick fix or an easy workaround for this, we'll want to deal with it later I think.

Oh also note that I added a dynamic assertion for this, the business with the LOADED constant, which asserts that we only load a backend once.

Sure, yeah, I understand that we don't want to dlopen multiple rustc_trans backends, but I was rather suggesting that we put all the diagnostic definition in driver and then the backend would use whichever ones it needed, though not all. This does increase the API surface of driver.

The diagnostics can probably be moved to the librustc_mir/monomorphize instance collector, so they're shared by all backends.

Ah yes indeed, such a strategy would for sure work!

Mark-Simulacrum · Jan 24, 2018

src/test/run-pass-fulldeps/create-dir-all-bare.rs

-    let td = TempDir::new("create-dir-all-bare").unwrap();
-    env::set_current_dir(td.path()).unwrap();
+    let path = PathBuf::from(env::var_os("RUST_TEST_TMPDIR").unwrap());
+    env::set_current_dir(&path).unwrap();


Why are these changes necessary? Can we no longer link to tempdir from tests? Or is it that tempdir wasn't being rebuilt because rustbuild is buggy (I would not be surprised).

The dependencies of librustc_trans are no longer available in the sysroot (as we just manually copy the one DLL), and one of its dependencies was tempdir. (no other crates in rustc use tempdir)

As I recall, the previous solution to this "problem" would be to move tempdir to a dependency of librustc. I'm fine with this too though.

It's true yeah we could do that but I'm also always a fan of tests with fewer dependencies :)

alexcrichton · Jan 24, 2018

Updated!

Mark-Simulacrum · Jan 24, 2018

Mark-Simulacrum approved these changes Jan 24, 2018

View changes

Changes seem good other than the comments I responded to.

alexcrichton · Jan 24, 2018

@bors: r=Mark-Simulacrum

bors · Jan 24, 2018

📌 Commit b14ccef has been approved by Mark-Simulacrum

pepyakin · Jan 25, 2018

pepyakin reviewed Jan 25, 2018

View changes

pepyakin · Jan 25, 2018

src/librustc_driver/lib.rs

+            }
+            Err(e) => {
+                let err = format!("couldn't load codegen backend as it \
+                                   doesn't export the `__rustc_backend_new` \


__rustc_codegen_backend symbol?

alexcrichton · Jan 25, 2018

@bors: r=Mark-Simulacrum

bors · Jan 25, 2018

📌 Commit 6509458 has been approved by Mark-Simulacrum

bors · Jan 27, 2018

💔 Test failed - status-travis

kennytm · Jan 27, 2018

dist-i686-linux linker failures when cross-compiling stage1-rustc_trans from 64-bit to 32-bit.

[01:38:24]    Compiling rustc_llvm v0.0.0 (file:///checkout/src/librustc_llvm)
[01:41:32] error: linking with `cc` failed: exit code: 1
[01:41:32]   |
<snip>
[01:41:32]   = note: /rustroot/lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../x86_64-unknown-linux-gnu/bin/ld: skipping incompatible /rustroot/lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../lib64/libgcc_s.so when searching for -lgcc_s
[01:41:32]           /rustroot/lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../x86_64-unknown-linux-gnu/bin/ld: i386:x86-64 architecture of input file `/tmp/rustc.CNu40ABKRiEL/librustc_llvm-9ea7cfc2f58bb7a4.rlib(compatibility.o)' is incompatible with i386 output
[01:41:32]           /rustroot/lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../x86_64-unknown-linux-gnu/bin/ld: i386:x86-64 architecture of input file `/tmp/rustc.CNu40ABKRiEL/librustc_llvm-9ea7cfc2f58bb7a4.rlib(compatibility-debug_list.o)' is incompatible with i386 output
...
<snip>
...
[01:41:32]           /rustroot/lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../x86_64-unknown-linux-gnu/bin/ld: i386:x86-64 architecture of input file `/tmp/rustc.CNu40ABKRiEL/librustc_llvm-9ea7cfc2f58bb7a4.rlib(thread.o)' is incompatible with i386 output
[01:41:32]           /rustroot/lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../x86_64-unknown-linux-gnu/bin/ld: i386:x86-64 architecture of input file `/tmp/rustc.CNu40ABKRiEL/librustc_llvm-9ea7cfc2f58bb7a4.rlib(regex.o)' is incompatible with i386 output
[01:41:32]           collect2: error: ld returned 1 exit status
[01:41:32]           
[01:41:32] 
[01:41:32] error: aborting due to previous error
[01:41:32] 
[01:41:33] error: Could not compile `rustc_trans`.

alexcrichton · Jan 27, 2018

@bors: r=Mark-Simulacrum

bors · Jan 27, 2018

📌 Commit c8ab8fc has been approved by Mark-Simulacrum

bors · Jan 27, 2018

⌛️ Testing commit c8ab8fc with merge 18342df...

bors · Jan 28, 2018

💔 Test failed - status-travis

alexcrichton · Jan 28, 2018

@bors: r=Mark-Simulacrum

bors · Jan 28, 2018

📌 Commit 884715c has been approved by Mark-Simulacrum

bors · Jan 28, 2018

⌛️ Testing commit 884715c with merge 87990a1...

bors · Jan 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing 87990a1 to master...

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from 593f3e1 to d41fbb9 Jan 23, 2018

rust-highfive assigned petrochenkov Jan 23, 2018

rust-highfive assigned Mark-Simulacrum and unassigned petrochenkov Jan 23, 2018

alexcrichton referenced this pull request Jan 23, 2018
Closed
Implement shipping a per-target LLVM backend #46819

kennytm added the S-waiting-on-review label Jan 23, 2018

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from d41fbb9 to 1ff8486 Jan 24, 2018

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from 1ff8486 to 13760b6 Jan 24, 2018

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch 3 times, most recently from 44486c9 to b14ccef Jan 24, 2018

alexcrichton referenced this pull request Jan 25, 2018
Merged
rustc: Split Emscripten to a separate codegen backend #47730

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from b14ccef to 6509458 Jan 25, 2018

kennytm added S-waiting-on-bors and removed S-waiting-on-review labels Jan 25, 2018

kennytm added S-waiting-on-author and removed S-waiting-on-bors labels Jan 27, 2018

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from 8347e55 to c8ab8fc Jan 27, 2018

kennytm added S-waiting-on-bors and removed S-waiting-on-author labels Jan 27, 2018

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from c8ab8fc to 884715c Jan 28, 2018

bors merged commit 884715c into rust-lang:master Jan 28, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

alexcrichton deleted the alexcrichton:trans-c-api-only branch Jan 28, 2018

cuviper referenced this pull request Mar 25, 2019
Closed
error_index_generator cannot find -lLLVM-3.9 #40717

rust-lang/rust

Join GitHub today

rustc: Load the `rustc_trans` crate at runtime #47671

Conversation

alexcrichton commented Jan 23, 2018 • edited

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from 593f3e1 to d41fbb9 Jan 23, 2018

rust-highfive assigned petrochenkov Jan 23, 2018

This comment has been minimized.

rust-highfive commented Jan 23, 2018

This comment has been minimized.

alexcrichton commented Jan 23, 2018

rust-highfive assigned Mark-Simulacrum and unassigned petrochenkov Jan 23, 2018

alexcrichton referenced this pull request Jan 23, 2018

This comment has been minimized.

alexcrichton commented Jan 23, 2018

kennytm added the S-waiting-on-review label Jan 23, 2018

eddyb reviewed Jan 23, 2018 View changes

This comment has been minimized.

eddyb reviewed Jan 23, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb Jan 23, 2018 • edited

This comment has been minimized.

eddyb Jan 23, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Mark-Simulacrum requested changes Jan 24, 2018 View changes

Mark-Simulacrum left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Mark-Simulacrum Jan 24, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from d41fbb9 to 1ff8486 Jan 24, 2018

This comment has been minimized.

alexcrichton commented Jan 24, 2018

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from 1ff8486 to 13760b6 Jan 24, 2018

Mark-Simulacrum approved these changes Jan 24, 2018 View changes

Mark-Simulacrum left a comment

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch 3 times, most recently from 44486c9 to b14ccef Jan 24, 2018

This comment has been minimized.

alexcrichton commented Jan 24, 2018

This comment has been minimized.

bors commented Jan 24, 2018

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 25, 2018

alexcrichton referenced this pull request Jan 25, 2018

pepyakin reviewed Jan 25, 2018 View changes

This comment has been minimized.

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from b14ccef to 6509458 Jan 25, 2018

This comment has been minimized.

alexcrichton commented Jan 25, 2018

This comment has been minimized.

bors commented Jan 25, 2018

alexcrichton commented Jan 23, 2018 •

edited

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from `593f3e1` to `d41fbb9` Jan 23, 2018

eddyb reviewed Jan 23, 2018

View changes

eddyb reviewed Jan 23, 2018

View changes

eddyb Jan 23, 2018 •

edited

eddyb Jan 23, 2018 •

edited

Mark-Simulacrum requested changes Jan 24, 2018

View changes

Mark-Simulacrum Jan 24, 2018 •

edited

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from `d41fbb9` to `1ff8486` Jan 24, 2018

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from `1ff8486` to `13760b6` Jan 24, 2018

Mark-Simulacrum approved these changes Jan 24, 2018

View changes

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch 3 times, most recently from `44486c9` to `b14ccef` Jan 24, 2018

pepyakin reviewed Jan 25, 2018

View changes

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from `b14ccef` to `6509458` Jan 25, 2018

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from `8347e55` to `c8ab8fc` Jan 27, 2018

alexcrichton force-pushed the alexcrichton:trans-c-api-only branch from `c8ab8fc` to `884715c` Jan 28, 2018

bors merged commit `884715c` into rust-lang:master Jan 28, 2018
2 checks passed