Skip to content

librustc_codegen_llvm: Don't eliminate empty structs in C ABI on linux-sparc64 #57085

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 24, 2018

Conversation

glaubitz
Copy link
Contributor

@glaubitz glaubitz commented Dec 23, 2018

This is in accordance with the SPARC Compliance Definition 2.4.1,
Page 3P-12. It says that structs of up to 8 bytes (which applies
to empty structs as well) are to be passed in one register.

@rust-highfive
Copy link
Contributor

r? @oli-obk

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 23, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Dec 23, 2018

I don't know anything about SPARC and have not been able to find the referenced document by some quick googling

r? @alexcrichton

@glaubitz
Copy link
Contributor Author

Ah, this could be a typo. It could be version "2.4.1" of the SPARC Compliance Definition, not "4.2.1". Let me check.

CC @karcherm

@glaubitz
Copy link
Contributor Author

Yep, it's 2.4.1, not 4.2.1. See: http://sparc.org/technical-documents/specifications/

Let me push a fixed version.

@glaubitz
Copy link
Contributor Author

Quoting page 48 from the PDF (http://sparc.org/wp-content/uploads/2014/01/SCD.2.4.1.pdf.gz):

Structure and Union arguments

Structure or union types up to eight bytes in size are assigned to one parameter array word, and align to eight-byte boundaries.

…x-sparc64

This is in accordance with the SPARC Compliance Definition 2.4.1,
Page 3P-12. It says that structs of up to 8 bytes (which applies
to empty structs as well) are to be passed in one register.
@nagisa
Copy link
Member

nagisa commented Dec 24, 2018

Ultimately what matters here is how clang/gcc behave here. clang crashes when provided with 0-sized arguments and gcc properly allocates the input registers for those, so...

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 24, 2018

📌 Commit 65dabd6 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 Dec 24, 2018
@nagisa
Copy link
Member

nagisa commented Dec 24, 2018

That being said, I believe that linux uses sysv on sparc(64), just like almost everywhere else, which may or may not match the vendor’s documentation at all. I only have the sysv document for 32-bit SPARC around (so can’t say much about 64-bit) and it does not appear to call out a similar requirement on my quick read-through.

Centril added a commit to Centril/rust that referenced this pull request Dec 24, 2018
librustc_codegen_llvm: Don't eliminate empty structs in C ABI on linux-sparc64

This is in accordance with the SPARC Compliance Definition 2.4.1,
Page 3P-12. It says that structs of up to 8 bytes (which applies
to empty structs as well) are to be passed in one register.
bors added a commit that referenced this pull request Dec 24, 2018
Rollup of 10 pull requests

Successful merges:

 - #55470 (box: Add documentation for `From` impls)
 - #56242 (Add missing link in docs)
 - #56944 (bootstrap: Link LLVM as a dylib with ThinLTO)
 - #56978 (Add `std::os::fortanix_sgx` module)
 - #56985 (Allow testing pointers for inboundedness while forbidding dangling pointers)
 - #56986 (rustc: Move jemalloc from rustc_driver to rustc)
 - #57010 (Actually run compiletest tests on CI)
 - #57021 (Enable emission of alignment attrs for pointer params)
 - #57074 (Fix recursion limits)
 - #57085 (librustc_codegen_llvm: Don't eliminate empty structs in C ABI on linux-sparc64)

Failed merges:

r? @ghost
@bors
Copy link
Collaborator

bors commented Dec 24, 2018

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

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 24, 2018
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 24, 2018
@bors bors merged commit 65dabd6 into rust-lang:master Dec 24, 2018
@bors
Copy link
Collaborator

bors commented Dec 24, 2018

⌛ Testing commit 65dabd6 with merge 50f3d6e...

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants