PowerPC C ZST ABI fixes #64259

smaeul · 2019-09-07T15:41:26Z

These patches fix some longstanding issues with the PowerPC C ABI that were causing several test failures. With these patches, I get a clean testsuite run on powerpc-unknown-linux-musl.

The PowerPC SVR4 ELF psABI specifies that, for parameter passing:

A struct, union, or long double, any of which shall be treated as
a pointer to the object, or to a copy of the object where necessary to
enforce call-by-value semantics. Only if the caller can ascertain that the
object is "constant" can it pass a pointer to the object itself.

The first commit fixes this for most aggregates; the second commit fixes this for zero-size aggregates.

rust-highfive · 2019-09-07T15:41:29Z

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

matthewjasper · 2019-09-09T13:56:26Z

r? @nagisa

Alexendoo · 2019-09-18T19:41:57Z

Ping from triage, any updates? @nagisa

hdhoang · 2019-09-27T01:42:56Z

from triaging, requesting a different reviewer. ping @oli-obk can you take a look?

nagisa · 2019-09-27T02:47:23Z

@bors r+

Sorry for taking so long to look at this

bors · 2019-09-27T02:47:25Z

📌 Commit 0f0d17295d7d706cd0430b86965b4fc98886f92a has been approved by nagisa

eddyb · 2019-09-27T10:33:19Z

src/librustc/ty/layout.rs

-            target.target_os == "linux" && target.arch == "sparc64" && target.target_env == "gnu";
+        let indirect_zst = match target.arch.as_ref() {
+            "powerpc" | "s390x" | "sparc64" => true,
+            "x86_64" => target.target_os == "windows" && target.target_env == "gnu",


One day I want to figure out how this happened.

At least for mingw it somewhat makes sense - the win64 ABI does something unusual by special-casing only powers of 2 (and 0 isn't a power of 2), and the interaction with the GNU extension allowing ZSTs was likely accidentally overlooked.

But what's up with the other architectures? Why do they care about ZSTs at all? Were these all bugs in the GCC support?

@eddyb The spec and the code driving this are quite simple: aggregates are passed by reference, full stop. There is no exception based on the size of the aggregate. Or in other words, they don't care about ZSTs, while rust does.

The PowerPC SVR4 psABI is linked above, and the gcc code driving this is rs6000_pass_by_reference() (with the if (DEFAULT_ABI == ABI_V4 && AGGREGATE_TYPE_P (type)) condition on line 12254).

Or in other words, they don't care about ZSTs, while rust does.

AFAIK they don't mention ZSTs because ZSTs are not in the C standard, so the GCC implementation is a defacto standard for the ZST GNU extension.

What I think happened was that:

GCC added this ZST extension

GCC didn't bypass target-specific logic for ZSTs (whereas we do by default)

new targets were added to GCC without anyone involved bringing up the ZST extension

Here are a couple architectures: https://godbolt.org/z/yOCJ-z - you can change the compilers involved to other architectures to check what they do, in that example MIPS64 is correct while MSP430 has the bug.

We might even support MSP430 without being aware of the bug - this is becoming quite sad, IMO.

That's because aggregates are passed in registers on MIPS/MIPS64/PowerPC64 (so the ZST takes up zero registers), while aggregates are passed indirectly on PowerPC/MSP (so the pointer to the ZST takes up one register, just like any other pointer). You need a caller to see the full picture.: https://godbolt.org/z/iMnRe-

eddyb · 2019-09-27T10:36:54Z

src/librustc/ty/layout.rs

-                if is_return || rust_abi || (!win_x64_gnu && !linux_s390x && !linux_sparc64) {
+                // FIXME: The C ABI case should be handled in adjust_for_cabi.
+                // Zero-sized struct arguments cannot be ignored in the C ABI
+                // if they are passed indirectly.


This makes it sound like it's normal to a pass a pointer to nothing, and not likely some historical accident 😞.

(To be clear, I'm not asking for a change here, my wording was probably too strong anyway)

The contents of the pointer don't really matter (the callee will never dereference it, although C++ guarantees that if you take the address of a ZST it will be unique). The important part is that we reserve a register or stack slot for the pointer, since the spec guarantees that there will be a pointer provided. Otherwise (and this was what was causing test failures), all arguments after the ZST will be misaligned (i.e. in the wrong register) from what the C compiler expects.

The spec doesn't apply to ZSTs because the C standard bans ZSTs (see my other comment for more details).
I don't think we need to read intent into what is pretty clearly a historical accident, given that x64/ARM/MIPS all do the right thing while some less-common targets don't.

eddyb · 2019-09-27T15:24:45Z

src/librustc/ty/layout.rs

-        let linux_sparc64 =
-            target.target_os == "linux" && target.arch == "sparc64" && target.target_env == "gnu";
+        let indirect_zst = match target.arch.as_ref() {
+            "powerpc" | "s390x" | "sparc64" => true,


Wait, you've removed the target_env == "gnu" - this workaround only makes sense in that case, because ZSTs in C are a GNU extension.

@eddyb That's not true. It makes equally as much sense for target_env == "musl", which follows the exact same ELF ABI as glibc. As I mentioned before:

With these patches, I get a clean testsuite run on powerpc-unknown-linux-musl.

It is also necessary for the BSDs, since again they use the same ABI (both as specified, and as implemented in gcc/clang).

You could probably add a has_gnu_extensions or something that's set for target_env being either "gnu" or "musl".

While this doesn't feel ideal, I'd certainly prefer to broaden the whitelist than start including combinations we might not be aware of, and it does make sense that musl would hit the same problems.

That still doesn't make any sense to me. target_env refers to the libc in use: glibc versus musl. That has nothing to do with the ABI, which is a property of the compiler.

Like I already said, this fix applies for the BSDs as well as Linux (regardless of target_env), because they all use GCC or a GCC-compatible compiler.

eddyb · 2019-09-27T15:25:29Z

@bors r- #64259 (comment)

bors · 2019-09-29T09:59:04Z

☔ The latest upstream changes (presumably #64886) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2019-10-05T19:04:20Z

Ping from triage.
@nagisa This PR has sat idle for the last few days. Can you please address the merge conflict?
CC @eddyb
Thank you!

smaeul · 2019-10-05T19:09:57Z

I've rebased this on master.

nagisa · 2019-10-05T19:48:43Z

r? @eddyb

JohnCSimon · 2019-10-12T03:38:31Z

Ping from triage. This seems to be waiting on review now.
@eddyb can you please review?

cc: @smaeul

Thanks!

wirelessringo · 2019-10-18T21:37:32Z

Ping from triage. @eddyb any updates on this? Thanks.

eddyb · 2019-10-23T15:30:14Z

This PR has been waiting on this comment, not me: #64259 (comment).

glaubitz · 2019-11-08T15:54:31Z

I think I actually ran into this problem and I understand what went wrong. This also explains the reluctance on the Rust side regarding this change.

q66 · 2019-11-08T15:57:25Z

It shouldn't be problematic for Rust itself, I'm fairly sure they cross compile all of their tier 2+ targets anyway - so once it's merged and a release is made with it, all of the Mozilla-released tarballs should be suitable for bootstrapping from scratch...

awilfox · 2019-11-08T21:14:37Z

Adélie does have a PPC32 port which uses the SVR4 ABI, with an install base (at least 50 at my count), and most of these people do enjoy having Firefox, which builds with Rust, which means Rust needs to be fixed.

To clarify, yes, we also have a PPC64BE port. This uses ELFv2, and works on 970 and up (G5, P5+). This has an even higher install base (a few hundred at my count). It's used in production for virtually all adelielinux.org services. We've worked with major projects across the ecosystem to ensure ELFv1/ELFv2 is a separate concern from BE/LE. Upstreams use auxv to determine VSX and VMX support so the silly requirement that ELFv2 is "only for POWER8" is irrelevant. Especially with things like microwatt and the opening of OpenPOWER, the ELFv2 ABI likely should be changed to note that auxv should be used instead of assuming an ISA level. But that's all irrelevant, I guess.

Please do merge this upstream so we can stop carrying it. It is still useful, whether or not chips are "being produced". Wii/Wii U, Freescale chips, Apple G3/G4, there are millions of devices right now that use ppc32. See ibmruntimes/node#30 for another instance of ppc32 being neglected despite users desiring support.

glaubitz · 2019-11-08T22:36:22Z

@awilfox Thanks for your comment. I agree with everything what you said and I highly appreciate the work you guys are doing on the PowerPC code. We profit from these fixes in openSUSE and Debian as well!

Fix C aggregate-passing ABI on powerpc The existing code (which looks like it was copied from MIPS) passes aggregates by value in registers. This is wrong. According to the SVR4 powerpc psABI, all aggregates are passed indirectly. See rust-lang#64259 for more discussion, which addresses the ABI for the special case of ZSTs (empty structs).

JohnCSimon · 2019-11-09T08:19:08Z

Ping from triage:
@smaeul - Can you please address the merge conflict?
cc: @eddyb

Thanks!

For targets that pass zero-sized aggregates indirectly (generally those that pass all aggregates indirectly), we must allocate a register for passing the address of the ZST. Clean up the existing cases and add powerpc, which requires this as well. While there are not currently musl targets for s390x or sparc64, they would have the same ABI as gnu targets, so remove the env == "gnu" check in the Linux case. Ideally, since it is a property of the C ABI, the `!rust_abi` case would be handled entirely in `adjust_c_abi`. However, that would require updating each implementation of `compute_abi_info` to handle ZSTs.

JohnCSimon · 2019-11-17T00:30:21Z

Ping from triage
@eddyb can you please review this? Thank you

Dylan-DPC-zz · 2019-11-28T12:37:44Z

This is being discussed on zulip so there is progress

pnkfelix · 2019-11-28T14:46:52Z

This is being discussed on zulip so there is progress

To be honest I'm not aware of discussion beyond that shown in https://zulip-archive.rust-lang.org/131828tcompiler/69822PowerPCCABIfixesPR64259.html, where (as of now) the most recent comment was posted on November 7th.

@Dylan-DPC did discussion continue after that in some other Zulip topic?

(It would be good to try to get unblocked here...)

Dylan-DPC-zz · 2019-11-28T20:50:24Z

Ah I probably misread it thinking it's latest. Sorry for that

pnkfelix · 2020-01-16T15:26:51Z

swiping from @eddyb to move this forward

glaubitz · 2020-01-16T15:28:47Z

Is there actually stuff left to merge? The ABI fixes that were shipped with 1.40 actually reduced the number of testsuite failures for us on Debian powerpc dramatically.

We're now down to only 23 failures, from over 60.

nikomatsakis · 2020-02-13T15:29:35Z

We discussed this in our compiler team triage meeting today. We'd very much like to get some version of these changes landed. @eddyb suggested there is a small (approx. 1 line) diff that would fix the immediate/known problems that they'd prefer, and there was general consensus that a small, conservative step would be a good place to start. Hopefully @eddyb can either comment here (and we can update this PR) or it may be easier for them to just open a PR with the diff.

@smaeul, I do wish to thank you both for the initial PR and for your patience!

pnkfelix · 2020-02-27T15:50:27Z

Discussed in T-compiler meeting. Closing under assumption that remaining desired work will be covered by PR's like #69263

Blacklist powerpc-unknown-linux-{gnu,musl} as having non-ignored GNU C ZSTs. Ref rust-lang#64259 (this is a simpler alternative to that). See also rust-lang#64259 (comment).

Blacklist powerpc-unknown-linux-{gnu,musl} as having non-ignored GNU C ZSTs. Ref #64259 (this is a simpler alternative to that). See also #64259 (comment).

glaubitz · 2020-04-09T11:42:16Z

This has reduced the number of testsuite failures on Debian/powerpc down to 10.

On the other side, it's currently not possible to bootstrap rustc on openSUSE using the binaries provided by the Rust website. It seems that the Rust binaries for powerpc are still built with a toolchain without these fixes, isn't it?

mati865 · 2020-04-09T11:56:02Z

@glaubitz #69263 is the beta but hasn't reached stable yet.

glaubitz · 2020-04-10T14:17:01Z

@mati865 I'm not sure whether that's related, is it?

And more surprisingly, the number of testsuite failures went up to 51 with 1.42.0 again.

mati865 · 2020-04-10T14:41:52Z

@glaubitz this PR has been closed under assumption #69263 is enough to fix PPC issue.

rust-highfive assigned matthewjasper Sep 7, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2019

rust-highfive assigned nagisa and unassigned matthewjasper Sep 9, 2019

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2019

eddyb reviewed Sep 27, 2019

View reviewed changes

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 27, 2019

smaeul force-pushed the patch/powerpc-abi branch from 0f0d172 to 45b7579 Compare October 5, 2019 19:08

rust-highfive assigned eddyb and unassigned nagisa Oct 5, 2019

JohnCSimon 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 Oct 12, 2019

hanna-kruppe mentioned this pull request Nov 8, 2019

Types equal to/wrapping () result in "not FFI-safe" warning #66202

Closed

smaeul force-pushed the patch/powerpc-abi branch from 45b7579 to 308071a Compare November 11, 2019 05:34

smaeul changed the title ~~PowerPC C ABI fixes~~ PowerPC C ZST ABI fixes Nov 17, 2019

pnkfelix self-assigned this Jan 16, 2020

anyska mentioned this pull request Feb 18, 2020

Blacklist powerpc-unknown-linux-{gnu,musl} as having non-ignored GNU C ZSTs. #69263

Merged

pnkfelix closed this Feb 27, 2020

pnkfelix removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Feb 27, 2020

bors added a commit that referenced this pull request Feb 29, 2020

Auto merge of #69263 - anyska:blacklist-powerpc-zst, r=nagisa

4f0edbd

Blacklist powerpc-unknown-linux-{gnu,musl} as having non-ignored GNU C ZSTs. Ref #64259 (this is a simpler alternative to that). See also #64259 (comment).

workingjubilee added the O-PowerPC Target: PowerPC processors label Feb 5, 2023

PowerPC C ZST ABI fixes #64259

PowerPC C ZST ABI fixes #64259

Conversation

smaeul commented Sep 7, 2019

rust-highfive commented Sep 7, 2019

matthewjasper commented Sep 9, 2019

Alexendoo commented Sep 18, 2019

hdhoang commented Sep 27, 2019

nagisa commented Sep 27, 2019

bors commented Sep 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaeul Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaeul Sep 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Sep 27, 2019

bors commented Sep 29, 2019

JohnCSimon commented Oct 5, 2019

smaeul commented Oct 5, 2019

nagisa commented Oct 5, 2019

JohnCSimon commented Oct 12, 2019 • edited Loading

wirelessringo commented Oct 18, 2019

eddyb commented Oct 23, 2019

glaubitz commented Nov 8, 2019

q66 commented Nov 8, 2019

awilfox commented Nov 8, 2019

glaubitz commented Nov 8, 2019

JohnCSimon commented Nov 9, 2019

JohnCSimon commented Nov 17, 2019

Dylan-DPC-zz commented Nov 28, 2019

pnkfelix commented Nov 28, 2019

Dylan-DPC-zz commented Nov 28, 2019

pnkfelix commented Jan 16, 2020

glaubitz commented Jan 16, 2020

nikomatsakis commented Feb 13, 2020

pnkfelix commented Feb 27, 2020 • edited Loading

glaubitz commented Apr 9, 2020

mati865 commented Apr 9, 2020

glaubitz commented Apr 10, 2020

mati865 commented Apr 10, 2020

smaeul Sep 27, 2019 •

edited

Loading

smaeul Sep 28, 2019 •

edited

Loading

JohnCSimon commented Oct 12, 2019 •

edited

Loading

pnkfelix commented Feb 27, 2020 •

edited

Loading