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

LLVM target's OS/ABI is lost #97535

Closed
mkroening opened this issue May 29, 2022 · 7 comments · Fixed by #97633
Closed

LLVM target's OS/ABI is lost #97535

mkroening opened this issue May 29, 2022 · 7 comments · Fixed by #97633
Labels
C-bug Category: This is a bug. P-low Low priority regression-untriaged Untriaged performance or correctness regression. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mkroening
Copy link
Contributor

When compiling for x86_64-unknown-hermit, the resulting binaries should have the OS/ABI (EI_OSABI) field in the ELF header set to Standalone (0xff) as per the x86_64-unknown-hermit LLVM target, which it did before nightly-2022-04-26. Now, it is set to "UNIX - System V", which I only tested on Linux.

git-bisect identified the introduction of synthetic object files for exported symbols 773f533 from #95604 to have changed this:

773f533eae25129cea7241b74e54f26ce5eebb62 is the first bad commit
commit 773f533eae25129cea7241b74e54f26ce5eebb62
Author: Gary Guo <[email protected]>
Date:   Sat Apr 2 22:54:51 2022 +0100

    Synthesis object file for `#[used]` and exported symbols

 compiler/rustc_codegen_ssa/src/back/link.rs        | 69 ++++++++++++++++++++++
 compiler/rustc_codegen_ssa/src/back/linker.rs      | 46 +++++++++++++++
 compiler/rustc_codegen_ssa/src/back/metadata.rs    |  2 +-
 .../rustc_codegen_ssa/src/back/symbol_export.rs    | 54 ++++++++++++++---
 compiler/rustc_codegen_ssa/src/base.rs             |  7 +++
 compiler/rustc_codegen_ssa/src/lib.rs              |  2 +
 .../rustc_middle/src/middle/exported_symbols.rs    | 10 ++++
 7 files changed, 182 insertions(+), 8 deletions(-)

I did not have a deep dive into the code yet, but I assume the synthetic object file has the host's OS/ABI, which is prevalent over the other OS/ABI.

@nbdd0121, do you have any insight?

Related: hermit-os/hermit-rs#300

@mkroening mkroening added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels May 29, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 29, 2022
@nbdd0121
Copy link
Contributor

Hmm, the object crate sets EI_OSABI to NONE, which seems to be an alias of SYSV.

Unfortunately currently object crate provide no ways to set this field, so to fix this I suppose some postprocessing of the generated ELF file would be needed.

I am surprised that this doesn't show up earlier, given that exactly the same routine is used to create rmeta files. Does hermit not support dylibs?

@mkroening
Copy link
Contributor Author

Ah, yes, object sets the OS/ABI here: object.rs#L297.

I think there are three possibilities:

  1. Add proper support for setting OS/ABI in object::write::Object. This would probably take a lot of effort.
  2. Patch file.write(): Vec<u8> before passing it to std::fs::write in
    let result = std::fs::write(&path, file.write().unwrap());

    This can be done quite easily.
  3. Pass the synthetic object file after (?) the real ones. I think lld infers the OS/ABI from the first proper input object file, if I am reading this correctly: Driver.cpp#L1649

For 1 and 2 we would need to know the proper OS/ABI from the real object files though. I am not sure how easy that would be to come by. What do you think?

I am surprised that this doesn't show up earlier, given that exactly the same routine is used to create rmeta files. Does hermit not support dylibs?

Hermit is a unikernel, which is linked statically to the application. There is no support for dynamic libraries.

A similar problem occurs when using linker-plugin-lto though (#73606). This issue was closed, because we did not need linker-plugin-lto anyways.

@nbdd0121
Copy link
Contributor

Here's how LLVM determines what OSABI to use: https://github.com/llvm/llvm-project/blob/18c1ee04de448a6a4babbaf8e5ba42866339b466/llvm/include/llvm/MC/MCELFObjectWriter.h. We could look at sess.target.options.os and use the same logic to determine the OSABI.

The synthetic object can be placed after real object files (it only needs to be linked before any libraries), but assuming the linker only looks at the first object file sounds a bit hacky.

cc @petrochenkov, what's your take on this issue?

@petrochenkov
Copy link
Contributor

object-rs has only recently been taught to set e_flags expected by some MIPS targets (#96930).
In think we can teach object-rs to set EI_OSABI too, and then just set it, if some linker expects it.

@mkroening
Copy link
Contributor Author

All right. Thanks for the quick feedback. I'll cook something up in the coming days.

@apiraino
Copy link
Contributor

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-low

@rustbot rustbot added P-low Low priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 31, 2022
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 31, 2022
@mkroening
Copy link
Contributor Author

I opened #97633, but first the necessary changes in object have to be released.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 24, 2022
@bors bors closed this as completed in 0af99c9 Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-low Low priority regression-untriaged Untriaged performance or correctness regression. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants