Skip to content

Move NativeLib::filename to the rmeta-link archive member#156735

Open
mehdiakiki wants to merge 1 commit into
rust-lang:mainfrom
mehdiakiki:fix/rlib-digest
Open

Move NativeLib::filename to the rmeta-link archive member#156735
mehdiakiki wants to merge 1 commit into
rust-lang:mainfrom
mehdiakiki:fix/rlib-digest

Conversation

@mehdiakiki
Copy link
Copy Markdown
Contributor

@mehdiakiki mehdiakiki commented May 19, 2026 •
edited by rustbot
Loading

View all comments

Second PR in #138243
Moves NativeLib::filename out of rmeta into lib.rmeta-link archive member that was introduced in the first PR. Filename is a link time only data so requiring a full metadata decode should be avoided. It is stored as (name, filename) pairs keyed by name, the new MetadataLoader::get_rlib_native_lib_filenames patches it back on decode. Also bumped METADATA_VERSION from version 10 to 11. Added also new round trip test and existing bundled-libs tests still pass.

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs 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 May 19, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 19, 2026

r? @dingxiangfei2009

rustbot has assigned @dingxiangfei2009.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 73 candidates
  • Random selection from 19 candidates

@rustbot

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 19, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mehdiakiki
Copy link
Copy Markdown
Contributor Author

mehdiakiki commented May 19, 2026

r? @petrochenkov
cc @bjorn3

petrochenkov
petrochenkov reviewed May 19, 2026
View reviewed changes
Comment thread tests/run-make/rlib-rmeta-link-native-lib-filenames/rmake.rs Outdated
Comment thread compiler/rustc_metadata/src/rmeta/mod.rs Outdated
Comment thread compiler/rustc_session/src/cstore.rs Outdated
Comment thread compiler/rustc_codegen_ssa/src/back/rmeta_link.rs Outdated
Comment thread compiler/rustc_codegen_ssa/src/back/link.rs Outdated
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2026
@mehdiakiki
Copy link
Copy Markdown
Contributor Author

mehdiakiki commented May 20, 2026

Started working on it today. Should have it ready soon.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mehdiakiki mehdiakiki force-pushed the fix/rlib-digest branch 2 times, most recently from c514597 to 45ee197 Compare May 23, 2026 21:33
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mehdiakiki
Copy link
Copy Markdown
Contributor Author

mehdiakiki commented May 23, 2026

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2026
@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented May 25, 2026

@bjorn3
Do you know if the trait MetadataLoader abstraction is still useful?
None of the in-tree backends uses it.

petrochenkov
petrochenkov reviewed May 25, 2026
View reviewed changes
Comment thread compiler/rustc_metadata/src/native_libs.rs Outdated
Comment thread compiler/rustc_session/src/cstore.rs Outdated
Comment thread compiler/rustc_codegen_ssa/src/back/metadata.rs
}
}

fn get_rlib_native_lib_filenames(&self, target: &Target, path: &Path) -> Vec<Option<String>> {
Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 25, 2026 •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

This trait currently has methods for getting the whole metadata blob, not some specific fields from it.
If we are working with MetadataLoader at all, then the added method should be something like fn get_rlib_link_metadata.

View changes since the review

Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 25, 2026

Choose a reason for hiding this comment

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

Note that rust_object_files are currently read from link-time metadata without involving MetadataLoader.
I think we should do the same thing for native_lib_filenames now.

Copy link
Copy Markdown
Member

@bjorn3 bjorn3 May 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

rust_object_files is read by the link code and individual codegen backends, while native_lib_filenames is read by common code too that an individual codegen backend can't override. I do agree with having get_rlib_link_metadata rather than get_rlib_native_lib_filenames. The MetadataLoader should treat everything as opaque blobs rather than deserialize it.

Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 25, 2026

Choose a reason for hiding this comment

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

while native_lib_filenames is read by common code too

Not actually, it's only read by code in link.rs, unlike other parts of NativeLib.

Comment thread compiler/rustc_metadata/src/creader.rs
self.metas[cnum].as_ref().unwrap_or_else(|| panic!("Failed to get crate data for {cnum:?}"))
}

pub(crate) fn metadata_loader(&self) -> &MetadataLoaderDyn {
Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 25, 2026 •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

This method can be inlined and removed.

View changes since the review

Comment thread compiler/rustc_codegen_ssa/src/back/rmeta_link.rs
Comment thread compiler/rustc_codegen_ssa/src/base.rs Outdated
native_libraries: Default::default(),
native_libraries_filenames: Default::default(),
used_libraries: tcx.native_libraries(LOCAL_CRATE).iter().map(Into::into).collect(),
used_libraries_filenames: local_native_lib_filenames(tcx),
Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

It is still too early to read the link-time metadata here, it only needs to be read in link.rs.

View changes since the review

@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented May 25, 2026

I didn't have time to look at some parts, but they'll likely need to be reworked anyway due to #156735 (comment).
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 25, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@bjorn3
Copy link
Copy Markdown
Member

bjorn3 commented May 25, 2026

Do you know if the trait MetadataLoader abstraction is still useful?
None of the in-tree backends uses it.

It would be a necessity for any external backend that wants to support an object file format or static library format that rustc doesn't.

@bjorn3
Copy link
Copy Markdown
Member

bjorn3 commented May 25, 2026

Don't remember if it was asked already, but would it be possible and make sense to move the entire NativeLib to the rmeta-link file rather than just the filename? That seems like it would be more robust to me. Moving just the filename would mean the list in the rmeta file and in the rmeta-link file can get out of sync, which would result in .zip() truncating the list and returning incorrect results.

@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented May 25, 2026

Don't remember if it was asked already, but would it be possible and make sense to move the entire NativeLib to the rmeta-link file rather than just the filename?

So the difference is that the filename is only used by link.rs code #156735 (comment), but other parts of NativeLib are used before that (*), by symbol exporting logic, for example.
So if pre-linking logic uses rmeta-link, then rmeta-link should supposedly be generated as early as regular rmeta for (cargo) pipelining, but we only want to generate it during linking.

(*) Although didn't check if this is true for all the other parts.

@bjorn3
Copy link
Copy Markdown
Member

bjorn3 commented May 25, 2026

I will be moving the symbol export calculation to the link phase as soon as I have moved LTO there (which would then also allow and require moving the list of exported symbols from the rmeta to rmeta-link). Only the actual link step and LTO need the list of exported symbols.

@mehdiakiki
Copy link
Copy Markdown
Contributor Author

mehdiakiki commented May 25, 2026

Ok read the full discussion. will proceed to totally separate link time vs compile time path then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@bjorn3 bjorn3 bjorn3 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@petrochenkov petrochenkov

Labels

A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mehdiakiki @rustbot @rust-log-analyzer @petrochenkov @bjorn3 @dingxiangfei2009