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

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
let needle_b = b"native_dep_b";
assert!(
rmeta_link.windows(needle_a.len()).any(|w| w == needle_a),
"lib.rmeta-link missing native_dep_a"
Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 19, 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.

I don't think we need a test making sure that filename lives in the .rmeta-link file specifically.
We rather need tests making sure that -Zpacked_bundled_libs works, and we have some of those. If this test doesn't add something new compared to them, then it can be removed.

View changes since the review

Comment thread compiler/rustc_metadata/src/rmeta/mod.rs Outdated
/// N.B., increment this if you change the format of metadata such that
/// the rustc version can't be found to compare with `rustc_version()`.
const METADATA_VERSION: u8 = 10;
const METADATA_VERSION: u8 = 11;
Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 19, 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 PR doesn't do anything requiring changing this.

View changes since the review

Comment thread compiler/rustc_session/src/cstore.rs Outdated
pub name: Symbol,
/// If packed_bundled_libs enabled, actual filename of library is stored.
#[stable_hash(ignore)]
pub filename: Option<Symbol>,
Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 19, 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.

The goal of the change is to remove this field entirely, and read the .rmeta-link file only during linking, not from fn get_native_libraries.

View changes since the review

Comment thread compiler/rustc_codegen_ssa/src/back/rmeta_link.rs
let mut decoder = MemDecoder::new(data, 0).ok()?;
let rust_object_files = Vec::<String>::decode(&mut decoder);
Some(RmetaLink { rust_object_files })
std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 19, 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.

What is the motivation for this change?
Currently the compiler can ICE if corrupted metadata is given to it, that's considered ok.

View changes since the review

Comment thread compiler/rustc_codegen_ssa/src/back/link.rs Outdated
let native_lib_filenames: Vec<(String, String)> = crate_info
.used_libraries
.iter()
.filter_map(|lib| Some((lib.name.to_string(), lib.filename?.to_string())))
Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov May 19, 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.

Library name is not a reliable key here, the library's index in the native_libraries vector would be more reliable.

View changes since the review

@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
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

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.

5 participants

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