Use more accurate ELF flags on MIPS #140634

smrobtzz · 2025-05-04T04:33:33Z

Changes the MIPS ELF flags used for metadata objects to be closer to what LLVM uses so the linker doesn't complain

rustbot · 2025-05-04T04:33:38Z

rustbot has assigned @workingjubilee.
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

rustbot · 2025-05-04T04:33:39Z

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

workingjubilee · 2025-05-04T05:25:57Z

Changes the MIPS ELF flags used for metadata objects to be closer to what LLVM uses so the linker doesn't complain

@smrobtzz what linker were you using from what distro to build what crate such that the linker complained?

workingjubilee · 2025-05-04T07:17:56Z

compiler/rustc_codegen_ssa/src/back/metadata.rs

+            if !sess.target.options.features.contains("noabicalls") {
+                e_flags |= elf::EF_MIPS_CPIC;
+            }


If this is defining whether it uses position independent calls for the entire emitted artifact, then no, I do not intend to have rustc respect another LLVM "feature" that defines a necessarily global quality of compilation, much less a negative feature. Rust has a model of what a target feature is that conflicts with that.

At the very least, respecting new CLI inputs like this is certainly something I would prefer we decide separately from what is a mere fix.

Makes sense, I'll remove that check.

workingjubilee · 2025-05-04T07:27:58Z

compiler/rustc_codegen_ssa/src/back/metadata.rs

+                _ => {
+                    if architecture == Architecture::Mips {
+                        e_flags |= elf::EF_MIPS_ABI_O32
+                    }
+                }


I do not like this continued guessing. Either

do nothing on this branch

explicitly recognize "n64" as a possible variant, add it to all the 64-bit MIPS targets, and panic if llvm_abiname is unset

I don't see how that change is worse than the current implementation since it's exactly the same logic, but I'll add better validation here and require llvm_abiname on 64-bit MIPS

I am at about the point where I think we should require the field on all MIPSy targets, just like it is required on RISCV (last I checked) and a similar-but-different field is required on x86. Especially for ABI-affecting fields, we generally trend towards making the fields hard requirements so that there's no ambiguity.

It's kinda the same thing as the lowercase issue: if there is a sensible default for or normalization of the data, then we should probably address that during deserialization and emit a warning if the field was not specified. Otherwise, we should usually panic on the field being unspecified.

I am okay with the 32-bit case, I think, being inferred, due to LLVM not supporting other alternative MIPS ABIs for 32-bit MIPS (though supposedly they exist)

workingjubilee

I notice a number of changes that seem to be acting on a matter of "thoroughness" or otherwise changing things that don't seem to need to be changed in order for the compiler to be used to its current range of ability for the existing targets we support. With the somewhat terse PR description that did not describe the actual errors you encountered, I can't tell if all of these changes are actually required, and some of them seem to allow new forms of being incorrect.

While we are allowed to arbitrarily trash compilation if the target-spec.json (or our own target definitions!) are incorrect, so we can corrupt our artifacts (or preferably fail to compile) if the llvm_abiname is set in an incorrect or clashing way? I would prefer it if the change was slightly more guarded against other forms of incorrectness like the target.options.cpu or target.options.features, which can be set by ordinary compiler users giving it input, last I checked.

workingjubilee · 2025-05-04T07:37:01Z

compiler/rustc_target/src/spec/mod.rs

-            "mips64" | "mips64r6" => (Architecture::Mips64, None),
+            "mips64" | "mips64r6" => (
+                if self.options.llvm_abiname.to_lowercase().as_str() == "n32" {
+                    Architecture::Mips64_N32
+                } else {
+                    Architecture::Mips64
+                },
+                None,
+            ),


We don't support any N32 targets so this change is not required, really.

But more puzzlingly, I'm not sure why we are spending the effort to be case-indifferent when we're not even case-indifferent for "mips64" vs. "MIPS64"? If we want to normalize the case then we should have done it elsewhere instead of complicating every time we check.

I'm using a custom target spec with n32, so I need this change most. Will remove the to_lowercase though

I see. That makes sense.

It would be slightly more productive if we had an actual N32 target as a skeleton for understanding what we are supposed to be supporting, even if it was extremely rudimentary.

Can you please leave a comment to that effect to save some headscratching down the line? i.e.

a custom target spec with n32

workingjubilee · 2025-05-04T07:41:10Z

compiler/rustc_codegen_ssa/src/back/metadata.rs

+        Architecture::Mips | Architecture::Mips64 | Architecture::Mips64_N32 => {
+            let mut e_flags = match sess.target.options.cpu.as_ref() {
                "mips1" => elf::EF_MIPS_ARCH_1,
                "mips2" => elf::EF_MIPS_ARCH_2,
                "mips3" => elf::EF_MIPS_ARCH_3,
                "mips4" => elf::EF_MIPS_ARCH_4,
                "mips5" => elf::EF_MIPS_ARCH_5,
-                s if s.contains("r6") => elf::EF_MIPS_ARCH_32R6,
+                "mips32r2" => elf::EF_MIPS_ARCH_32R2,
+                "mips32r6" => elf::EF_MIPS_ARCH_32R6,
+                "mips64r2" => elf::EF_MIPS_ARCH_64R2,
+                "mips64r6" => elf::EF_MIPS_ARCH_64R6,
                _ => elf::EF_MIPS_ARCH_32R2,
            };


Mmh. If we set EF_MIPS_ARCH_32R2 for Architecture::Mips64 then I'm pretty sure that all further compilation logic is, uh, I believe the technical term is "totally fucked"? Please do not allow invalid combinations, so split things along the 32-bit and 64-bit lines and use panic!() or bug!() appropriately. If this means you need to extract code shared across the different MIPS bitnesses and ABIs into another function or just double-down on an initial guard for some cases then that's fine.

If you can find a DiagCtxt a few steps up the callstack and want to thread it down (or pass more data up from here?) to make a nicer error, that would be fine too. I just want to make sure we don't support obviously incorrect pairs.

OK, will fix

smrobtzz · 2025-05-04T19:28:19Z

Sorry, should've explained more. I'm using a custom target specification with this combination of options:

"arch": "mips64",
"cpu": "mips3",
"llvm-abiname": "n32",
"features": "+noabicalls",

With these options, Rust's metadata objects (not sure what they're called) are always ELF64 mips64r2 objects, whereas the code objects emitted by LLVM are ELF32 mips3 objects. So, linking an executable (or, making an rlib and trying to link that with something else) fails because the objects are obviously incompatible.

My changes are attempting to be "thorough" because the criteria for what the GNU and LLVM linkers consider compatible objects is pretty unclear. They can complain about nan2008 not matching, the ABI not matching, the CPU not matching, etc. So, I figured that it would be at better (resulting in the least amount of linker complaints for whatever custom or builtin targets people may be using) to at least put some effort into making sure the ELF files emitted are close to what LLVM outputs, rather than (for mips64 at least) defaulting to what mips64el-linux-gnuabi64-gcc foo.c -c did, and assuming that's fine.

smrobtzz · 2025-05-04T19:31:39Z

I've added better validation that should catch the obviously wrong combinations of CPU, architecture, ABI, and removed the explicit check for noabicalls

workingjubilee

Things look good overall now. I have some notes that might be useful to reference for later revisions of this code. Please feel free to reword them as you prefer, especially if they need corrections for accuracy.

compiler/rustc_codegen_ssa/src/back/metadata.rs

workingjubilee · 2025-05-05T03:22:30Z

compiler/rustc_codegen_ssa/src/back/metadata.rs

+                _ => {
+                    if architecture == Architecture::Mips {
+                        e_flags |= elf::EF_MIPS_ABI_O32
+                    }
+                }


I am okay with the 32-bit case, I think, being inferred, due to LLVM not supporting other alternative MIPS ABIs for 32-bit MIPS (though supposedly they exist)

compiler/rustc_codegen_ssa/src/back/metadata.rs

Co-authored-by: Jubilee <[email protected]>

workingjubilee · 2025-05-05T18:21:07Z

Thanks!

Hopefully this fixes the thing you're trying to wrangle.

@bors r+

bors · 2025-05-05T18:21:10Z

📌 Commit 57941af has been approved by workingjubilee

It is now in the queue for this repository.

workingjubilee · 2025-05-05T18:44:06Z

Note that if you develop a need for a MIPS N32 target for rustc to continue to work on an ongoing basis, it would be somewhat more to-the-point to add a tier 3 target by sending a PR for it and agreeing to be the point of contact for it. Until then we may arbitrarily stomp on this code, depending on the reviewer's whimsy. We technically may do so to tier 3 targets, but at that point we have someone to ask for help and will try to do so as a matter of courtesy, so that we can together explore options that aren't simply shrugging and blithely paving over the target's supporting code.

…jubilee Use more accurate ELF flags on MIPS Changes the MIPS ELF flags used for metadata objects to be closer to what LLVM uses so the linker doesn't complain

Rollup of 12 pull requests Successful merges: - rust-lang#139550 (Fix `-Zremap-path-scope` rmeta handling) - rust-lang#139773 (Implement `Iterator::last` for `vec::IntoIter`) - rust-lang#140035 (Implement RFC 3503: frontmatters) - rust-lang#140176 (Fix linking statics on Arm64EC) - rust-lang#140251 (coverage-dump: Resolve global file IDs to filenames) - rust-lang#140393 (std: get rid of `sys_common::process`) - rust-lang#140532 (Fix RustAnalyzer discovery of rustc's `stable_mir` crate) - rust-lang#140598 (Steer docs to `utf8_chunks` and `Iterator::take`) - rust-lang#140634 (Use more accurate ELF flags on MIPS) - rust-lang#140673 (Clean rustdoc tests folder) - rust-lang#140678 (Be a bit more relaxed about not yet constrained infer vars in closure upvar analysis) - rust-lang#140687 (Update mdbook to 0.4.49) r? `@ghost` `@rustbot` modify labels: rollup

…jubilee Use more accurate ELF flags on MIPS Changes the MIPS ELF flags used for metadata objects to be closer to what LLVM uses so the linker doesn't complain

Rollup of 12 pull requests Successful merges: - rust-lang#139550 (Fix `-Zremap-path-scope` rmeta handling) - rust-lang#139764 (Consistent trait bounds for ExtractIf Debug impls) - rust-lang#139773 (Implement `Iterator::last` for `vec::IntoIter`) - rust-lang#140035 (Implement RFC 3503: frontmatters) - rust-lang#140251 (coverage-dump: Resolve global file IDs to filenames) - rust-lang#140393 (std: get rid of `sys_common::process`) - rust-lang#140532 (Fix RustAnalyzer discovery of rustc's `stable_mir` crate) - rust-lang#140598 (Steer docs to `utf8_chunks` and `Iterator::take`) - rust-lang#140634 (Use more accurate ELF flags on MIPS) - rust-lang#140673 (Clean rustdoc tests folder) - rust-lang#140678 (Be a bit more relaxed about not yet constrained infer vars in closure upvar analysis) - rust-lang#140687 (Update mdbook to 0.4.49) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned workingjubilee May 4, 2025

rustbot added 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 4, 2025

workingjubilee reviewed May 4, 2025

View reviewed changes

workingjubilee requested changes May 4, 2025

View reviewed changes

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 4, 2025

smrobtzz force-pushed the mips-elf-fixes branch from 4542d01 to 2fdfefc Compare May 4, 2025 19:14

smrobtzz requested a review from workingjubilee May 4, 2025 19:32

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 4, 2025

This comment has been minimized.

Sign in to view

smrobtzz force-pushed the mips-elf-fixes branch from 2fdfefc to 527dc5a Compare May 4, 2025 20:25

workingjubilee approved these changes May 5, 2025

View reviewed changes

smrobtzz and others added 2 commits May 5, 2025 12:23

Use more accurate ELF flags on MIPS

fa6d0d1

Apply suggestions from code review

57941af

Co-authored-by: Jubilee <[email protected]>

smrobtzz force-pushed the mips-elf-fixes branch from 715699f to 57941af Compare May 5, 2025 16:34

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 May 5, 2025

Zalathar mentioned this pull request May 6, 2025

Rollup of 12 pull requests #140690

Closed

Zalathar mentioned this pull request May 6, 2025

Rollup of 12 pull requests #140695

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use more accurate ELF flags on MIPS #140634

Use more accurate ELF flags on MIPS #140634

smrobtzz commented May 4, 2025

rustbot commented May 4, 2025

rustbot commented May 4, 2025

workingjubilee commented May 4, 2025

workingjubilee May 4, 2025 •

edited

Loading

smrobtzz May 4, 2025

workingjubilee May 4, 2025

smrobtzz May 4, 2025

workingjubilee May 5, 2025 •

edited

Loading

workingjubilee May 5, 2025

workingjubilee left a comment •

edited

Loading

workingjubilee May 4, 2025

smrobtzz May 4, 2025

workingjubilee May 5, 2025

jieyouxu May 5, 2025 •

edited

Loading

smrobtzz May 5, 2025

workingjubilee May 4, 2025 •

edited

Loading

workingjubilee May 4, 2025 •

edited

Loading

smrobtzz May 4, 2025

smrobtzz commented May 4, 2025

smrobtzz commented May 4, 2025

This comment has been minimized.

workingjubilee left a comment

workingjubilee May 5, 2025

workingjubilee commented May 5, 2025

bors commented May 5, 2025

workingjubilee commented May 5, 2025

Use more accurate ELF flags on MIPS #140634

Are you sure you want to change the base?

Use more accurate ELF flags on MIPS #140634

Conversation

smrobtzz commented May 4, 2025

rustbot commented May 4, 2025

rustbot commented May 4, 2025

workingjubilee commented May 4, 2025

workingjubilee May 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee May 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu May 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee May 4, 2025 • edited Loading

Choose a reason for hiding this comment

workingjubilee May 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smrobtzz commented May 4, 2025

smrobtzz commented May 4, 2025

This comment has been minimized.

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented May 5, 2025

bors commented May 5, 2025

workingjubilee commented May 5, 2025

workingjubilee May 4, 2025 •

edited

Loading

workingjubilee May 5, 2025 •

edited

Loading

workingjubilee left a comment •

edited

Loading

jieyouxu May 5, 2025 •

edited

Loading

workingjubilee May 4, 2025 •

edited

Loading

workingjubilee May 4, 2025 •

edited

Loading