Verify that all crate sources are in sync #113695

bjorn3 · 2023-07-14T14:51:14Z

This ensures that rustc will not attempt to link against a cdylib as if it is a rust dylib when an rlib for the same crate is available. Previously rustc didn't actually check if any further formats of a crate which has been loaded are of the same version and if they are actually valid. This caused a cdylib to be interpreted as rust dylib as soon as the corresponding rlib was loaded. As cdylibs don't export any rust symbols, linking would fail if rustc decides to link against the cdylib rather than the rlib.

Two crates depended on the previous behavior by separately compiling a test crate as both rlib and dylib. These have been changed to capture their original spirit to the best of my ability while still working when rustc verifies that all crates are in sync. It is unlikely that build systems depend on the current behavior and in any case we are taking a lot of measures to ensure that any change to either the source or the compilation options (including crate type) results in rustc rejecting it as incompatible. We merely didn't do this check here for now obsolete perf reasons.

Note: This increases the size of rust dylibs as their crate metadata is no longer compressed. Cdylibs don't have crate metadata and as such are unaffected.

Fixes #10786
Fixes #82151
Fixes #82972
Closes bevy-cheatbook/bevy-cheatbook#114

rustbot · 2023-07-14T14:51:20Z

r? @TaKO8Ki

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

bjorn3 · 2023-07-14T14:52:30Z

compiler/rustc_metadata/src/locator.rs

-            } else if m.len() == 1 {
-                return Ok(Some(m.into_iter().next().unwrap()));


This is the meat of the change. By removing this the rest of the function will check compatibility of the candidates with the slot which contains the svh of the rlib that has already been loaded by the call of extract_one for the rlib.

compiler/rustc_metadata/src/locator.rs

bjorn3 · 2023-07-14T15:05:17Z

@bors try @rust-timer queue

This now loads crate metadata for libstd.so when linking, but not when compiling rlibs.

bors · 2023-07-14T15:05:29Z

⌛ Trying commit 6a6fa0d0619dbaa9f73e590131dd5bc50e4be9a8 with merge b0d1df2c2e875462902ed1ccb6318a28dd82d484...

pnkfelix · 2023-07-14T15:14:15Z

r=me assuming the perf run shows that this doesn't slow things down too much. (We should be willing to accept a performance hit here to get the benefits outlined, especially considering the kinds of problems people are hitting here in practice.)

petrochenkov · 2023-07-14T15:27:34Z

r? @petrochenkov

Mark-Simulacrum · 2023-07-14T15:54:43Z

Does this help with #10786? Should it?

bors · 2023-07-14T16:20:55Z

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

bjorn3 · 2023-07-14T16:52:47Z

Does this help with #10786? Should it?

Yes

rust-timer · 2023-07-15T02:32:05Z

Finished benchmarking commit (b0d1df2c2e875462902ed1ccb6318a28dd82d484): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 may lead 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-perf +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)	4.7%	[0.5%, 9.2%]	17
Regressions ❌ (secondary)	4.7%	[0.3%, 9.5%]	61
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.7%	[0.5%, 9.2%]	17

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)	7.8%	[2.7%, 18.0%]	24
Regressions ❌ (secondary)	10.3%	[3.4%, 20.7%]	59
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-4.3%, -3.1%]	2
All ❌✅ (primary)	7.8%	[2.7%, 18.0%]	24

Cycles

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)	7.9%	[1.3%, 13.3%]	14
Regressions ❌ (secondary)	7.7%	[2.3%, 13.2%]	50
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	7.9%	[1.3%, 13.3%]	14

Binary size

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

Bootstrap: 659.454s -> 658.736s (-0.11%)

rustbot · 2023-07-15T10:03:59Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

bjorn3 · 2023-07-15T10:05:57Z

Let's try with dylib metadata compression disabled to see how much of the perf loss is from dylib metadata decompression. This doubles the size of libstd.so from 6.4MB to 12MB which is not all that much compared to the total size of all standard library crates combined.

@bors try @rust-timer queue

bors · 2023-07-15T10:06:07Z

⌛ Trying commit 77b8a7fdb3a84054a2ddf60a5ff7c774ec5dd5d1 with merge 2da07d23d194da9a54d3161894b3db0bd64bcb43...

This ensures that rustc will not attempt to link against a cdylib as if it is a rust dylib when an rlib for the same crate is available. Previously rustc didn't actually check if any further formats of a crate which has been loaded are of the same version and if they are actually valid. This caused a cdylib to be interpreted as rust dylib as soon as the corresponding rlib was loaded. As cdylibs don't export any rust symbols, linking would fail if rustc decides to link against the cdylib rather than the rlib. Two crates depended on the previous behavior by separately compiling a test crate as both rlib and dylib. These have been changed to capture their original spirit to the best of my ability while still working when rustc verifies that all crates are in sync. It is unlikely that build systems depend on the current behavior and in any case we are taking a lot of measures to ensure that any change to either the source or the compilation options (including crate type) results in rustc rejecting it as incompatible. We merely didn't do this check here for now obsolete perf reasons.

…rces being in sync

bjorn3 · 2023-07-19T18:44:20Z

Fixed all review comments.

petrochenkov · 2023-07-19T19:55:33Z

@bors r=pnkfelix,petrochenkov

bors · 2023-07-19T19:55:35Z

📌 Commit 8c9a8b6 has been approved by pnkfelix,petrochenkov

It is now in the queue for this repository.

bors · 2023-07-19T20:06:41Z

