Set `attrs.pointee_align` when constructing function ABI by Aaron1011 · Pull Request #80822 · rust-lang/rust

Aaron1011 · 2021-01-08T20:21:57Z

Fixes #80127

This will require additional testing to see if the old FIXME still
applies

Fixes rust-lang#80127 This will require additional testing to see if the old FIXME still applies

rust-highfive · 2021-01-08T20:22:01Z

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

Aaron1011 · 2021-01-08T20:22:13Z

cc @eddyb

nagisa · 2021-01-08T23:26:56Z

Originally FIXME was added as part of #45225 in 88e4d2c.

nagisa · 2021-01-10T00:41:00Z

I bootstrapped the compiler with these changes with build=i686-pc-windows-msvc. The following two tests failed:

ui/abi/extern/extern-pass-TwoU64.rs; and
ui/abi/struct-enums/struct-return.rs.

Both failures look a lot like wrong offsets, e.g. the former test asserts receiving TwoU64s { one: 94501929412, two: 98784247808 } instead of TwoU64s { one: 22, two: 23 }.

(NB: test names or the numbers may have errors because I typed them over by hand)

bors · 2021-01-24T06:10:17Z

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

camelid · 2021-02-12T04:33:28Z

Ping from triage: @Aaron1011 could you fix the merge conflicts?

Dylan-DPC-zz · 2021-03-02T01:24:09Z

@Aaron1011 any updates?

crlf0710 · 2021-04-23T13:46:55Z

@Aaron1011 any updates here?

oli-obk · 2021-04-23T13:53:27Z

r? @nagisa

eddyb · 2021-04-30T10:00:39Z

@Aaron1011 @nagisa oh god I completely missed something when I first wrote this code.

So what I thought we'd get is better optimizations, because of the assumption of alignment.

But LLVM must be using this align to align byval stack entries, too!

Can you try making make_indirect_byval unset pointee_align? Also might need to check any other accesses to on_stack elsewhere.

~~This aspect could also mean there might be padding in places implemented less efficiently than just setting align.~~

Actually, scratch that last part, it doesn't make sense because of the semantics of pointee_align - it's kind of silly of LLVM to implement stack-passing this way, and so we should probably also make sure to never emit the align attribute from pointee_align when we emit byval because of on_stack - if there's an align value for byval it should be as part of on_stack and not separate.

Which means that fixing my FIXME is not enough for #80127, now that I look closer. x86_64 does use byval:

rust/compiler/rustc_target/src/abi/call/x86_64.rs

Line 216 in bcd696d

arg.make_indirect_byval();

So what probably needs to happen is what I saying above about making on_stack hold an alignment. And then someone would have to go through every use of byval (make_indirect_byval or otherwise) and see what Clang does in that case for alignment.

@eddyb

…ecting the alignment of `byval` on x86 in the process. 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)

rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of `byval` on x86 in the process. 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)

@eddyb

…ecting the alignment of `byval` on x86 in the process. 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)

Set attrs.pointee_align when constructing function ABI

0bb51f8

Fixes rust-lang#80127 This will require additional testing to see if the old FIXME still applies

rust-highfive assigned oli-obk Jan 8, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 8, 2021

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 12, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 4, 2021

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2021

rust-highfive assigned nagisa and unassigned oli-obk Apr 23, 2021

eddyb mentioned this pull request Apr 30, 2021

rustc has wrong signature for C function with 16-byte aligned stack argument in x86_64 Linux #80127

Closed

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2021

Dylan-DPC closed this Mar 29, 2022

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 29, 2022

BGR360 mentioned this pull request Nov 1, 2022

rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of byval on x86 in the process. #103830

Closed

RalfJung mentioned this pull request Mar 9, 2024

Stop using LLVM struct types for byval/sret #122050

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set `attrs.pointee_align` when constructing function ABI#80822

Set `attrs.pointee_align` when constructing function ABI#80822
Aaron1011 wants to merge 1 commit intorust-lang:masterfrom
Aaron1011:fix/attr-pointee-align

Aaron1011 commented Jan 8, 2021

Uh oh!

rust-highfive commented Jan 8, 2021

Uh oh!

Aaron1011 commented Jan 8, 2021

Uh oh!

nagisa commented Jan 8, 2021

Uh oh!

nagisa commented Jan 10, 2021

Uh oh!

bors commented Jan 24, 2021

Uh oh!

camelid commented Feb 12, 2021

Uh oh!

Dylan-DPC-zz commented Mar 2, 2021

Uh oh!

crlf0710 commented Apr 23, 2021

Uh oh!

oli-obk commented Apr 23, 2021

Uh oh!

eddyb commented Apr 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Uh oh!

Conversation

Aaron1011 commented Jan 8, 2021

Uh oh!

rust-highfive commented Jan 8, 2021

Uh oh!

Aaron1011 commented Jan 8, 2021

Uh oh!

nagisa commented Jan 8, 2021

Uh oh!

nagisa commented Jan 10, 2021

Uh oh!

bors commented Jan 24, 2021

Uh oh!

camelid commented Feb 12, 2021

Uh oh!

Dylan-DPC-zz commented Mar 2, 2021

Uh oh!

crlf0710 commented Apr 23, 2021

Uh oh!

oli-obk commented Apr 23, 2021

Uh oh!

eddyb commented Apr 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants