-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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.
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
[AIX] change system dynamic library format #132362
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
This comment has been minimized.
This comment has been minimized.
Hi @mustartt, is this PR blocked on something? |
@rustbot author |
Keeping this as draft for now. Need to investigate if dylibs should be archived into |
45354fd
to
7af8f2e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@rustbot review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in terms of the AIX implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this LGTM overall, left a few nits/questions.
src/bootstrap/src/utils/helpers.rs
Outdated
// todo: reading the entire file as &[u8] into memory seems excessive | ||
// look into either mmap it or use the &CacheReader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: can you convert this to a FIXME and open a C-cleanup
issue then link to it FIXME(#nnnnnn): ...
?
@rustbot author |
3058de9
to
d999999
Compare
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
@@ -1965,9 +1965,9 @@ checksum = "baff4b617f7df3d896f97fe922b64817f6cd9a756bb81d40f8883f2f66dcb401" | |||
|
|||
[[package]] | |||
name = "libc" | |||
version = "0.2.161" | |||
version = "0.2.164" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libc 0.2.163
contains the new RTLD_MEMBER
This comment has been minimized.
This comment has been minimized.
Thanks for the comments. Rebase on newer master to avoid llvm assertion for f128. Bumped up libc to @rustbot review |
fmt fix cfg for windows remove unused imports address comments update libc to 0.2.164 fmt remove unused imports
25da642
to
0db9059
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. My only reservation is having to load the whole shared archive in bootstrap for dylib detection, but since that only happens for .a
I suppose it's not too bad, and has an associated cleanup issue #133268, so I think that's acceptable.
@bors r+ rollup=never (this PR contains a libc bump) |
It have a pretty small impact on the bootstrap time maybe an additional minute at most. But I'm wondering is the bootstrap process okay with including other dependencies such as |
Ideally bootstrap needs to build fast with minimal deps, so I'm inclined to say no, not for just this one purpose. |
…ouxu [AIX] change system dynamic library format Historically on AIX, almost all dynamic libraries are distributed in `.a` Big Archive Format which can consists of both static and shared objects in the same archive (e.g. `libc++abi.a(libc++abi.so.1)`). During the initial porting process, the dynamic libraries are kept as `.a` to simplify the migration, but semantically having an XCOFF object under the archive extension is wrong. For crate type `cdylib` we want to be able to distribute the libraries as archives as well. We are migrating to archives with the following format: ``` $ ar -t lib<name>.a lib<name>.so ``` where each archive contains a single member that is a shared XCOFF object that can be loaded.
💔 Test failed - checks-actions |
@bors retry (not even trying) |
☀️ Test successful - checks-actions |
Finished benchmarking commit (5d3c6ee): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 1.6%, secondary -3.1%)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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 797.142s -> 796.222s (-0.12%) |
Historically on AIX, almost all dynamic libraries are distributed in
.a
Big Archive Format which can consists of both static and shared objects in the same archive (e.g.libc++abi.a(libc++abi.so.1)
). During the initial porting process, the dynamic libraries are kept as.a
to simplify the migration, but semantically having an XCOFF object under the archive extension is wrong. For crate typecdylib
we want to be able to distribute the libraries as archives as well.We are migrating to archives with the following format:
where each archive contains a single member that is a shared XCOFF object that can be loaded.