Skip to content

Conversation

@fneddy
Copy link
Contributor

@fneddy fneddy commented Jan 7, 2026

as we want to enable rust in kernel on s390x we need to do some things:

  • allow the use of soft-float feature only if also soft-float ABI is enabled
  • add a -packed-stack rustc option. this adds the packed-stack function-attribute to every function call. I implemented it the same way as it is implemented in clang12. In clang this is a cli argument. It is not a part of the target feature. Therefore we cannot pass it via the features field in the target.json.
  • added a check for packed-stack && backchain** && !soft-float as this combination should never occur. However I don't know if I put it in the right spot. 3

Footnotes

  1. https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-mpacked-stack

  2. https://github.com/llvm/llvm-project/blob/release/20/clang/lib/CodeGen/CodeGenFunction.cpp#L1202-L1208

  3. compiler/rustc_target/src/spec/mod.rs

@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2026

These commits modify compiler targets.
(See the Target Tier Policy.)

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 7, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2026

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@fneddy fneddy marked this pull request as draft January 7, 2026 14:04
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 7, 2026
@fneddy fneddy force-pushed the new_target_s390x_softfloat branch from 7fdc148 to 30628ca Compare January 7, 2026 14:09
@fneddy fneddy marked this pull request as ready for review January 7, 2026 14:10
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 7, 2026
Comment on lines 490 to 493
## packed-stack

This flag enables packed StackFrames on s390x.
If backchain is also enabled, switch to the soft-float ABI.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should say something about allowed values and what the default is. (if only for consistency).

Also i like your phrasing from above better: enabling both packed-stack and backchain is incompatible with hard-float, but is compatible with soft-float. The current text could be read to mean that soft-float would be switched to automatically.

Comment on lines +1214 to +1215
// On s390x only the hard-float ABI is valid. However for kernel compilation we need to
// allow soft-float if and only if the ABI is explicitly set to soft-float.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this refer to the linux kernel? Also I feel like this mixes up the SoftFloat ABI and the soft-float feature, maybe you can clarify which way the implication goes?

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Having flags that change ABI is a problem because everything quietly breaks if you don't rebuild the world (among other problems); I don't think we want to add this to any more targets, we're trying to rip it out. See context at #116344, cc @RalfJung.

The thing to do is to submit an MCP to create a new separate -softfloat target, like we have with aarch64-unknown-none-softfloat, loongarch64-unknown-none-softfloat, x86_64-unknown-none (softfloat is implied there), and others.

Also cc @uweigand @cuviper target maintainers

View changes since this review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 8, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

FeatureConstraints { required: &[], incompatible: &["soft-float"] }
// On s390x only the hard-float ABI is valid. However for kernel compilation we need to
// allow soft-float if and only if the ABI is explicitly set to soft-float.
if matches!(self.abi, Abi::SoftFloat) {
Copy link
Member

@RalfJung RalfJung Jan 8, 2026

Choose a reason for hiding this comment

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

self.abi is just used for cfg(target_abi), it doesn't actually control the ABI.

It looks like s390x is just as bad as x86 when it comes to ABI handling so the logic should probably be similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is only one valid abi on s390x. This ABI uses hard-float.

The idea is that soft-floatremains incompatible unless the user explicitly requests to use the soft-float abi(e.g.1). This will not change the abi, as there is no other ABI specified. And to my knowledge there wont be a soft-float abi specified that really could be used by llvm.

It is used as a gate, so if you want to use soft-float you also have to explicitly change the ABI. And make it clear that this code is incompatible with the normal code.

Footnotes

  1. https://github.com/torvalds/linux/compare/master...fneddy:linux:add_rustc_s390x#diff-8b2b79dc9fe8130845984440e8df685c928623dfe1ebd12fe937efe4ab352aa9R267

Copy link
Contributor

Choose a reason for hiding this comment

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

This will not change the abi, as there is no other ABI specified.

What exactly do you mean by this - floats still get passed in vregs even with soft-float? What does the flag do then?

Copy link
Member

Choose a reason for hiding this comment

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

There might only be one official ABI, but the soft-float ABI is real and we're not going to pretend that it isn't. The soft-float ABI is whatever LLVM does to pass float args across function barriers when the soft-float target feature is on. So there might be no specification for such an ABI, but that doesn't stop LLVM from using it.

That's what happened on x86, anyway. I suspect it is similar on s390x.

Copy link
Member

Choose a reason for hiding this comment

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

so if you want to use soft-float you also have to explicitly change the ABI.

Exactly. But self.abi does not change the ABI, it just tells cfg what the ABI is. llvm_abiname, llvm_floatabi, and rustc_abi change the ABI.

(Well, that's not entirely true. self.abi actually is used in some places in the ABI adjustment logic. It's all a bit messy unfortunately...)

Copy link
Member

Choose a reason for hiding this comment

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

Note that further up this file it still says

("soft-float", Forbidden { reason: "currently unsupported ABI-configuration feature" }, &[]),

So using the soft-float target feature still doesn't work even with this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

On that note, it would be great to have codegen tests for these kind of flags.

}

if sess.opts.cg.packed_stack {
Some(llvm::CreateAttrString(cx.llcx, "packed-stack"))
Copy link
Member

Choose a reason for hiding this comment

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

What are the ABI considerations for this flag? Is it possible to freely mix code built with and without this flag? If not, it needs to be made a "target modifier" to avoid unsoundness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes packed-stack and non packed stack code can be mixed. It just affects the calling code and not the called code.

overflow_checks: Option<bool> = (None, parse_opt_bool, [TRACKED],
"use overflow checks for integer arithmetic"),
packed_stack: bool = (false, parse_bool, [TRACKED],
"use packed stack frames (s390x only) (default: no)"),
Copy link
Member

Choose a reason for hiding this comment

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

It's bad UI to just silently ignore the flag for targets where it does not make sense. Please ensure it raises an error instead.

@rust-log-analyzer
Copy link
Collaborator

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    |
319 |     if sess.opts.cg.packed_stack {
    |                     ^^^^^^^^^^^^ unknown field
    |
    = note: available fields are: `ar`, `code_model`, `codegen_units`, `collapse_macro_debuginfo`, `control_flow_guard` ... and 48 others

For more information about this error, try `rustc --explain E0609`.
[RUSTC-TIMING] rustc_codegen_llvm test:false 3.395
error: could not compile `rustc_codegen_llvm` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

@nnethercote
Copy link
Contributor

I don't know anything about this stuff, so I will hand this over to a target maintainer:

r? @cuviper

@rustbot rustbot assigned cuviper and unassigned nnethercote Jan 8, 2026
@cuviper
Copy link
Member

cuviper commented Jan 8, 2026

cc @uweigand, in case you're available to look at this, otherwise I'll try to review it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants