Skip to content
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

LLVM data layout for i128 doesn't match Clang's alignment of __int128_t on 64-bit PowerPC, 64-bit SPARC and 64-bit MIPS #102783

Closed
3 tasks done
beetrees opened this issue Aug 11, 2024 · 10 comments

Comments

@beetrees
Copy link
Contributor

beetrees commented Aug 11, 2024

On 64-bit PowerPC, 64-bit SPARC and 64-bit MIPS, Clang gives __int128_t an alignment of 16 whereas the LLVM data layout (implicitly) gives i128 an alignment of 8. This means that LLVM-based compilers that use the LLVM data layout alignment directly (such as rustc) are ABI incompatible with Clang itself. This is the same as a previous bug on x86-64 that was fixed in D86310, just on different targets. As Clang's behaviour matches GCC (and the PowerPC64 ABI specification) the LLVM data layout is what needs to be updated.

Affected architectures:

@github-actions github-actions bot added the clang Clang issues not falling into any other category label Aug 11, 2024
@EugeneZelenko EugeneZelenko added backend:MIPS backend:PowerPC backend:Sparc ABI Application Binary Interface and removed clang Clang issues not falling into any other category labels Aug 11, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 11, 2024

@llvm/issue-subscribers-backend-powerpc

Author: None (beetrees)

On 64-bit PowerPC, 64-bit SPARC and 64-bit MIPS, Clang gives `__int128_t` an alignment of 16 whereas the LLVM data layout (implicitly) gives `i128` an alignment of 8. This means that LLVM-based compilers that use the LLVM data layout alignment directly (such as `rustc`) are ABI incompatible with Clang itself. This is the same as a previous bug on x86-64 that was fixed in [D86310](https://reviews.llvm.org/D86310), just on different targets. As Clang's behaviour matches GCC (and the PowerPC64 ABI specification) the LLVM data layout is what needs to be updated.

@llvmbot
Copy link
Member

llvmbot commented Aug 11, 2024

@llvm/issue-subscribers-backend-mips

Author: None (beetrees)

On 64-bit PowerPC, 64-bit SPARC and 64-bit MIPS, Clang gives `__int128_t` an alignment of 16 whereas the LLVM data layout (implicitly) gives `i128` an alignment of 8. This means that LLVM-based compilers that use the LLVM data layout alignment directly (such as `rustc`) are ABI incompatible with Clang itself. This is the same as a previous bug on x86-64 that was fixed in [D86310](https://reviews.llvm.org/D86310), just on different targets. As Clang's behaviour matches GCC (and the PowerPC64 ABI specification) the LLVM data layout is what needs to be updated.

koachan added a commit to koachan/llvm-project that referenced this issue Sep 2, 2024
Align i128s to 16 bytes, following the example at https://reviews.llvm.org/D86310.

clang already does this, but do it in backend code too for the benefit of other
frontends (see e.g llvm#102783).
@brad0
Copy link
Contributor

brad0 commented Sep 3, 2024

cc @wzssyqa

@brad0
Copy link
Contributor

brad0 commented Sep 3, 2024

cc @chenzheng1030

koachan added a commit that referenced this issue Sep 30, 2024
Align i128s to 16 bytes, following the example at
https://reviews.llvm.org/D86310.

clang already does this implicitly, but do it in backend code too for
the benefit of other frontends (see e.g
#102783 &
rust-lang/rust#128950).
puja2196 pushed a commit to puja2196/LLVM-tutorial that referenced this issue Sep 30, 2024
Align i128s to 16 bytes, following the example at
https://reviews.llvm.org/D86310.

clang already does this implicitly, but do it in backend code too for
the benefit of other frontends (see e.g
llvm/llvm-project#102783 &
rust-lang/rust#128950).
puja2196 pushed a commit to puja2196/LLVM-tutorial that referenced this issue Oct 2, 2024
Align i128s to 16 bytes, following the example at
https://reviews.llvm.org/D86310.

clang already does this implicitly, but do it in backend code too for
the benefit of other frontends (see e.g
llvm/llvm-project#102783 &
rust-lang/rust#128950).
@brad0
Copy link
Contributor

brad0 commented Oct 3, 2024

@chenzheng1030

@brad0
Copy link
Contributor

brad0 commented Oct 6, 2024

cc @yingopq

yingopq added a commit to yingopq/llvm-project that referenced this issue Oct 12, 2024
yingopq added a commit to yingopq/llvm-project that referenced this issue Oct 12, 2024
yingopq added a commit to yingopq/llvm-project that referenced this issue Nov 6, 2024
yingopq added a commit to yingopq/llvm-project that referenced this issue Nov 6, 2024
@brad0
Copy link
Contributor

brad0 commented Nov 10, 2024

@chenzheng1030 The Power backend is the last left to be fixed.

@chenzheng1030
Copy link
Collaborator

@chenzheng1030 The Power backend is the last left to be fixed.

@lei137 @amy-kwan Hi, would you please help to look at this issue? There are some good fix examples in the issue description for other targets. Thanks.

@brad0
Copy link
Contributor

brad0 commented Nov 25, 2024

@lei137 @amy-kwan Ping.

@brad0
Copy link
Contributor

brad0 commented Dec 9, 2024

@beetrees AFAIK you should be able to close this.

tgross35 added a commit to tgross35/rust that referenced this issue Feb 20, 2025
Rust's 128-bit integers have historically been incompatible with C [1].
However, there have been a number of changes in Rust and LLVM that
mean this is no longer the case:

* Incorrect alignment of `i128` on x86 [1]: adjusting Rust's alignment
  proposed at rust-lang/compiler-team#683,
  implemented at rust-lang#116672.
* LLVM version of the above: resolved in LLVM, including ABI fix.
  Present in LLVM18 (our minimum supported version).
* Incorrect alignment of `i128` on 64-bit PowerPC, SPARC, and MIPS [2]:
  Rust's data layouts adjusted at
  rust-lang#132422,
  rust-lang#132741,
  rust-lang#134115.
* LLVM version of the above: done in LLVM 20
  llvm/llvm-project#102783.
* Incorrect return convention of `i128` on Windows: adjusted to match
  GCC and Clang at rust-lang#134290.

At [3], the lang team considered it acceptable to remove `i128` from
`improper_ctypes_definitions` if the LLVM version is known to be
compatible. Time has elapsed since then and we have dropped support for
LLVM versions that do not have the x86 fixes, meaning a per-llvm-version
lint should no longer be necessary. The PowerPC, SPARC, and MIPS changes
only came in LLVM 20 but since Rust's datalayouts have also been updated
to match, we will be using the correct alignment regardless of LLVM
version.

Closes: rust-lang#134288
Closes: rust-lang#128950

[1]: rust-lang#54341
[2]: rust-lang#128950
[3]: rust-lang/lang-team#255 (comment)
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

5 participants