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

elf: add EF_MIPS_ABI_* e_flags constants #433

Merged
merged 3 commits into from
May 2, 2022
Merged

Conversation

ayrtonm
Copy link
Contributor

@ayrtonm ayrtonm commented May 1, 2022

This adds a few constants that are needed to fix some targets that are broken on nightly rustc. Values were taken from the LLVM source.

Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add EF_MIPS_ABI too. Also add support in the readobj example. There's existing support for EF_MIPS_ARCH that you can use as a guide.

@ayrtonm
Copy link
Contributor Author

ayrtonm commented May 1, 2022

I just added the EF_MIPS_ABI mask and updated readobj. I'm not entirely sure whether EF_MIPS_ABI2 should be moved to the new flags section in readobj or not. I kept it where it was since the EF_MIPS_ABI mask doesn't cover EF_MIPS_ABI2, but I could see an argument for either case.

Edit: I still need to update the testfiles

@philipc
Copy link
Contributor

philipc commented May 1, 2022

I think EF_MIPS_ABI2 is correct where it is, because, as you say, it is not covered by the EF_MIPS_ABI mask.

@ayrtonm
Copy link
Contributor Author

ayrtonm commented May 1, 2022

The existing MIPS test files show <unknown> (0x0) for the new field since they use the 64-bit ABI. I have a MIPS binary using the O32 ABI that I can check in to the object-testfiles repo to actually test the new flags. Is there anything else I'd need to do to update the tests?

@philipc
Copy link
Contributor

philipc commented May 1, 2022

As long as the tests pass for you locally, then this PR would just need to update the submodule too. I don't think there's anything else.

@philipc
Copy link
Contributor

philipc commented May 1, 2022

We may want to check that the EF_MIPS_ABI fields is non-zero, rather than printing unknown. For example:

if section.characteristics.get(LE) & IMAGE_SCN_ALIGN_MASK != 0 {
p.flags(
section.characteristics.get(LE),
IMAGE_SCN_ALIGN_MASK,
FLAGS_IMAGE_SCN_ALIGN,
);
}

@ayrtonm
Copy link
Contributor Author

ayrtonm commented May 2, 2022

We may want to check that the EF_MIPS_ABI fields is non-zero, rather than printing unknown.

Done and I updated the testfile references. I think the only thing left is updating the submodule, but I assume that has to be after the test binaries are merged.

@philipc philipc merged commit 70a955c into gimli-rs:master May 2, 2022
@philipc
Copy link
Contributor

philipc commented May 2, 2022

Thanks! I'm guessing you want a release with this, so I'll try to do one in the near future.

@overdrivenpotato
Copy link

A release would help fix some issues in the rust-psp target too 🙂

@philipc
Copy link
Contributor

philipc commented May 9, 2022

Published 0.28.4.

ayrtonm added a commit to ayrtonm/rust that referenced this pull request May 11, 2022
In rust-lang#95604 the compiler started generating a temporary symbols.o which is added
to the linker invocation. This object file has an `e_flags` which may be invalid
for 32-bit MIPS targets. Even though symbols.o doesn't contain code, linking
    with [lld fails](https://github.com/llvm/llvm-project/blob/main/lld/ELF/Arch/MipsArchTree.cpp#L79) with
```
rust-lld: error: foo-cgu.0.rcgu.o: ABI 'o32' is incompatible with target ABI 'n64'
```
because it omits the ABI bits (EF_MIPS_ABI_O32) so lld assumes it's using the
N64 ABI. This breaks linking on nightly for the out-of-tree [psx
target](ayrtonm/psx-sdk-rs#9), the builtin
mipsel-sony-psp target (cc @overdrivenpotato) and any other 32-bit MIPS
target using lld.

This PR sets the ABI in `e_flags` to O32 since that's the only ABI for 32-bit
MIPS that LLVM supports. It also sets other `e_flags` bits based on the target.
I had to bump the object crate version since some of these constants were [added
recently](gimli-rs/object#433). I'm not sure if this
PR needs a test, but I can confirm that it fixes the linking issue on both
targets I mentioned.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 13, 2022
Fix e_flags for 32-bit MIPS targets in generated object file

In rust-lang#95604 the compiler started generating a temporary symbols.o which is added to the linker invocation. This object file has an `e_flags` which is invalid for 32-bit MIPS targets. Even though symbols.o doesn't contain code, linking these targets with [lld fails](https://github.com/llvm/llvm-project/blob/main/lld/ELF/Arch/MipsArchTree.cpp#L76-L79) with
```
rust-lld: error: foo-cgu.0.rcgu.o: ABI 'o32' is incompatible with target ABI 'n64'
```
because it omits the ABI bits (`EF_MIPS_ABI_O32`) so lld assumes it's using the N64 ABI. This breaks linking on nightly for the out-of-tree [mipsel-sony-psx target](ayrtonm/psx-sdk-rs#9), the builtin mipsel-sony-psp target (cc `@overdrivenpotato)` and probably any other 32-bit MIPS target using lld.

This PR sets the ABI in `e_flags` to O32 since that's the only ABI for 32-bit MIPS that LLVM supports. It also sets other `e_flags` bits based on the target to avoid similar issues with the object file arch and PIC. I had to bump the object crate version since some of these constants were [added recently](gimli-rs/object#433). I'm not sure if this PR needs a test, but I can confirm that it fixes the linking issue on both targets I mentioned.
mcbegamerxx954 pushed a commit to mcbegamerxx954/object that referenced this pull request Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants