replace dynamic library module with libloading #90716

euclio · 2021-11-08T23:57:52Z

This PR deletes the rustc_metadata::dynamic_lib module in favor of the popular and better tested libloading crate.

We don't benefit from libloading's symbol lifetimes since we end up leaking the loaded library in all cases, but the call-sites look much nicer by improving error handling and abstracting away some transmutes. We also can remove rustc_metadata's direct dependencies on libc and winapi.

This PR also adds an exception for libloading (and its license) to tidy, so this will need sign-off from the compiler team.

rust-highfive · 2021-11-08T23:57:55Z

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2021-11-09T16:11:18Z

src/test/run-make-fulldeps/extern-fn-reachable/main.rs

-
-extern crate rustc_metadata;
-
-use rustc_metadata::dynamic_lib::DynamicLibrary;


We shouldn't remove the test itself, but symbols need to be checked in some other way.

Fixed, took a page from run-make-fulldeps/symbol-visibility and used nm in the Makefile to just grep for the symbols.

petrochenkov · 2021-11-09T16:16:00Z

src/tools/tidy/src/deps.rs

@@ -40,6 +40,7 @@ const EXCEPTIONS: &[(&str, &str)] = &[
    ("snap", "BSD-3-Clause"),    // rustc
    // FIXME: this dependency violates the documentation comment above:
    ("fortanix-sgx-abi", "MPL-2.0"), // libstd but only for `sgx` target
+    ("libloading", "ISC"),           // MIT-like


The new license exception needs attention, but I'm not sure from whom exactly.
cc @Mark-Simulacrum

AFAIK MIT and ISC are considered functionally equivalent, but I am not a lawyer. https://en.wikipedia.org/wiki/ISC_license

I can sign off on using ISC as a library exception as a member of core. I did consult the foundations lawyer before.

We should also add ISC to the LICENSES list (i.e., it can always be used). Thanks for the ping!

cjgillot

I don't have a lot of knowledge of dynamic libraries. Left a few comments, nothing substantial.

compiler/rustc_plugin_impl/src/load.rs

cjgillot · 2021-11-21T17:23:05Z

compiler/rustc_interface/src/util.rs

-            Err(e) => {
+
+    let backend_sym =
+        unsafe { lib.get::<fn() -> Box<dyn CodegenBackend>>(b"__rustc_codegen_backend") }


I had a little trouble parsing this one. Can you create a type definition for it?

compiler/rustc_codegen_llvm/src/llvm_util.rs

cjgillot · 2021-11-25T17:53:32Z

@bors try @rust-timer queue

rust-timer · 2021-11-25T17:53:33Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-25T17:53:42Z

⌛ Trying commit 6ddfcfd156cc3ddcf3711d0f0a7e48b68ca69ab6 with merge ecb59a7e8817a96e09ca2e4b736c3dbd24003e2a...

bors · 2021-11-25T19:27:15Z

☀️ Try build successful - checks-actions
Build commit: ecb59a7e8817a96e09ca2e4b736c3dbd24003e2a (ecb59a7e8817a96e09ca2e4b736c3dbd24003e2a)

rust-timer · 2021-11-25T19:27:16Z

Queued ecb59a7e8817a96e09ca2e4b736c3dbd24003e2a with parent 862962b, future comparison URL.

rust-timer · 2021-11-25T21:36:23Z

Finished benchmarking commit (ecb59a7e8817a96e09ca2e4b736c3dbd24003e2a): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

Moderate improvement in instruction counts (up to -1.3% on incr-unchanged builds of wg-grammar)
Small regression in instruction counts (up to 0.4% on incr-unchanged builds of deeply-nested-closures)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

cjgillot · 2021-12-04T10:41:08Z

The perf results show a very small regression for check builds. The PR allows to rely on a better tested crate.
@nagisa, as crate author, do you have any remark?
Otherwise, r=me.

nagisa · 2021-12-04T19:36:12Z

I don't have any concerns and the changes look fine.

In the past, years ago, when I was thinking about a change like this, the ISC license was the blocker. Glad to hear that this question finally reached a resolution.

Though also see @Mark-Simulacrum review comment above. I think it makes sense to just specify ISC as allowed always, rather than adding an exception for libloading.

euclio · 2021-12-06T19:10:40Z

@cjgillot Thanks for the review!

I pushed a new commit that sets ISC as an allowed license following the other review comments.

cjgillot · 2021-12-09T17:07:00Z

@bors r+

bors · 2021-12-09T17:07:01Z

📌 Commit 923f939 has been approved by cjgillot

bors · 2021-12-12T17:28:55Z

⌛ Testing commit 923f939 with merge 6bda5b3...

bors · 2021-12-12T20:28:41Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 6bda5b3 to master...

rust-timer · 2021-12-12T21:59:47Z

Finished benchmarking commit (6bda5b3): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

Moderate improvement in instruction counts (up to -0.8% on incr-unchanged builds of externs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

rust-highfive assigned cjgillot Nov 8, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 8, 2021

This comment has been minimized.

Sign in to view

petrochenkov self-assigned this Nov 9, 2021

euclio force-pushed the libloading branch from 66ff33a to 3b0f35c Compare November 9, 2021 15:06

petrochenkov reviewed Nov 9, 2021

View reviewed changes

petrochenkov removed their assignment Nov 9, 2021

euclio force-pushed the libloading branch from 3b0f35c to 9aa9448 Compare November 9, 2021 17:48

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 11, 2021

cjgillot reviewed Nov 21, 2021

View reviewed changes

euclio force-pushed the libloading branch from 9aa9448 to 6ddfcfd Compare November 23, 2021 19:13

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 25, 2021

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 25, 2021

cjgillot added the perf-regression-triaged The performance regression has been triaged. label Dec 4, 2021

euclio force-pushed the libloading branch from 6ddfcfd to cbe4ec9 Compare December 6, 2021 16:59

replace dynamic library module with libloading

923f939

euclio force-pushed the libloading branch from cbe4ec9 to 923f939 Compare December 6, 2021 17:04

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 12, 2021

bors merged commit 6bda5b3 into rust-lang:master Dec 12, 2021

rustbot added this to the 1.59.0 milestone Dec 12, 2021

euclio deleted the libloading branch December 12, 2021 20:32

bors mentioned this pull request Dec 12, 2021

Allow loading LLVM plugins with both legacy and new pass manager #91125

Merged

rustbot removed the perf-regression Performance regression. label Dec 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace dynamic library module with libloading #90716

replace dynamic library module with libloading #90716

euclio commented Nov 8, 2021

rust-highfive commented Nov 8, 2021

This comment has been minimized.

petrochenkov Nov 9, 2021

euclio Nov 9, 2021

petrochenkov Nov 9, 2021

bjorn3 Nov 12, 2021

skade Nov 16, 2021

Mark-Simulacrum Nov 16, 2021

cjgillot left a comment

cjgillot Nov 21, 2021

euclio Nov 23, 2021

cjgillot commented Nov 25, 2021

rust-timer commented Nov 25, 2021

bors commented Nov 25, 2021

bors commented Nov 25, 2021

rust-timer commented Nov 25, 2021

rust-timer commented Nov 25, 2021

cjgillot commented Dec 4, 2021

nagisa commented Dec 4, 2021

euclio commented Dec 6, 2021

cjgillot commented Dec 9, 2021

bors commented Dec 9, 2021

bors commented Dec 12, 2021

bors commented Dec 12, 2021

rust-timer commented Dec 12, 2021


		extern crate rustc_metadata;

		use rustc_metadata::dynamic_lib::DynamicLibrary;

replace dynamic library module with libloading #90716

replace dynamic library module with libloading #90716

Conversation

euclio commented Nov 8, 2021

rust-highfive commented Nov 8, 2021

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Nov 25, 2021

rust-timer commented Nov 25, 2021

bors commented Nov 25, 2021

bors commented Nov 25, 2021

rust-timer commented Nov 25, 2021

rust-timer commented Nov 25, 2021

cjgillot commented Dec 4, 2021

nagisa commented Dec 4, 2021

euclio commented Dec 6, 2021

cjgillot commented Dec 9, 2021

bors commented Dec 9, 2021

bors commented Dec 12, 2021

bors commented Dec 12, 2021

rust-timer commented Dec 12, 2021