Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PowerPC C ZST ABI fixes #64259

Closed
wants to merge 1 commit into from
Closed

PowerPC C ZST ABI fixes #64259

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions src/librustc/ty/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2642,12 +2642,11 @@ where
};

let target = &cx.tcx().sess.target.target;
let win_x64_gnu =
target.target_os == "windows" && target.arch == "x86_64" && target.target_env == "gnu";
let linux_s390x =
target.target_os == "linux" && target.arch == "s390x" && target.target_env == "gnu";
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,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@smaeul smaeul Sep 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"x86_64" => target.target_os == "windows" && target.target_env == "gnu",
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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-

_ => false,
};
let rust_abi = match sig.abi {
RustIntrinsic | PlatformIntrinsic | Rust | RustCall => true,
_ => false,
Expand Down Expand Up @@ -2709,11 +2708,10 @@ where
let is_return = arg_idx.is_none();
let mut arg = mk_arg_type(ty, arg_idx);
if arg.layout.is_zst() {
// For some forsaken reason, x86_64-pc-windows-gnu
// doesn't ignore zero-sized struct arguments.
// The same is true for s390x-unknown-linux-gnu
// and sparc64-unknown-linux-gnu.
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.
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@smaeul smaeul Sep 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if is_return || rust_abi || !indirect_zst {
arg.mode = PassMode::Ignore;
}
}
Expand Down