Support raw-dylib functions being used inside inlined functions #102988

dpaoliello · 2022-10-12T22:18:19Z

Issue Details:
When generating the import library for raw-dylib symbols, we currently only use the functions and variables declared within the current crate. This works fine if all crates are static libraries or rlibs as the generated import library will be contained in the static library or rlib itself, but if a dependency is a dynamic library AND the use of a raw-dylib function or variable is inlined or part of a generic instantiation then the current crate won't see its dependency's import library and so linking will fail.

Fix Details:
Instead, when we generate the import library for a dylib or bin crate, we will now generate it for the symbols both for the current crate and all upstream crates. We do this in two steps so that the import library for the current crate is passed into the linker first, thus it is preferred if there are any ambiguous symbols.

rustbot · 2022-10-12T22:18:23Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

rust-highfive · 2022-10-12T22:18:23Z

r? @davidtwco

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

compiler/rustc_codegen_ssa/src/back/link.rs

dpaoliello · 2022-10-17T17:35:00Z

r? @michaelwoerister

michaelwoerister · 2022-10-20T11:00:18Z

Thanks, @dpaoliello! I'm in the process of reviewing this.

petrochenkov · 2022-10-20T12:27:03Z

This issue exists for all kinds on native libraries, not only raw dylibs.

If we have a tree like this

- Final executable
  - Rust dylib
    - Native dylib
    - Native raw dylib
    - Native staticlib

then the "Rust dylib" can leak symbols from all the native libs through inline/const/generic functions.

For this reason

Native dylibs have to be linked into the final binary even if they were already linked into the intermediate Rust dylib (rustc already does this for long time).
Native raw dylibs have to be linked into the final binary even if they were already linked into the intermediate Rust dylib (this PR implement it).
Not sure what happens with native staticlibs, does Rust dylib just always export their symbols so they are available from the Rust dylib in case of inlining?

Also, what happens with Rust dylib dependencies of the second order (Final executable -> Rust dylib -> Rust dylib -> Native libs)? Are they also always linked into the final binary (together with their native dependencies)? UPD: Yes, they are.

This all feels suboptimal.
Perhaps we need an analysis that determines which exactly native symbols are leaked through inline/const/generic functions, and which exactly libraries need to be linked into the final binary due to them.
We are already doing something similar when calculating export symbols sets.

bjorn3 · 2022-10-20T13:05:39Z

Not sure what happens with native staticlibs, does Rust dylib just always export their symbols so they are available from the Rust dylib in case of inlining?

I believe it exports all functions which you declared in extern "C" { ... } where you actually used #[link(name = "...")] to indicate what library it originated from.

michaelwoerister · 2022-10-20T13:53:06Z

Perhaps we need an analysis that determines which exactly native symbols are leaked through inline/const/generic functions, and which exactly libraries need to be linked into the final binary due to them.

@petrochenkov, to make sure I understand this correctly: you propose to do pretty much the same as this PR, but generate import libraries that only contain the symbols that are actually needed?

petrochenkov · 2022-10-20T14:05:17Z

@michaelwoerister
Yes, and do the same thing for dylibs and staticlibs too.
Not in this PR, of course.

src/test/run-make/raw-dylib-inline-cross-dylib/Makefile

michaelwoerister · 2022-10-21T09:13:07Z

@michaelwoerister Yes, and do the same thing for dylibs and staticlibs too. Not in this PR, of course.

Yes, that sounds reasonable to me. Although there is a trade-off between the additional complexity this might introduce in rustc (not sure how complex it would actually be) and the gains we might expect from generating more targeted import libs. I'd guess, there's no way of telling without giving it a try.

But yes, not in this PR.

michaelwoerister · 2022-10-21T09:24:33Z

Thanks for the PR, @dpaoliello! The implementation looks good to me :)

It would be great to add another test for the transitive case @petrochenkov mentions, i.e. executable -> dylib -> dylib -> native raw dylib. That should be achievable by adding another Rust dylib with a public #[inline] function that in turn calls inline_library_function().

dpaoliello · 2022-10-21T18:46:58Z

Thanks for the PR, @dpaoliello! The implementation looks good to me :)

It would be great to add another test for the transitive case @petrochenkov mentions, i.e. executable -> dylib -> dylib -> native raw dylib. That should be achievable by adding another Rust dylib with a public #[inline] function that in turn calls inline_library_function().

Added this test case.

michaelwoerister · 2022-10-24T09:05:17Z

Thanks, @dpaoliello!
@bors r+

bors · 2022-10-24T09:05:18Z

📌 Commit db52b64b672c489823d760e87faa48a6e33d34cd has been approved by michaelwoerister

It is now in the queue for this repository.

bors · 2022-10-24T19:31:11Z

⌛ Testing commit db52b64b672c489823d760e87faa48a6e33d34cd with merge 5edf6a1ac36d232f1e8be86f64d64bf79e92c9f9...

bors · 2022-10-24T20:48:43Z

💔 Test failed - checks-actions

wesleywiser · 2022-10-24T21:20:08Z

Looks like the added test just needs to be updated to account for raw-dylib being unstable on x86.

@bors delegate+

bors · 2022-10-24T21:20:10Z

✌️ @dpaoliello can now approve this pull request

dpaoliello · 2022-10-24T23:18:35Z

@bors r+

bors · 2022-10-24T23:18:36Z

📌 Commit 3a1ef50 has been approved by dpaoliello

It is now in the queue for this repository.

bors · 2022-10-25T04:57:54Z

⌛ Testing commit 3a1ef50 with merge 31d754a...

bors · 2022-10-25T07:48:03Z

☀️ Test successful - checks-actions
Approved by: dpaoliello
Pushing 31d754a to master...

rust-timer · 2022-10-25T10:35:31Z

Finished benchmarking commit (31d754a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.9%	[2.3%, 5.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

pnkfelix · 2022-10-27T20:39:46Z

discussed in T-compiler meeting today

beta backport approved, with expectation that it will be part of Rust 1.65 that goes out in one week.

@rustbot label: beta-accepted

…mulacrum [beta] backport rollup * poll_fn and Unpin: fix pinning rust-lang#102737 * Support raw-dylib functions being used inside inlined functions rust-lang#102988 * Fix line numbers for MIR inlined code rust-lang#103071 * Add architectures to fn create_object_file rust-lang#103240 * Add eval hack in super_relate_consts back rust-lang#103279 * Mark std::os::wasi::io::AsFd etc. as stable. rust-lang#103308 * Truncate thread names on Linux and Apple targets rust-lang#103379 * Do not consider repeated lifetime params for elision. rust-lang#103450 * rustdoc: add missing URL redirect rust-lang#103588 * Remove commit_if_ok probe from NLL type relation rust-lang#103601 Also includes a copy of the release notes. r? `@ghost`

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 12, 2022

rust-highfive assigned davidtwco Oct 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2022

bjorn3 reviewed Oct 12, 2022

View reviewed changes

compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved

dpaoliello force-pushed the inlinerawdylib branch from ebf5338 to 9ed8a0f Compare October 13, 2022 22:06

dpaoliello requested a review from bjorn3 October 17, 2022 17:34

rust-highfive assigned michaelwoerister and unassigned davidtwco Oct 17, 2022

michaelwoerister reviewed Oct 20, 2022

View reviewed changes

src/test/run-make/raw-dylib-inline-cross-dylib/Makefile Show resolved Hide resolved

dpaoliello force-pushed the inlinerawdylib branch from 9ed8a0f to d21a168 Compare October 20, 2022 19:44

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 20, 2022

michaelwoerister reviewed Oct 21, 2022

View reviewed changes

src/test/run-make/raw-dylib-inline-cross-dylib/Makefile Show resolved Hide resolved

dpaoliello force-pushed the inlinerawdylib branch from d21a168 to f9acfc4 Compare October 21, 2022 18:46

This comment has been minimized.

Sign in to view

dpaoliello force-pushed the inlinerawdylib branch from f9acfc4 to db52b64 Compare October 21, 2022 21:50

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 Oct 24, 2022

wesleywiser added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 24, 2022

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

This comment has been minimized.

Sign in to view

Support raw-dylib functions being used inside inlined functions

3a1ef50

dpaoliello force-pushed the inlinerawdylib branch from db52b64 to 3a1ef50 Compare October 24, 2022 23:17

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 Oct 24, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 25, 2022

bors merged commit 31d754a into rust-lang:master Oct 25, 2022

rustbot added this to the 1.66.0 milestone Oct 25, 2022

ChrisDenton mentioned this pull request Oct 25, 2022

Use raw-dylib in the std for non-x86 Windows #102327

Closed

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 27, 2022

Mark-Simulacrum modified the milestones: 1.66.0, 1.65.0 Oct 29, 2022

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 29, 2022

Mark-Simulacrum mentioned this pull request Oct 29, 2022

[beta] backport rollup #103739

Merged

dpaoliello deleted the inlinerawdylib branch January 16, 2024 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support raw-dylib functions being used inside inlined functions #102988

Support raw-dylib functions being used inside inlined functions #102988

dpaoliello commented Oct 12, 2022

rustbot commented Oct 12, 2022

rust-highfive commented Oct 12, 2022

dpaoliello commented Oct 17, 2022

michaelwoerister commented Oct 20, 2022

petrochenkov commented Oct 20, 2022 •

edited

Loading

bjorn3 commented Oct 20, 2022

michaelwoerister commented Oct 20, 2022

petrochenkov commented Oct 20, 2022

michaelwoerister commented Oct 21, 2022

michaelwoerister commented Oct 21, 2022

dpaoliello commented Oct 21, 2022

This comment has been minimized.

michaelwoerister commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

This comment has been minimized.

wesleywiser commented Oct 24, 2022

bors commented Oct 24, 2022

dpaoliello commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 25, 2022

bors commented Oct 25, 2022

rust-timer commented Oct 25, 2022

pnkfelix commented Oct 27, 2022

Support raw-dylib functions being used inside inlined functions #102988

Support raw-dylib functions being used inside inlined functions #102988

Conversation

dpaoliello commented Oct 12, 2022

rustbot commented Oct 12, 2022

rust-highfive commented Oct 12, 2022

dpaoliello commented Oct 17, 2022

michaelwoerister commented Oct 20, 2022

petrochenkov commented Oct 20, 2022 • edited Loading

bjorn3 commented Oct 20, 2022

michaelwoerister commented Oct 20, 2022

petrochenkov commented Oct 20, 2022

michaelwoerister commented Oct 21, 2022

michaelwoerister commented Oct 21, 2022

dpaoliello commented Oct 21, 2022

This comment has been minimized.

michaelwoerister commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

This comment has been minimized.

wesleywiser commented Oct 24, 2022

bors commented Oct 24, 2022

dpaoliello commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 25, 2022

bors commented Oct 25, 2022

rust-timer commented Oct 25, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

pnkfelix commented Oct 27, 2022

petrochenkov commented Oct 20, 2022 •

edited

Loading