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.

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

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

Merged
merged 2 commits into from
Feb 29, 2020

Conversation

anyska
Copy link
Contributor

@anyska anyska commented Feb 18, 2020

Ref #64259 (this is a simpler alternative to that). See also #64259 (comment).

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2020
@anyska anyska force-pushed the blacklist-powerpc-zst branch from e1a8c31 to b030280 Compare February 18, 2020 15:36
@davidtwco

This comment has been minimized.

@rust-highfive rust-highfive assigned eddyb and unassigned davidtwco Feb 18, 2020
@eddyb
Copy link
Member

eddyb commented Feb 18, 2020

r? @nikomatsakis or @pnkfelix

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Feb 18, 2020
@smaeul
Copy link
Contributor

smaeul commented Feb 23, 2020

This does not close #64259, as it does not solve the original issue. I found the bug on powerpc-unknown-linux-musl, and I would like it to be fixed on that target. As I mentioned multiple times in comments on the other PR, the ABI is determined by the compiler, not by libc. At least on Linux, target_env only denotes the libc in use, so it is irrelevant to this issue.

A sparc64 port of musl is in progress, and this PR won't fix that target either.

So while this PR does slightly improve the situation, #64259 (or a rebased version of it) still needs to be open to fix the bug on non-glibc targets that use the same ABI: Linux/musl, NetBSD, and OpenBSD.

@anyska anyska changed the title Blacklist powerpc-unknown-linux-gnu as having non-ignored GNU C ZSTs. Blacklist powerpc-unknown-linux-{gnu,musl} as having non-ignored GNU C ZSTs. Feb 24, 2020
@nikomatsakis
Copy link
Contributor

@smaeul would your preference be to close this PR in favor of #64259?

@eddyb
Copy link
Member

eddyb commented Feb 25, 2020

For the record, this PR was updated to include powerpc-unknown-linux-musl.
@anyska could take out the "Closes #64259" from the description.

Also, it might be more productive to open an issue about generalizing our blacklisting here to include all affected targets (we need a predicate for "platform C compiler supports GNU extensions" which might be "everything except MSVC" for all supported targets).

We should probably also have linted against passing ZSTs by value, since AFAICT it's not even compatible between empty structs defined in C vs C++, and there may be other gotchas too.
The only common uses of by-value ZSTs in FFI should be returning () or !.
I know @joshtriplett brought up #ifdef'd out fields, but I wonder how many of those cases are by-value or by-pointer (as I recently realized on another issue most C APIs I've seen almost always put structs behind pointers).

Oh also do we even handle returning ZSTs correctly, on the affected platforms? (EDIT: oh, that's #65111 (comment))

@nikomatsakis
Copy link
Contributor

The truth is I don't really feel like the right person to review this -- @eddyb maybe you?

@eddyb
Copy link
Member

eddyb commented Feb 25, 2020

The truth is I don't really feel like the right person to review this -- @eddyb maybe you?

I suggested this approach, so I'd rather not unilaterally approve it, maybe @nagisa or @hanna-kruppe?

@smaeul
Copy link
Contributor

smaeul commented Feb 26, 2020

Thanks for updating the PR to include -musl targets.

I'm fine with going with this PR, and closing #64259 in the process, as long as there is an issue (maybe #65111?) to track fixing it "for real" at some point.

@nagisa
Copy link
Member

nagisa commented Feb 26, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Feb 26, 2020

📌 Commit 162d727 has been approved by nagisa

@bors 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 Feb 26, 2020
@nagisa
Copy link
Member

nagisa commented Feb 26, 2020

Edited the original comment to remove Closes.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 28, 2020
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).
@bors
Copy link
Contributor

bors commented Feb 29, 2020

⌛ Testing commit 162d727 with merge 4f0edbd...

@bors
Copy link
Contributor

bors commented Feb 29, 2020

☀️ Test successful - checks-azure
Approved by: nagisa
Pushing 4f0edbd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 29, 2020
@bors bors merged commit 4f0edbd into rust-lang:master Feb 29, 2020
@anyska anyska deleted the blacklist-powerpc-zst branch March 4, 2020 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants