Skip to content
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

Make rlib metadata strip works with MIPSr6 architecture #90001

Merged
merged 4 commits into from
Jan 14, 2022

Conversation

Fearyncess
Copy link
Contributor

@Fearyncess Fearyncess commented Oct 18, 2021

Because MIPSr6 has many differences with previous MIPSr2 arch, the previous rlib metadata stripping code in rustc_codegen_ssa is only for MIPSr2/r3/r5 (which share the same elf e_flags).

This commit fixed this problem. It makes rustc_codegen_ssa happy when compiling rustc for MIPSr6 target or hosts.

e_flags REF: https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d931492/llvm/include/llvm/BinaryFormat/ELF.h#L562

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@cjgillot
Copy link
Contributor

Thanks for the PR @Fearyncess.
Unfortunately, I don't have much knowledge of this area of the compiler.
r? @alexcrichton

@alexcrichton
Copy link
Member

It looks like the only addition here is EF_MIPS_NAN2008, so could the code be refactored to not have duplicate flags on each branch and instead only add that flag as necessary? Otherwise seems fine to me, but if you're up for it it'd probably be good to update the object crate dependency from other crates to ideally only use one version instead of introducing another.

@Fearyncess
Copy link
Contributor Author

Fearyncess commented Oct 26, 2021

It looks like the only addition here is EF_MIPS_NAN2008, so could the code be refactored to not have duplicate flags on each branch and instead only add that flag as necessary? Otherwise seems fine to me, but if you're up for it it'd probably be good to update the object crate dependency from other crates to ideally only use one version instead of introducing another.

EF_MIPS_ARCH_{64,32}R6 detection is needed. If not, linker will treat the extracted rlib meta as MIPS r2 binaries (however the rlibs compiled using MIPS r6 target). They are completely different. It will be failed when linking then.

@alexcrichton
Copy link
Member

Er yes the other flags are needed as well, but it looks like these targets also need the EF_MIPS_NAN2008 over the existing flags that were already configured. If that's the case I think the patch here can be a bit smaller with less duplicating by only adding EF_MIPS_NAN2008 for these specific targets instead of re-listing all the existing flags twice.

@Fearyncess
Copy link
Contributor Author

Er yes the other flags are needed as well, but it looks like these targets also need the EF_MIPS_NAN2008 over the existing flags that were already configured. If that's the case I think the patch here can be a bit smaller with less duplicating by only adding EF_MIPS_NAN2008 for these specific targets instead of re-listing all the existing flags twice.

I've checked that MIPSr5 (which has MSA enforced) and Loongson 4000 series is used NaN2008 encoding by default, but does it will be a ABI break problem? https://web.archive.org/web/20180830093617/https://dmz-portal.mips.com/wiki/MIPS_ABI_-_NaN_Interlinking

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 28, 2021
@alexcrichton
Copy link
Member

Sorry I don't actually know much about mips to the point that I can comment on the technical bits of this patch, I'm just commenting on the code structure itself and how it can be refactored to have a little less duplication than it currently has. Otherwise I would trust you that the flags are working as intended and are necessary for the target that you're working with.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2021
@chenx97
Copy link
Contributor

chenx97 commented Nov 23, 2021

9859b81 is the initial support for version 6 of MIPS. @wzssyqa Could you help review this pull request?

It looks like the only addition here is EF_MIPS_NAN2008, so could the code be refactored to not have duplicate flags on each branch and instead only add that flag as necessary? Otherwise seems fine to me, but if you're up for it it'd probably be good to update the object crate dependency from other crates to ideally only use one version instead of introducing another.

@alexcrichton I would love to help factor out the duplication. Should I make e_flags be common flags | the if-else block of r6 detection? e.g.

let e_flags = elf::EF_MIPS_CPIC
    | elf::EF_MIPS_PIC
    | if sess.target.options.cpu.contains("r6") {
        // copied from the `e_flags` field of the object
        // generated by `mipsisa64r6el-linux-gnuabi64-gcc foo.c -c`
        elf::EF_MIPS_ARCH_64R6 | elf::EF_MIPS_NAN2008
    } else {
        elf::EF_MIPS_ARCH_64R2
    };