⌛ Testing commit 8c9a8b6 with merge e094c70bbf1faa6bb972d7ecfd47860795d6c7a2...

bors · 2023-07-20T00:21:41Z

💥 Test timed out

petrochenkov · 2023-07-20T08:34:28Z

@bors retry

bors · 2023-07-20T09:00:13Z

⌛ Testing commit 8c9a8b6 with merge b14fd23...

bors · 2023-07-20T10:48:25Z

☀️ Test successful - checks-actions
Approved by: pnkfelix,petrochenkov
Pushing b14fd23 to master...

rust-timer · 2023-07-20T12:05:43Z

Finished benchmarking commit (b14fd23): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-1.2%	[-1.2%, -1.2%]	1
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	0.6%	[-1.2%, 2.5%]	2

Cycles

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.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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.5%	[0.3%, 0.7%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.3%, 0.7%]	8

Bootstrap: 649.877s -> 649.458s (-0.06%)

klensy · 2023-08-11T16:24:49Z

This should mention size increase somewhere in header: as i found size diff around 2x, bisected to here and found, that this was expected.

bjorn3 · 2023-08-13T12:16:30Z

Done @klensy

rustbot assigned TaKO8Ki Jul 14, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 14, 2023

bjorn3 commented Jul 14, 2023

View reviewed changes

compiler/rustc_metadata/src/locator.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 14, 2023

bjorn3 mentioned this pull request Jul 14, 2023

undefined reference to linker error when using dylibs #82151

Closed

This comment has been minimized.

Sign in to view

rustbot assigned petrochenkov and unassigned TaKO8Ki Jul 14, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 15, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 15, 2023

bjorn3 added 4 commits July 19, 2023 14:47

Don't compress dylib metadata

52853c2

Rewrite rmeta-rpass test to work with the new check for all crate sou…

aa98c5d

…rces being in sync

Fix review comments

8c9a8b6

bjorn3 force-pushed the fix_rlib_cdylib_metadata_handling branch from 00f8d96 to 8c9a8b6 Compare July 19, 2023 14:53

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 19, 2023

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 Jul 20, 2023

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 Jul 20, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 20, 2023

bors merged commit b14fd23 into rust-lang:master Jul 20, 2023
12 checks passed

rustbot added this to the 1.73.0 milestone Jul 20, 2023

bjorn3 mentioned this pull request Jul 20, 2023

Pitfall: some crates bork bevy/dynamic bevy-cheatbook/bevy-cheatbook#114

Open

bjorn3 deleted the fix_rlib_cdylib_metadata_handling branch July 20, 2023 16:41

bjorn3 mentioned this pull request Jul 25, 2023

Make C-API usable from Rust bytecodealliance/wasmtime#6765

Merged

bjorn3 mentioned this pull request Sep 23, 2023

Cannot be compiled into dylib on aarch64 with stable rust seanmonstar/reqwest#1252

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify that all crate sources are in sync #113695

Verify that all crate sources are in sync #113695

bjorn3 commented Jul 14, 2023 •

edited

rustbot commented Jul 14, 2023

bjorn3 Jul 14, 2023

bjorn3 commented Jul 14, 2023

This comment has been minimized.

bors commented Jul 14, 2023

This comment has been minimized.

pnkfelix commented Jul 14, 2023

petrochenkov commented Jul 14, 2023

Mark-Simulacrum commented Jul 14, 2023

bors commented Jul 14, 2023

This comment has been minimized.

bjorn3 commented Jul 14, 2023

rust-timer commented Jul 15, 2023

rustbot commented Jul 15, 2023

bjorn3 commented Jul 15, 2023

This comment has been minimized.

bors commented Jul 15, 2023

bjorn3 commented Jul 19, 2023

petrochenkov commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 20, 2023

petrochenkov commented Jul 20, 2023

bors commented Jul 20, 2023

bors commented Jul 20, 2023

rust-timer commented Jul 20, 2023

klensy commented Aug 11, 2023 •

edited

bjorn3 commented Aug 13, 2023

		} else if m.len() == 1 {
		return Ok(Some(m.into_iter().next().unwrap()));

Verify that all crate sources are in sync #113695

Verify that all crate sources are in sync #113695

Conversation

bjorn3 commented Jul 14, 2023 • edited

rustbot commented Jul 14, 2023

Choose a reason for hiding this comment

bjorn3 commented Jul 14, 2023

This comment has been minimized.

bors commented Jul 14, 2023

This comment has been minimized.

pnkfelix commented Jul 14, 2023

petrochenkov commented Jul 14, 2023

Mark-Simulacrum commented Jul 14, 2023

bors commented Jul 14, 2023

This comment has been minimized.

bjorn3 commented Jul 14, 2023

rust-timer commented Jul 15, 2023

Overall result: ❌ regressions - ACTION NEEDED

rustbot commented Jul 15, 2023

bjorn3 commented Jul 15, 2023

This comment has been minimized.

bors commented Jul 15, 2023

bjorn3 commented Jul 19, 2023

petrochenkov commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 20, 2023

petrochenkov commented Jul 20, 2023

bors commented Jul 20, 2023

bors commented Jul 20, 2023

rust-timer commented Jul 20, 2023

Overall result: no relevant changes - no action needed

klensy commented Aug 11, 2023 • edited

bjorn3 commented Aug 13, 2023

bjorn3 commented Jul 14, 2023 •

edited

klensy commented Aug 11, 2023 •

edited