Resurrect: rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of byval on x86 in the process. #111551
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
erikdesjardins
commented
May 14, 2023
nikic
reviewed
May 14, 2023
nikic
reviewed
May 14, 2023
erikdesjardins
force-pushed
the
align
branch
2 times, most recently
from
89793d4
to
cbd8f2a
Compare
|
@bors r+ rollup=never |
|
📌 Commit cbd8f2ad9bf82e1a633be5ce2c158fa3480a0e44 has been approved by It is now in the queue for this repository. |
bors
added
S-waiting-on-bors
|
⌛ Testing commit cbd8f2ad9bf82e1a633be5ce2c158fa3480a0e44 with merge efd471c2299c272c2a42764ae6dfb4aed5c033ce... |
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors r+ |
|
📌 Commit d44c17586ff2f3dcf99d0e26d3c321ee7f7cebb9 has been approved by It is now in the queue for this repository. |
bors
added
S-waiting-on-bors
|
⌛ Testing commit d44c17586ff2f3dcf99d0e26d3c321ee7f7cebb9 with merge 4d0ee3b6a07bebe0a7841a08c7f96f9adbe58f6d... |
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Rebased and updated the newly added addr-of-mutate test |
bors
added
S-waiting-on-review
|
Running the test binary just fails with no output? Do assertion failures not output anything on MSVC? Trying to reproduce this in a fresh Windows VM, it segfaults building stage1 core, even on master. I'll try to figure out why... (it's not OOM) Edit: "cross-compiling" from x64 windows to x86 works, so it probably was OOM, of a sort (32 bit address space exhaustion) |
|
Okay, it seems like our handling of the This appears to be our fault and not LLVM's, since Clang gets it right: https://godbolt.org/z/cG35c7TrE Note the difference in the signature of the #[repr(C)]
#[repr(align(16))]
pub struct TwoU64s {
pub a: u64,
pub b: u64,
}
extern "C" {
fn many_args(h: TwoU64s) -> i32;
}generates the following signature, passing by stack (using declare noundef i32 @many_args(ptr noalias nocapture noundef byval(%TwoU64s) dereferenceable(16))while the equivalent C: #ifdef _MSC_VER
__declspec(align(16))
struct TwoU64s
{
uint64_t a;
uint64_t b;
};
#endif
extern int32_t many_args(struct TwoU64s h);generates the following signature, passing by reference: declare i32 @many_args(ptr noundef)Without the alignment attribute (https://godbolt.org/z/v8dc11MEK) declare noundef i32 @many_args(ptr noalias nocapture noundef byval({ i64, i64 }) dereferenceable(16))while Clang passes the struct by value in two LLVM arguments: declare dso_local i32 @many_args(i64, i64)Edit: actually this second example ends up generating equivalent code in both cases (by accident?), since those "by value" arguments get passed on the stack like |
erikdesjardins
commented
May 20, 2023
|
This looks relevant: https://reviews.llvm.org/D72114
|
|
@rustbot author |
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustbot
added
S-waiting-on-review
|
ping @nikic |
|
Pushed the wrong branch. Wish github didn't (permanently) autoclose when there's no diff... |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 15, 2023
Resurrect: rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of byval on x86 in the process. Same as rust-lang#111551, which I [accidentally closed](rust-lang#111551 (comment)) :/ --- This resurrects PR rust-lang#103830, which has sat idle for a while. Beyond rust-lang#103830, this also: - fixes byval alignment for types containing vectors on Darwin (see `tests/codegen/align-byval-vector.rs`) - fixes byval alignment for overaligned types on x86 Windows (see `tests/codegen/align-byval.rs`) - fixes ABI for types with 128bit requested alignment on ARM64 Linux (see `tests/codegen/aarch64-struct-align-128.rs`) r? `@nikic` --- `@pcwalton's` original PR description is reproduced below: Commit 88e4d2c from five years ago removed support for alignment on indirectly-passed arguments because of problems with the `i686-pc-windows-msvc` target. Unfortunately, the `memcpy` optimizations I recently added to LLVM 16 depend on this to forward `memcpy`s. This commit attempts to fix the problems with `byval` parameters on that target and now correctly adds the `align` attribute. The problem is summarized in [this comment] by `@eddyb.` Briefly, 32-bit x86 has special alignment rules for `byval` parameters: for the most part, their alignment is forced to 4. This is not well-documented anywhere but in the Clang source. I looked at the logic in Clang `TargetInfo.cpp` and tried to replicate it here. The relevant methods in that file are `X86_32ABIInfo::getIndirectResult()` and `X86_32ABIInfo::getTypeStackAlignInBytes()`. The `align` parameter attribute for `byval` parameters in LLVM must match the platform ABI, or miscompilations will occur. Note that this doesn't use the approach suggested by eddyb, because I felt it was overkill to store the alignment in `on_stack` when special handling is really only needed for 32-bit x86. As a side effect, this should fix rust-lang#80127, because it will make the `align` parameter attribute for `byval` parameters match the platform ABI on LLVM x86-64. [this comment]: rust-lang#80822 (comment)
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Jul 16, 2023
Resurrect: rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of byval on x86 in the process. Same as #111551, which I [accidentally closed](rust-lang/rust#111551 (comment)) :/ --- This resurrects PR #103830, which has sat idle for a while. Beyond #103830, this also: - fixes byval alignment for types containing vectors on Darwin (see `tests/codegen/align-byval-vector.rs`) - fixes byval alignment for overaligned types on x86 Windows (see `tests/codegen/align-byval.rs`) - fixes ABI for types with 128bit requested alignment on ARM64 Linux (see `tests/codegen/aarch64-struct-align-128.rs`) r? `@nikic` --- `@pcwalton's` original PR description is reproduced below: Commit 88e4d2c from five years ago removed support for alignment on indirectly-passed arguments because of problems with the `i686-pc-windows-msvc` target. Unfortunately, the `memcpy` optimizations I recently added to LLVM 16 depend on this to forward `memcpy`s. This commit attempts to fix the problems with `byval` parameters on that target and now correctly adds the `align` attribute. The problem is summarized in [this comment] by `@eddyb.` Briefly, 32-bit x86 has special alignment rules for `byval` parameters: for the most part, their alignment is forced to 4. This is not well-documented anywhere but in the Clang source. I looked at the logic in Clang `TargetInfo.cpp` and tried to replicate it here. The relevant methods in that file are `X86_32ABIInfo::getIndirectResult()` and `X86_32ABIInfo::getTypeStackAlignInBytes()`. The `align` parameter attribute for `byval` parameters in LLVM must match the platform ABI, or miscompilations will occur. Note that this doesn't use the approach suggested by eddyb, because I felt it was overkill to store the alignment in `on_stack` when special handling is really only needed for 32-bit x86. As a side effect, this should fix #80127, because it will make the `align` parameter attribute for `byval` parameters match the platform ABI on LLVM x86-64. [this comment]: #80822 (comment)
bjorn3
pushed a commit
to rust-lang/rustc_codegen_cranelift
that referenced
this pull request
Jul 22, 2023
Resurrect: rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of byval on x86 in the process. Same as #111551, which I [accidentally closed](rust-lang/rust#111551 (comment)) :/ --- This resurrects PR #103830, which has sat idle for a while. Beyond #103830, this also: - fixes byval alignment for types containing vectors on Darwin (see `tests/codegen/align-byval-vector.rs`) - fixes byval alignment for overaligned types on x86 Windows (see `tests/codegen/align-byval.rs`) - fixes ABI for types with 128bit requested alignment on ARM64 Linux (see `tests/codegen/aarch64-struct-align-128.rs`) r? `@nikic` --- `@pcwalton's` original PR description is reproduced below: Commit 88e4d2c from five years ago removed support for alignment on indirectly-passed arguments because of problems with the `i686-pc-windows-msvc` target. Unfortunately, the `memcpy` optimizations I recently added to LLVM 16 depend on this to forward `memcpy`s. This commit attempts to fix the problems with `byval` parameters on that target and now correctly adds the `align` attribute. The problem is summarized in [this comment] by `@eddyb.` Briefly, 32-bit x86 has special alignment rules for `byval` parameters: for the most part, their alignment is forced to 4. This is not well-documented anywhere but in the Clang source. I looked at the logic in Clang `TargetInfo.cpp` and tried to replicate it here. The relevant methods in that file are `X86_32ABIInfo::getIndirectResult()` and `X86_32ABIInfo::getTypeStackAlignInBytes()`. The `align` parameter attribute for `byval` parameters in LLVM must match the platform ABI, or miscompilations will occur. Note that this doesn't use the approach suggested by eddyb, because I felt it was overkill to store the alignment in `on_stack` when special handling is really only needed for 32-bit x86. As a side effect, this should fix #80127, because it will make the `align` parameter attribute for `byval` parameters match the platform ABI on LLVM x86-64. [this comment]: #80822 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resurrects PR #103830, which has sat idle for a while.
I have applied the test changes requested by @wesleywiser and @nikic here and here in the latest commits.
r? @wesleywiser
@pcwalton's original PR description is reproduced below:
Commit 88e4d2c from five years ago removed
support for alignment on indirectly-passed arguments because of problems with
the
i686-pc-windows-msvctarget. Unfortunately, thememcpyoptimizations Irecently added to LLVM 16 depend on this to forward
memcpys. This commitattempts to fix the problems with
byvalparameters on that target and nowcorrectly adds the
alignattribute.The problem is summarized in this comment by @eddyb. Briefly, 32-bit x86 has
special alignment rules for
byvalparameters: for the most part, theiralignment is forced to 4. This is not well-documented anywhere but in the Clang
source. I looked at the logic in Clang
TargetInfo.cppand tried to replicateit here. The relevant methods in that file are
X86_32ABIInfo::getIndirectResult()andX86_32ABIInfo::getTypeStackAlignInBytes(). Thealignparameter attributefor
byvalparameters in LLVM must match the platform ABI, or miscompilationswill occur. Note that this doesn't use the approach suggested by eddyb, because
I felt it was overkill to store the alignment in
on_stackwhen specialhandling is really only needed for 32-bit x86.
As a side effect, this should fix #80127, because it will make the
alignparameter attribute for
byvalparameters match the platform ABI on LLVMx86-64.