add rustc option -Zpacked-stack#152432
Conversation
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I'll add tests for assembly output and for target incompatibility. |
This comment has been minimized.
This comment has been minimized.
5ddcabb to
4ebec9d
Compare
This comment has been minimized.
This comment has been minimized.
0d6d167 to
0c182af
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0c182af to
7fef6ea
Compare
This comment has been minimized.
This comment has been minimized.
|
ok, i think most should be ok now. |
This comment has been minimized.
This comment has been minimized.
7fef6ea to
31a40dc
Compare
compiler/rustc_interface/src/util.rs
Outdated
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
changed it to check on the soft-float unstable_target_feature which llvm will evaluate to switch abi for s390x.
There was a problem hiding this comment.
I think checking rustc_abi makes most sense.
compiler/rustc_interface/src/util.rs
Outdated
| // 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")) |
There was a problem hiding this comment.
What happens if someone tries to enable backchain just for a particular function via #[target_feature] instead of -Ctarget-feature?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ..
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah I see you're now doing the check in packed_stack_attr. That also seems reasonable.
This comment has been minimized.
This comment has been minimized.
d886fbf to
f39c52b
Compare
e9e5f4c to
fd39c98
Compare
This comment has been minimized.
This comment has been minimized.
|
Kind reminder this PR exists and breaks every now and then due to |
|
r? @nikic |
This comment has been minimized.
This comment has been minimized.
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>
fd39c98 to
f39fa9e
Compare
|
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. |
|
@rustbot ready |
|
The interaction with LLVM looks fine. A question I have is whether it is normal to emit errors during codegen attribute lowering? Do I understand correctly that if |
Yeah it's a hack to deal with a somewhat ill-behaved target. Can you think of a better way of dealing with this? |
Not having |
Options:disallow
|
s390x has a frame pointer, which is used for accessing local variables and stack slots in functions using variable-sized stack allocation - the main purpose of frame pointers everywhere. On some other platforms (in particular x86 and arm), the frame pointer can also be used for stack unwinding, but that does not work on s390x, because our frame pointer indicates the bottom of the stack frame (like the stack pointer), not the top of the stack frame. The main reason for this is that some instructions do not support negative displacements, so a pointer to the top of the stack frame would be awkward to use. Because the frame pointer is not usable for stack unwinding, there's no point in trying to use something like |
|
Not sure whether the per-function notion is a particular problem, but this aspect isn't really important - users typically enable backchain for the whole compilation unit. The fact that this is modeled in LLVM as a "target feature" is really more of an implementation detail. If this simplifies the Rust logic, I'd also be fine with only supporting a command line option like The one important use case is the Linux kernel, which is fully built with backchain, packed-stack, and soft-float. This combination must work (and be active for the kernel target). |
Yeah this is currently not a thing. Conceptually it's not that hard to add, but doing it in a way that the target feature table does not become a lot more verbose could be tricky. This is all still unstable obviously so we could also just leave a note on the backchain tracking issue that this should probably not be stabilized for per-function use, only for |
I think in practice, "I want a frame pointer" really means "I want non-DWARF unwind", which is why I consider frame pointers and backchain to be the same thing, even if they technically aren't... But I guess the ship here has sailed in terms of option naming. |
|
@bors r+ As everything involved here is unstable anyway, I think this is fine to land as-is... |
Rollup merge of #152432 - fneddy:s390x_packed_stack_attribute, r=nikic add rustc option -Zpacked-stack 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 > [...]
…uwer Rollup of 4 pull requests Successful merges: - rust-lang/rust#150752 (Update libc to v0.2.183) - rust-lang/rust#152432 (add rustc option -Zpacked-stack) - rust-lang/rust#154634 (Use `Hcx`/`hcx` consistently for `StableHashingContext`.) - rust-lang/rust#154635 (./x run miri: default to edition 2021)
View all comments
this enables
-Zpacked-stackjust as-mpacked-stackin 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)