Conversation
LLVM 24 is pickier about things like hard-float being disabled but the compiled module ABI mentioning hard-float. As an example, if the user specifies target-features=-d to disable the d extension but the declared target-abi is lp64d that's now an error where before I guess it was a warning. This fix seems somewhat inelegant, but in the name of keeping the behavior changes minimal I did gate the new behavior to only happen on LLVM 24. I'm very open to alternative solutions! An LLM was used to identify the breaking commit and help me sort out _why_ the commit was breaking.
rustbot
added
A-LLVM
|
r? @mejrs rustbot has assigned @mejrs. Use Why was this reviewer chosen?The reviewer was selected based on:
|
rustbot
added
the
llvm-main
|
Apologies for this breakage. My change was intended to buy NFC but apparently there is no test coverage for this. While longer term this should probably be an error it's definitely not supposed to cause churn for downstreams. |
|
It does seem like this change is an improvement regardless of whether my commit is reverted or not so it definitely LGTM from a functional point. Not a rust expert so can't comment on style or anything else. |
|
cc @RalfJung |
|
Agreed, the proposed behavior in this PR is IMO a very bad idea. It's basically re-introducing the broken LLVM behavior that LLVM finally fixed, of silently changing the ABI. We should never silently change the ABI, that's plain unsound.
Agreed. This matches #161280. |
No need to apologize, I am very happy that LLVM is finally moving towards actually complaining about nonsensical ABI requests from the frontend, rather than silently doing something arbitrary. :) llvm/llvm-project#111334 deliberately introduced such errors for the ARM backend. I'd love for RISCV and all the other backends to do the same. |
…d by streamer Commit 105ff16 (llvm#213410) changed RISCVABI::computeTargetABI() to return Expected<ABI> and added validation to RISCVAsmParser::onBeginOfFile() to report invalid -target-abi flags with real source location in llvm-mc. However, this broke LTO builds containing inline assembly (e.g. Android riscv64 builds and downstream Rust in rust-lang/rust#162783). During LTO, LLD sets TargetOptions.MCOptions.ABIName from the module's target-abi metadata ("lp64d"), while the linker's default TargetMachine subtarget lacks "+d" (individual functions specify "+d" in target-features). RISCVSubtarget handles this gracefully by emitting a diagnostic note and falling back to lp64 for code generation. When inline assembly was subsequently parsed, AsmPrinter::emitInlineAsm instantiated RISCVAsmParser with a subtarget lacking "+d", causing onBeginOfFile() to re-validate TargetOptions.ABIName and fail with a fatal error. To fix this, skip the parser validation whenever the streamer has already resolved an ABI. This preserves llvm-mc diagnostics on invalid command-line flags while avoiding conflicting validation when assembling inline asm during code generation. This commit was created with the help of AI tools
|
Sounds good. Any thoughts on what the right fix for |
|
Changing the |
|
Given llvm/llvm-project#223606 it seems like the LLVM error here is considered a bug by upstream LLVM? If they plan to revert that change there's probably no point in us making this a hard error. That said, I'd prefer if LLVM actually made this an error consistently, not just for this specific inline-asm-LTO corner case. ;) |
6 participants
LLVM 24 is pickier about things like hard-float being disabled but the compiled module ABI mentioning hard-float. As an example, if the user specifies target-features=-d to disable the d extension but the declared target-abi is lp64d that's now an error where before I guess it was a warning.
This fix seems somewhat inelegant, but in the name of keeping the behavior changes minimal I did gate the new behavior to only happen on LLVM 24. I'm very open to alternative solutions!
An LLM was used to identify the breaking commit and help me sort out why the commit was breaking.
@rustbot label: +llvm-main