Also, regarding having NaN2008 on MIPSr5, I think this should be out of the scope of this pull request. This pull request is meant to only deal with mipsisa{32,64}r6el-related targets. What we need to make sure here is that this pull request won't change anything for those Tier-2 MIPS targets, targeting chips from MIPSr2 to MIPSr5.

@chenx97
Copy link
Contributor

chenx97 commented Nov 24, 2021

@alexcrichton Sorry for suddenly jumping into the conversation. I'm a colleague of the author of this pull request, so I asked the questions on behalf of our team. We are at CIP United and we will continue to improve Rust's support for the MIPSr6 family, and hopefully make it tier-2 in the future (which also means making real MIPSr6 hardware, and we look forward to it).😊

@alexcrichton
Copy link
Member

My review of this PR is basically purely stylistic, not really technical. The MIPS flags are all duplicated where the presence of the r6 feature implies EF_MIPS_NAN2008 is or-ed into the list of flags. I would stylistically request less duplication, along the lines of what #90001 (comment) mentions.

@bors
Copy link
Contributor

bors commented Dec 8, 2021

☔ The latest upstream changes (presumably #91604) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 6, 2022

☔ The latest upstream changes (presumably #89819) made this pull request unmergeable. Please resolve the merge conflicts.

Because MIPSr6 has many differences with previous MIPSr2 arch, the previous rlib metadata stripping code in `rustc_codegen_ssa` is only for MIPSr2/r3/r5 (which share the same elf e_flags).
This commit fixed this problem. It makes `rustc_codegen_ssa` happy when compiling rustc for MIPSr6 target or hosts.
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Fearyncess
Copy link
Contributor Author

Fearyncess commented Jan 8, 2022

My review of this PR is basically purely stylistic, not really technical. The MIPS flags are all duplicated where the presence of the r6 feature implies EF_MIPS_NAN2008 is or-ed into the list of flags. I would stylistically request less duplication, along the lines of what #90001 (comment) mentions.

@alexcrichton

i have make the code more simpilified and update the dep of rustc_codegen_ssa for it. it seems works well

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 10, 2022

📌 Commit cf36c21 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 12, 2022
Make rlib metadata strip works with MIPSr6 architecture

Because MIPSr6 has many differences with previous MIPSr2 arch, the previous rlib metadata stripping code in `rustc_codegen_ssa` is only for MIPSr2/r3/r5 (which share the same elf e_flags).

This commit fixed this problem. It makes `rustc_codegen_ssa` happy when compiling rustc for MIPSr6 target or hosts.

e_flags REF: https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d931492/llvm/include/llvm/BinaryFormat/ELF.h#L562
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 13, 2022
Make rlib metadata strip works with MIPSr6 architecture

Because MIPSr6 has many differences with previous MIPSr2 arch, the previous rlib metadata stripping code in `rustc_codegen_ssa` is only for MIPSr2/r3/r5 (which share the same elf e_flags).

This commit fixed this problem. It makes `rustc_codegen_ssa` happy when compiling rustc for MIPSr6 target or hosts.

e_flags REF: https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d931492/llvm/include/llvm/BinaryFormat/ELF.h#L562
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 14, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#90001 (Make rlib metadata strip works with MIPSr6 architecture)
 - rust-lang#91687 (rustdoc: do not emit tuple variant fields if none are documented)
 - rust-lang#91938 (Add `std::error::Report` type)
 - rust-lang#92006 (Welcome opaque types into the fold)
 - rust-lang#92142 ([code coverage] Fix missing dead code in modules that are never called)
 - rust-lang#92277 (rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1))
 - rust-lang#92334 (rustdoc: Preserve rendering of macro_rules matchers when possible)
 - rust-lang#92807 (Update cargo)
 - rust-lang#92832 (Update RELEASES for 1.58.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c7ada00 into rust-lang:master Jan 14, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants