Use the relative sysroot path in the linker command line to specify sysroot rlibs #112597

danakj · 2023-06-13T21:58:23Z

This addresses #112586

rustbot · 2023-06-13T21:58:31Z

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

danakj · 2023-06-13T22:04:16Z

FWIW I considered putting the FilePathMapping object into each FooLinker as a member, however then it makes borrowing harder as it holds a borrow on self in the return and you end up having to copy the returned Path every time. This is worse in the (normal) case of no remappings, where the FilePathMapping is an empty Vec and thus creating it as a temporary is ~free. So I went back to just calling it through the sess.opts.

bjorn3 · 2023-06-13T22:10:23Z

compiler/rustc_codegen_ssa/src/back/linker.rs

@@ -329,7 +329,8 @@ impl<'a> GccLinker<'a> {
 // the right `-Wl,-install_name` with an `@rpath` in it.
 if self.sess.opts.cg.rpath || self.sess.opts.unstable_opts.osx_rpath_install_name {
 let mut rpath = OsString::from("@rpath/");
- rpath.push(out_filename.file_name().unwrap());
+ let mapping = self.sess.opts.file_path_mapping();
+ rpath.push(mapping.map_prefix(out_filename).0.file_name().unwrap());


This is already just the filename and not the full path, so -Zremap-cwd-prefix doesn't have any effect and I'm not sure --remap-path-prefix is supposed to allow remapping individual files as opposed to directories. It probably doesn't break anythinf though.

bjorn3 · 2023-06-13T22:15:29Z

compiler/rustc_codegen_ssa/src/back/linker.rs

@@ -461,19 +466,19 @@ impl<'a> Linker for GccLinker<'a> {
 }
 fn link_rlib(&mut self, lib: &Path) {
 self.hint_static();
- self.cmd.arg(lib);
+ self.cmd.arg(self.sess.opts.file_path_mapping().map_prefix(lib).0.as_ref());


If the remap is anything other than turning an absolute path into a relative path that still resolves to the same absolute path given the working directory of rustc this will break linking as the linker actually needs to read the file at this path. So for example remapping /path/to/rust_source to /rustc/$hash as is done by rust's CI will be broken.

This is the reason for the CI failure.

michaelwoerister · 2023-06-14T08:19:04Z

I can take the review for this.
r? @michaelwoerister

petrochenkov · 2023-06-14T08:35:09Z

linker.rs doesn't look like the right place to do this.
It encapsulates details specific to different linkers, but path remapping is the same for all linkers and is not a linker's business in general.
It should probably be moved one layer above.

danakj · 2023-06-14T11:12:03Z

Ah ok so there's 2 issues here which I will address. Thanks for the feedback so far.

We need to differentiate between remap-path-prefix and remap-cwd-prefix, where the latter is allowed to be applied to paths used for IO and thus must be a valid path still. I have an idea of how to express that in the code.
I understand your point about the linker impls being the wrong place @petrochenkov, though the upside to being there is that we can be sure it happens, whereas finding and ensuring all callers do it is a bit harder. What if we replace &Path with a newtype MappedPath(&'a Path) for the methods that show up in the linker output (rlib and object paths) and this way help ensure callers do the mapping?

When the `--sysroot` is specified as relative to the current working directory, the sysroot's rlibs should also be specified as relative paths. Otherwise, the current working directory ends up in the absolute paths, and in the linker command line. And the entire linker command line appears in the PDB file generated by the MSVC linker. When adding an rlib to the linker command line, if the rlib's canonical path is in the sysroot's canonical path, then use the current sysroot path + filename instead of the full absolute path to the rlib. This means that when `--sysroot=foo` is specified, the linker command line will contain `foo/rustlib/target/lib/lib*.rlib` instead of the full absolute path to the same. This addresses rust-lang#112586

danakj · 2023-06-14T14:47:41Z

PTAL this is now done in link.rs and just rehomes rlib paths in the linker command line that are into the sysroot to use the actual sysroot path instead of the full absolute path.

michaelwoerister · 2023-06-15T13:12:23Z

We'll also need to take care of add_dynamic_crate, which is rather similar.

danakj · 2023-06-15T14:21:15Z

Oh, can the stdlib crates be dylibs?

bjorn3 · 2023-06-15T14:23:04Z

Yes, libstd.so and libtest.so exist. There are some (minor) limitations around their use though.

Like for rlibs, the paths on the linker command line need to be relative paths if the sysroot was specified by the user to be a relative path. Dylibs put the path in /LIBPATH instead of into the file path of the library itself, so we rehome the libpath and adjust the rehoming function to be able to support both use cases, rlibs and dylibs.

danakj · 2023-06-15T15:13:26Z

Done for add_dynamic_crate() now as well.

michaelwoerister · 2023-06-16T08:05:56Z

Thanks, @danakj! This looks good to me. The next beta branch is still a while off, so we should find out in time if it has any unintended side effects.

@bors r+ rollup=never

When we fix the issue with the object file in /tmp, I'd like us to look into improving the reproducibility tests and extend them to Windows.

bors · 2023-06-16T08:05:58Z

📌 Commit c340325 has been approved by michaelwoerister

It is now in the queue for this repository.

bors · 2023-06-16T09:02:54Z

⌛ Testing commit c340325 with merge 99b3346...

bors · 2023-06-16T11:51:36Z

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 99b3346 to master...

rust-timer · 2023-06-16T15:25:47Z

Finished benchmarking commit (99b3346): comparison URL.

Overall result: ❌ regressions - 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)	1.6%	[1.6%, 1.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
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)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	3.6%	[3.6%, 3.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

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)	3.3%	[2.8%, 4.7%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-5.7%, -0.9%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-5.7%, 4.7%]	14

Binary size

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

Bootstrap: 648.963s -> 646.331s (-0.41%)

rustbot assigned petrochenkov Jun 13, 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 Jun 13, 2023

danakj mentioned this pull request Jun 13, 2023

-Zremap-cwd-prefix is not applied to the sysroot rlib paths #112586

Closed

This comment has been minimized.

Sign in to view

bjorn3 reviewed Jun 13, 2023

View reviewed changes

rustbot assigned michaelwoerister and unassigned petrochenkov Jun 14, 2023

danakj force-pushed the map-linker-paths branch from e7321b9 to 52c9394 Compare June 14, 2023 12:14

This comment has been minimized.

Sign in to view

danakj force-pushed the map-linker-paths branch from 52c9394 to f5a9805 Compare June 14, 2023 12:21

This comment has been minimized.

Sign in to view

danakj force-pushed the map-linker-paths branch from f5a9805 to 32bf875 Compare June 14, 2023 14:07

danakj force-pushed the map-linker-paths branch from 32bf875 to 7e07271 Compare June 14, 2023 14:45

danakj changed the title ~~Remap linker output paths with --remap-path-prefix~~ Use the relative sysroot path in the linker command line to specify sysroot rlibs Jun 14, 2023

danakj force-pushed the map-linker-paths branch from 2768c92 to 7e07271 Compare June 14, 2023 14:56

Move the comment on fixing paths to where it belongs

4b83156

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 Jun 16, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 16, 2023

bors merged commit 99b3346 into rust-lang:master Jun 16, 2023

rustbot added this to the 1.72.0 milestone Jun 16, 2023

danakj deleted the map-linker-paths branch June 16, 2023 13:12

lqd mentioned this pull request Jun 25, 2024

ICE: Where clause ... was applicable to ... but now is not in compiler/rustc_trait_selection/src/traits/select/confirmation.rs #126944

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the relative sysroot path in the linker command line to specify sysroot rlibs #112597

Use the relative sysroot path in the linker command line to specify sysroot rlibs #112597

danakj commented Jun 13, 2023 •

edited

Loading

rustbot commented Jun 13, 2023

danakj commented Jun 13, 2023

This comment has been minimized.

bjorn3 Jun 13, 2023

bjorn3 Jun 13, 2023

michaelwoerister commented Jun 14, 2023

petrochenkov commented Jun 14, 2023

danakj commented Jun 14, 2023

This comment has been minimized.

This comment has been minimized.

danakj commented Jun 14, 2023

michaelwoerister commented Jun 15, 2023

danakj commented Jun 15, 2023 •

edited

Loading

bjorn3 commented Jun 15, 2023

danakj commented Jun 15, 2023

michaelwoerister commented Jun 16, 2023

bors commented Jun 16, 2023

bors commented Jun 16, 2023

bors commented Jun 16, 2023

rust-timer commented Jun 16, 2023

Use the relative sysroot path in the linker command line to specify sysroot rlibs #112597

Use the relative sysroot path in the linker command line to specify sysroot rlibs #112597

Conversation

danakj commented Jun 13, 2023 • edited Loading

rustbot commented Jun 13, 2023

danakj commented Jun 13, 2023

This comment has been minimized.

bjorn3 Jun 13, 2023

Choose a reason for hiding this comment

bjorn3 Jun 13, 2023

Choose a reason for hiding this comment

michaelwoerister commented Jun 14, 2023

petrochenkov commented Jun 14, 2023

danakj commented Jun 14, 2023

This comment has been minimized.

This comment has been minimized.

danakj commented Jun 14, 2023

michaelwoerister commented Jun 15, 2023

danakj commented Jun 15, 2023 • edited Loading

bjorn3 commented Jun 15, 2023

danakj commented Jun 15, 2023

michaelwoerister commented Jun 16, 2023

bors commented Jun 16, 2023

bors commented Jun 16, 2023

bors commented Jun 16, 2023

rust-timer commented Jun 16, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

danakj commented Jun 13, 2023 •

edited

Loading

danakj commented Jun 15, 2023 •

edited

Loading