Add rlib digest to identify Rust object files#154861
Add rlib digest to identify Rust object files#154861mehdiakiki wants to merge 1 commit intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
mehdiakiki
force-pushed
the
fix/rlib-digest
branch
from
4b32d99 to
d966fa8
Compare
|
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. |
|
Got too much on my plate already. @rustbot reroll |
| create_wrapper_file(sess, rlib_digest::SECTION.to_string(), &digest_data); | ||
| let digest_file = emit_wrapper_file(sess, &wrapper, tmpdir.as_ref(), rlib_digest::FILENAME); | ||
| ab.add_file(&digest_file); | ||
| } |
There was a problem hiding this comment.
Can this be added right after the crate metadata instead? That saves time iterating over the rlib at staticlib build time.
There was a problem hiding this comment.
+1, although it's a bit of a complication to the code, perhaps we can do everything else and benchmark first.
| pub(crate) fn read(rlib_path: &Path) -> Option<RlibDigest> { | ||
| let file = File::open(rlib_path).ok()?; | ||
| let mmap = unsafe { Mmap::map(file).ok()? }; | ||
| let archive = ArchiveFile::parse(&*mmap).ok()?; |
There was a problem hiding this comment.
Ideally this mmap would be combined with the one in add_archive.
There was a problem hiding this comment.
Yes, we should open and parse the rlib once, and read the digest from already opened and parsed rlib.
|
|
||
| for m in &compiled_modules.modules { | ||
| if let Some(obj) = m.object.as_ref() { | ||
| if let Some(name) = obj.file_name().and_then(|n| n.to_str()) { |
There was a problem hiding this comment.
add_file uses unwraps, so they can be used here too.
| // Don't include Rust objects if LTO is enabled | ||
| if lto && looks_like_rust_object_file(fname) { | ||
| // Ignore the rlib digest file. | ||
| if fname == rlib_digest::FILENAME { |
There was a problem hiding this comment.
| if fname == rlib_digest::FILENAME { | |
| if fname == METADATA_FILENAME || fname == rlib_digest::FILENAME { |
|
|
||
| use super::metadata::search_for_section; | ||
|
|
||
| pub(crate) const FILENAME: &str = "lib.rlib-digest"; |
There was a problem hiding this comment.
Bikeshed: we need a good "official" name for this.
If the regular metadata is lib.rmeta, then perhaps this can be called lib.rmeta-link, or lib.rmeta-late, or something.
petrochenkov
added
S-waiting-on-author
|
Will address the review comments by tomorrow! |
mehdiakiki
force-pushed
the
fix/rlib-digest
branch
from
d966fa8 to
8276413
Compare
This comment has been minimized.
This comment has been minimized.
mehdiakiki
force-pushed
the
fix/rlib-digest
branch
from
8276413 to
18bc7ad
Compare
| // Don't include Rust objects if LTO is enabled | ||
| if lto && looks_like_rust_object_file(fname) { | ||
| // Don't include Rust objects if LTO is enabled. | ||
| if lto && digest.rust_object_files.iter().any(|f| f == fname) { |
There was a problem hiding this comment.
This also needs to be done in add_static_crate, right? And then the looks_like_rust_object_file function can be removed.
7 participants
This adds a metadata entry to
rlibarchives that lists which members are Rust object files instead of relying on the filename heuristic inlooks_like_rust_object.file. I also added a fallback to the old behavior forrlibsbuilt by older compilers.Part of #138243.