Skip to content

Commit

Permalink
Auto merge of rust-lang#112157 - erikdesjardins:align, r=nikic
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
bors committed Jul 15, 2023
2 parents d45c8c3 + 388a6b5 commit 4f16abd
Showing 1 changed file with 1 addition and 8 deletions.
9 changes: 1 addition & 8 deletions src/abi/comments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,7 @@ pub(super) fn add_local_place_comments<'tcx>(
return;
}
let TyAndLayout { ty, layout } = place.layout();
let rustc_target::abi::LayoutS {
size,
align,
abi: _,
variants: _,
fields: _,
largest_niche: _,
} = layout.0.0;
let rustc_target::abi::LayoutS { size, align, .. } = layout.0.0;

let (kind, extra) = place.debug_comment();

Expand Down

0 comments on commit 4f16abd

Please sign in to comment.