Skip to content

linker: Update some outdated comments #99993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 24, 2022
Merged

linker: Update some outdated comments #99993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions compiler/rustc_codegen_ssa/src/back/link.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1947,7 +1947,6 @@ fn linker_with_args<'a>(
// Upstream rust libraries are not supposed to depend on our local native
// libraries as that would violate the structure of the DAG, in that
// scenario they are required to link to them as well in a shared fashion.
// (The current implementation still doesn't prevent it though, see the FIXME below.)
//
// Note that upstream rust libraries may contain native dependencies as
// well, but they also can't depend on what we just started to add to the
Expand All @@ -1968,15 +1967,16 @@ fn linker_with_args<'a>(
// and move this option back to the top.
cmd.add_as_needed();

// FIXME: Move this below to other native libraries
// (or alternatively link all native libraries after their respective crates).
// This change is somewhat breaking in practice due to local static libraries being linked
// as whole-archive (#85144), so removing whole-archive may be a pre-requisite.
// Local native libraries of all kinds.
//
// If `-Zlink-native-libraries=false` is set, then the assumption is that an
// external build system already has the native dependencies defined, and it
// will provide them to the linker itself.
if sess.opts.unstable_opts.link_native_libraries {
add_local_native_libraries(cmd, sess, codegen_results);
}

// Upstream rust libraries and their non-bundled static libraries
// Upstream rust libraries and their (possibly bundled) static native libraries.
add_upstream_rust_crates(
cmd,
sess,
Expand All @@ -1986,11 +1986,11 @@ fn linker_with_args<'a>(
tmpdir,
);

// Upstream dynamic native libraries linked with `#[link]` attributes at and `-l`
// command line options.
// If -Zlink-native-libraries=false is set, then the assumption is that an
// external build system already has the native dependencies defined, and it
// will provide them to the linker itself.
// Dynamic native libraries from upstream crates.
//
// FIXME: Merge this to `add_upstream_rust_crates` so that all native libraries are linked
// together with their respective upstream crates, and in their originally specified order.
// This may be slightly breaking due to our use of `--as-needed` and needs a crater run.
if sess.opts.unstable_opts.link_native_libraries {
add_upstream_native_libraries(cmd, sess, codegen_results);
}
Expand Down Expand Up @@ -2401,7 +2401,7 @@ fn add_upstream_rust_crates<'a>(
// or is an rlib already included via some other dylib crate, the symbols from
// native libs will have already been included in that dylib.
//
// If -Zlink-native-libraries=false is set, then the assumption is that an
// If `-Zlink-native-libraries=false` is set, then the assumption is that an
// external build system already has the native dependencies defined, and it
// will provide them to the linker itself.
if sess.opts.unstable_opts.link_native_libraries {
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_codegen_ssa/src/back/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,12 @@ pub enum MetadataPosition {
Last,
}

// For rlibs we "pack" rustc metadata into a dummy object file. When rustc
// creates a dylib crate type it will pass `--whole-archive` (or the
// platform equivalent) to include all object files from an rlib into the
// final dylib itself. This causes linkers to iterate and try to include all
// files located in an archive, so if metadata is stored in an archive then
// it needs to be of a form that the linker will be able to process.
// For rlibs we "pack" rustc metadata into a dummy object file.
//
// Historically it was needed because rustc linked rlibs as whole-archive in some cases.
// In that case linkers try to include all files located in an archive, so if metadata is stored
// in an archive then it needs to be of a form that the linker is able to process.
// Now it's not clear whether metadata still needs to be wrapped into an object file or not.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments in source code say that you cannot put a random non-object file into the first position in an archive.
However, I wasn't actually able to reproduce this - I tried an archive with a garbage first file on Linux and on windows-gnu targets, with both lld and bfd, and it worked successfully.
@bjorn3 Maybe you know where the relevant code in lld or binutils is? I did some search, but couldn't find it.

I'd like to make a YOLO PR that removes the object file wrapping from metadata, but still puts it on the first place in the archive, and run this PR through various targets available on CI.
Maybe in 1-2 weeks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly on macOS non-object files must be at the end. I believe I recall hitting this requirement early on in the development of cg_clif.

//
// Note, though, that we don't actually want this metadata to show up in any
// final output of the compiler. Instead this is purely for rustc's own
Expand Down