Skip to content

add rustc option -Zpacked-stack#152432

Open
fneddy wants to merge 2 commits intorust-lang:mainfrom
fneddy:s390x_packed_stack_attribute
Open

add rustc option -Zpacked-stack#152432
fneddy wants to merge 2 commits intorust-lang:mainfrom
fneddy:s390x_packed_stack_attribute

Conversation

@fneddy
Copy link
Contributor

@fneddy fneddy commented Feb 10, 2026

this enables -Zpacked-stack just as -mpacked-stack in clang and gcc. packed-stack is needed on s390x for kernel development.

For reference: #151154 and #150766

look at @uweigand s post for full explanation of what this does. Here a wrap-up:

#150766 (comment)

[...]
packed-stack [...] modifies how the compiler-generated function prolog/epilog code makes use of the 160 byte register save area provided by a caller to the callee [...] this variant is not actually incompatible with the ABI - packed-stack and regular functions can freely call each other without ABI issues.
[...]
combination of -mpacked-stack and -mbackchain [...] the location in the stack frame where the backchain link ought to be stored is not available. [...] is not supported at all with the default ABI
[...]
However, in the special case of also using soft-float, our (implied) soft-float ABI provides a different location for the backchain that is compatible with -mpacked-stack, so that combination should be supported
[...]

@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 Feb 10, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2026

r? @chenyukang

rustbot has assigned @chenyukang.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 66 candidates
  • Random selection from 14 candidates

@fneddy
Copy link
Contributor Author

fneddy commented Feb 10, 2026

@fneddy fneddy marked this pull request as draft February 10, 2026 10:24
@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 Feb 10, 2026
@fneddy
Copy link
Contributor Author

fneddy commented Feb 10, 2026

I'll add tests for assembly output and for target incompatibility.

@rust-log-analyzer

This comment has been minimized.

@fneddy fneddy force-pushed the s390x_packed_stack_attribute branch from 5ddcabb to 4ebec9d Compare February 10, 2026 11:29
@fneddy fneddy marked this pull request as ready for review February 13, 2026 11:19
@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 Feb 13, 2026
@rust-bors

This comment has been minimized.

@fneddy fneddy force-pushed the s390x_packed_stack_attribute branch from 0d6d167 to 0c182af Compare February 13, 2026 12:58
@rustbot

This comment has been minimized.

@rust-bors

This comment has been minimized.

@fneddy fneddy force-pushed the s390x_packed_stack_attribute branch from 0c182af to 7fef6ea Compare February 16, 2026 09:35
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@fneddy
Copy link
Contributor Author

fneddy commented Feb 16, 2026

ok, i think most should be ok now.

@rust-log-analyzer

This comment has been minimized.

@fneddy fneddy force-pushed the s390x_packed_stack_attribute branch from 7fef6ea to 31a40dc Compare February 16, 2026 09:50
if sess.target.arch == Arch::S390x
&& sess.opts.unstable_opts.packed_stack
&& sess.unstable_target_features.contains(&Symbol::intern(&"backchain"))
&& sess.target.abi != Abi::SoftFloat
Copy link
Member

Choose a reason for hiding this comment

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

target.abi is just in charge of cfg(target_abi), it doesn't control the actual ABI. Please check rustc_abi which controls the actual ABI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it to check on the soft-float unstable_target_feature which llvm will evaluate to switch abi for s390x.

Copy link
Member

Choose a reason for hiding this comment

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

I think checking rustc_abi makes most sense.

// this is the first place in the `run_compiler` flow where all this is available
if sess.target.arch == Arch::S390x
&& sess.opts.unstable_opts.packed_stack
&& sess.unstable_target_features.contains(&Symbol::intern(&"backchain"))
Copy link
Member

Choose a reason for hiding this comment

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

What happens if someone tries to enable backchain just for a particular function via #[target_feature] instead of -Ctarget-feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ufff yes we have to check this for every call to llfn_attrs_from_instance at compiler runtime. is it ok to do ....contains(&Symbol::intern(... every time?

Copy link
Member

Choose a reason for hiding this comment

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

from_target_feature_attr is probably a better, higher-level place to check this?

You can add backchain as a pre-interned symbol in compiler/rustc_span/src/symbol.rs so you don't have to re-intern it every time.

Copy link
Contributor Author

@fneddy fneddy Feb 24, 2026

Choose a reason for hiding this comment

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

I am back on working on this. yes in from_target_feature_attr we can check for the #[target_feature(...)] attributes. BUT unfortunately not for the -Ctarget-feature... attributes ..

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the handling for -Ctarget-feature and #[target_feature] is entirely separate. Maybe it's possible to introduce a new helper that gets called form those 2 places and that performs these kinds of consistency checks?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see you're now doing the check in packed_stack_attr. That also seems reasonable.

@rust-log-analyzer

This comment has been minimized.

@fneddy fneddy force-pushed the s390x_packed_stack_attribute branch from d886fbf to f39c52b Compare February 16, 2026 14:18
@fneddy fneddy force-pushed the s390x_packed_stack_attribute branch from f39c52b to dc98326 Compare February 16, 2026 14:19
@chenyukang
Copy link
Member

looks good for me, but it's better to be reviewed by someone with related domain knowledge.
@rustbot reroll

@rustbot rustbot assigned wesleywiser and unassigned chenyukang Feb 24, 2026
@rust-log-analyzer

This comment has been minimized.

@fneddy fneddy force-pushed the s390x_packed_stack_attribute branch 2 times, most recently from c490b46 to ebb7f42 Compare February 24, 2026 14:34
@fneddy
Copy link
Contributor Author

fneddy commented Feb 24, 2026

ok now this is something. i think i added a good check for all the different possibilities of the attribute with a check here and test here.

@rust-log-analyzer

This comment has been minimized.

@fneddy
Copy link
Contributor Author

fneddy commented Feb 24, 2026

hold on I messed up that test with the last commit. I'll fix that tomorrow morning.

this enables packed-stack just as -mpacked-stack in clang and gcc.
packed-stack is needed on s390x for kernel development.

Co-authored-by: Ralf Jung <post@ralfj.de>
@fneddy fneddy force-pushed the s390x_packed_stack_attribute branch from ebb7f42 to 6753aec Compare February 25, 2026 08:22
@fneddy
Copy link
Contributor Author

fneddy commented Feb 25, 2026

ok I think its ready now for next review round.

@rustbot ready

return None;
}

// The backchain and softfloat flags can be set via -Ctarget-features=...
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest an early return on !sess.opts.unstable_opts.packed_stack to avoid doing any of this work if we don't have to.

removed comment that was not needed in the end
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-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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants