Skip to content

LLVM target's OS/ABI is lost #97535

Description

@mkroening

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 <gary@garyguo.net>
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

Activity

  1. added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on May 29, 2022
  2. nbdd0121 commented on May 30, 2022

    @nbdd0121
    Member

    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?

  3. mkroening commented on May 30, 2022

    @mkroening
    ContributorAuthor

    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.

  4. nbdd0121 commented on May 30, 2022

    @nbdd0121
    Member

    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?

  5. petrochenkov commented on May 30, 2022

    @petrochenkov
    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.

  6. mkroening commented on May 30, 2022

    @mkroening
    ContributorAuthor

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

  7. apiraino commented on May 31, 2022

    @apiraino
    Contributor

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

    @rustbot label -I-prioritize +P-low

  8. added
    P-lowLow priority
    and removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on May 31, 2022
  9. added
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    on May 31, 2022
  10. mkroening commented on Jun 1, 2022

    @mkroening
    ContributorAuthor

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

  11. added 2 commits that reference this issue on Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-bugCategory: This is a bug.P-lowLow priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.regression-untriagedUntriaged performance or correctness regression.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions