Skip to content

llvm-mc uses wrong ABI for RISC-V platform on Linux by default #123077

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

Closed
directhex opened this issue Jan 15, 2025 · 10 comments
Closed

llvm-mc uses wrong ABI for RISC-V platform on Linux by default #123077

directhex opened this issue Jan 15, 2025 · 10 comments
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl'

Comments

@directhex
Copy link

This is a related issue to #115679 and possibly #50591. Generally, the default ABI handling is done differently in every tool, where https://reviews.llvm.org/D69383 defined the current behaviour for clang.

Roughly speaking, it is reasonable for a user to expect these two compiler invocations to act in an equivalent way:

$ clang -o bottles.o bottles.c
$ clang -S -o bottles.S bottles.c
$ llvm-mc --filetype=obj -o bottles.o bottles.S

However, they don't. Without use of the (undocumented) -target-abi=lp64d flag, llvm-mc targets a different ABI and the resulting object files cannot be linked with objects produced by clang/clang++

Per the changes in D69383 to clang/lib/Driver/ToolChains/Arch/RISCV.cpp, the logic used by clang to this day is:

    if (Triple.getOS() == llvm::Triple::UnknownOS)
      return "lp64";
    else
      return "lp64d";

i.e. use lp64 unless a named OS is used, in which case use lp64d. Explicitly forcing an OS into the triple (i.e. passing -triple riscv64-unknown-linux-gnu to llvm-mc) does not result in the clang behaviour of defaulting to lp64d when calling llvm-mc.

As time allows, I intend to try and get a PR filed to unify llvm-mc's behaviour - I wanted to document the issue first.

@EugeneZelenko EugeneZelenko added llvm-tools All llvm tools that do not have corresponding tag and removed new issue labels Jan 15, 2025
@directhex
Copy link
Author

so llvm/lib/MC/MCTargetOptionsCommandFlags.cpp's llvm::mc::RegisterMCTargetOptionsFlags::RegisterMCTargetOptionsFlags() is used by llvm-mc and loads the default via Options.ABIName = getABIName(). Getting a bit lost in what's being included from where, but I have a bad feeling about computeTargetABI in llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp being the problem (specifically https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp#L77-L81 sure feels like it's parsing feature bits but not considering target OS, which is the differentiator used between lp64/lp64d defaults in clang)

@topperc
Copy link
Collaborator

topperc commented Jan 15, 2025

As far as I know, llvm-mc also doesn't default to having F and D extensions enabled. You need to add -mattr=+d. I think adding -mattr=+d will change the default ABI.

@directhex
Copy link
Author

As far as I know, llvm-mc also doesn't default to having F and D extensions enabled. You need to add -mattr=+d. I think adding -mattr=+d will change the default ABI.

llvm-mc will throw a warning without -mattr=+d, but it does emit an object with the correct ELF tags in it at least:

$ llvm-mc -o bottles.o bottles.S --filetype=obj
$ file bottles.o 
bottles.o: ELF 64-bit LSB relocatable, UCB RISC-V, soft-float ABI, version 1 (SYSV), not stripped

$ llvm-mc -target-abi=lp64d -o bottles.o bottles.S --filetype=obj
Hard-float 'd' ABI can't be used for a target that doesn't support the D instruction set extension (ignoring target-abi)
$ file bottles.o 
bottles.o: ELF 64-bit LSB relocatable, UCB RISC-V, double-float ABI, version 1 (SYSV), not stripped

$ llvm-mc -mattr=+d -o bottles.o bottles.S --filetype=obj
$ file bottles.o
bottles.o: ELF 64-bit LSB relocatable, UCB RISC-V, double-float ABI, version 1 (SYSV), not stripped

I would need a more sophisticated test file than 99 bottles to see what the actual ABI looks like in the .o file, beyond the claims made in the ELF flag. And the point remains that the defaults don't match between tools.

@topperc
Copy link
Collaborator

topperc commented Jan 15, 2025

And the point remains that the defaults don't match between tools.

I'm not sure llvm-mc should have OS triple dependent behavior. It's a low level tool.

CC @MaskRay

@EugeneZelenko EugeneZelenko added mc Machine (object) code and removed llvm-tools All llvm tools that do not have corresponding tag labels Jan 15, 2025
@arichardson
Copy link
Member

And the point remains that the defaults don't match between tools.

I'm not sure llvm-mc should have OS triple dependent behavior. It's a low level tool.

CC @MaskRay

I agree with this and would not want any of that logic in these developer tools.

That said I know many years ago I initially got confused about llvm-mc and thought it could be used as a replacement for GNU as. Maybe the real solution here is to provide a hypothetical clang-as tool that defaults to processing assembly input (and maybe add support for some of the gnu as flags?).

topperc added a commit that referenced this issue Jan 16, 2025
This option is very important for RISC-V as it controls calling
convention and a field in the ELF header. It is used in a large number
of RISC-V lit tests.

Expose the option to -help.

Fixes one issue raised in #123077
@directhex
Copy link
Author

That said I know many years ago I initially got confused about llvm-mc and thought it could be used as a replacement for GNU as. Maybe the real solution here is to provide a hypothetical clang-as tool that defaults to processing assembly input (and maybe add support for some of the gnu as flags?).

I assume some relevant code owner would need to sign off on that concept before I sit down & try to implement it

@MaskRay
Copy link
Member

MaskRay commented Jan 18, 2025

(Travelling with limited Internet access)

llvm-mc, was originally designed for internal testing of the Machine Code (MC) layer, particularly the assembler and disassembler components. Despite its internal origins, llvm-mc is often distributed as part of the LLVM toolset, making it accessible to users.

That said, users should really not treat llvm-mc as user-facing utility (which normally use OptTable instead llvm::cl). Some folks want to add a gas compatible tool in LLVM, but so far I don't think there is sufficient justification for the complexity.

https://briancallahan.net/blog/20240122.html is a nice write-up.

Agreed that we try to keep target-specific behaviors minimal for llvm-mc. I don't think coding RISC-V specific target-specific behaviors is within the scope of llvm-mc, which would make testing more brittle. Testing is still the primary use case of llvm-mc.

@directhex
Copy link
Author

I'm closing, on the basis of 3acb0e9

It seems llvm-mc usage should be replaced by clang -x assembler, and that means Clang-style defaults.

@luyanaa
Copy link

luyanaa commented Jan 23, 2025

Sorry for the inconvenience of replying to a closed issue.

(I built the AMD ROCm 5.4.0 for RISC-V for testing and had a temporarily suspended project ROCm-port to make ROCm runnable on any reasonable architecture)

For example, in the ROCm OpenCL Runtime here, it just uses "llvm-mc" directly.
https://github.com/ROCm/clr/blob/amd-staging/hipamd/src/hip_embed_pch.sh
Line 159: $LLVM_DIR/bin/llvm-mc -o hip_pch.o $tmp/hip_pch.mcin --filetype=obj &&
(Similar behavior happens in hipamd/src/hiprtc/CMakeLists.txt)
This line will generate lp64 instead of lp64d objects.

Previously, even after fixing these two parts, HIP will still use llvm-mc in the background in clang/lib/Driver/ToolChains/HIPUtility.cpp:HIP::constructGenerateObjFileFromHIPFatBinary but luckily it has been corrected recently in #112041.

  1. It's unknown to us if any other existing code utilizes this feature (if ROCm utilizes it, it's possible that others would use similar tricks)
  2. It can be taken as a RISC-V-specified problem (Most common 64-bit architecture supported in LLVM mainline sticks to a default ABI, eg: x86_64, ARM64, LoongArch, Power)

So, (just from my/our perspective), it causes extra architecture-specified complexity for us to fix this problem.

And since lp64d is already the default ABI for RV64, and during our current effort we didn't find anywhere other than llvm-mc has the same issue(most should have been fixed in the lp64 to lp64d transition or never happened), I think it should be corrected.

@EugeneZelenko EugeneZelenko added clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' and removed mc Machine (object) code labels Jan 23, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 23, 2025

@llvm/issue-subscribers-clang-driver

Author: Jo Shields (directhex)

This is a related issue to https://github.com//issues/115679 and possibly https://github.com//issues/50591. Generally, the default ABI handling is done differently in every tool, where https://reviews.llvm.org/D69383 defined the current behaviour for clang.

Roughly speaking, it is reasonable for a user to expect these two compiler invocations to act in an equivalent way:

$ clang -o bottles.o bottles.c
$ clang -S -o bottles.S bottles.c
$ llvm-mc --filetype=obj -o bottles.o bottles.S

However, they don't. Without use of the (undocumented) -target-abi=lp64d flag, llvm-mc targets a different ABI and the resulting object files cannot be linked with objects produced by clang/clang++

Per the changes in D69383 to clang/lib/Driver/ToolChains/Arch/RISCV.cpp, the logic used by clang to this day is:

    if (Triple.getOS() == llvm::Triple::UnknownOS)
      return "lp64";
    else
      return "lp64d";

i.e. use lp64 unless a named OS is used, in which case use lp64d. Explicitly forcing an OS into the triple (i.e. passing -triple riscv64-unknown-linux-gnu to llvm-mc) does not result in the clang behaviour of defaulting to lp64d when calling llvm-mc.

As time allows, I intend to try and get a PR filed to unify llvm-mc's behaviour - I wanted to document the issue first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl'
Projects
None yet
Development

No branches or pull requests

7 participants