Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

32-bit x86: Alignment issues for LL byval params #1356

Open
kinke opened this issue Mar 13, 2016 · 10 comments
Open

32-bit x86: Alignment issues for LL byval params #1356

kinke opened this issue Mar 13, 2016 · 10 comments

Comments

@kinke
Copy link
Member

kinke commented Mar 13, 2016

User code may crash when decorating certain function parameters with an LLVM align attribute for MSVC targets. It may lead to LLVM assertions in the inliner too, I don't remember exactly.

For 64-bit MSVC, the issue occurs for the special sret parameter. We currently work around it by not specifying the alignment for the pointer parameter, but still use the alignment when allocating the result. Not an issue anymore with recent LLVM.
For 32-bit MSVC, byval pointer parameters are also affected (we don't use byval at all for the Win64 ABI). The resulting user code crashes (when enabling the align attribute) seem to indicate that LLVM isn't aligning the function parameters stack correctly.

Linux and OSX don't have these issues.

Pinging @majnemer. :)

@kinke
Copy link
Member Author

kinke commented Mar 13, 2016

Today's LLVM is able to happily compile all modules (incl. unittests) with all alignment attributes enabled, for both 32-bit and 64-bit MSVC.
All release unittests even still pass on x64.
But a few unittests crash on x86, namely:

std.complex (SEGFAULT)
std.datetime (SEGFAULT)
std.format (SEGFAULT)

@majnemer
Copy link

I don't think the MS ABI really works with aligned byval arguments.
For:

struct __declspec(align(8)) S {};
void f(S);
void g() {
  f(S());
}

Clang generates:

target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
target triple = "i686-pc-windows-msvc18.0.0"

%struct.S = type { [8 x i8] }

define void @"\01?g@@YAXXZ"() #0 {
entry:
  %agg.tmp = alloca %struct.S, align 8
  call void @"\01?f@@YAXUS@@@Z"(%struct.S* byval align 4 %agg.tmp)
  ret void
}

declare void @"\01?f@@YAXUS@@@Z"(%struct.S* byval align 4)

@majnemer
Copy link

MSVC won't compile my example. Instead, it will issue error C2719:

'unnamed-parameter': formal parameter with requested alignment of 8 won't be aligned

https://msdn.microsoft.com/en-us/library/373ak2y1.aspx

@kinke
Copy link
Member Author

kinke commented Mar 14, 2016

I was afraid it was that kind of limitation. We could work around that by copying the original argument at the caller's site and passing a pointer to that aligned copy (that's what we do when passing value types > 64bit by value on Win64), at least for extern(D), but then we'd have our own home-brewed Win32 ABI, incompatible with DMD...

@majnemer
Copy link

Would inalloca work? inalloca allows you to manually create the argument stack space using an alloca. The alloca's alignment would be appropriately aligned.

@kinke
Copy link
Member Author

kinke commented Mar 15, 2016

Thanks for the hint, that could work (as long as the argument stack base is appropriately aligned too). Is there a reason why LLVM doesn't directly support aligned byval params for MSVC targets (32-bit at least)? I thought LLVM tries to be quite ABI-agnostic and that it's the front-end's responsibility to control the ABI... e.g., we manually implement the System V ABI (partially) for non-Windows x86_64 targets.
So if the 32-bit MSVC++ ABI doesn't support aligned byval params, I think the clang front-end shouldn't allow it, but it should still be possible for other languages/front-ends...

@kinke kinke changed the title MSVC: Alignment issues for LL byval/sret params MSVC: Alignment issues for LL byval params Mar 15, 2016
kinke added a commit to kinke/ldc that referenced this issue Mar 18, 2016
As for regular args due to LDC issue ldc-developers#1356.
This fixes runnable/ldc_cabi1 and so dmd-testsuite passes again.
kinke added a commit to kinke/ldc that referenced this issue Mar 18, 2016
As for regular args due to LDC issue ldc-developers#1356.
This fixes runnable/ldc_cabi1 and so dmd-testsuite passes again.
kinke added a commit to kinke/ldc that referenced this issue Mar 18, 2016
As for regular args due to LDC issue ldc-developers#1356.
This fixes runnable/ldc_cabi1 and so dmd-testsuite passes again.
kinke added a commit to kinke/ldc that referenced this issue Mar 18, 2016
As for regular args due to LDC issue ldc-developers#1356.
This fixes runnable/ldc_cabi1 and so dmd-testsuite passes again.
@majnemer
Copy link

@kinke What happens with GDC/DMD with the following:

  struct __declpsec(align(32)) S { char x[32]; };
  void f(S s, true);
  void g() {
    f(S(), true);
  }

I'd guess the following happens:

  • The incoming stack pointer gets aligned to 32-bytes.
  • 28 bytes of padding would need to get inserted between the 'bool' and the 'S' in order to get the 'S' value aligned.

@kinke
Copy link
Member Author

kinke commented Mar 20, 2016

While I could figure that out (another time, just passing by before going to bed), let me first say that we're not trying to mimic the ABI of other compilers in all aspects. What you described is what I would expect from LLVM, regardless of the actual target (nit-picking: I'd expect 31 bytes of padding inbetween the single-byte bool and the struct). If the bool was passed in a register, I'd simply expect an aligned function arguments stack base/incoming stack pointer.

@kinke kinke changed the title MSVC: Alignment issues for LL byval params 32-bit MSVC: Alignment issues for LL byval params Feb 23, 2022
kinke added a commit to ldc-developers/druntime that referenced this issue Feb 23, 2022
@kinke kinke changed the title 32-bit MSVC: Alignment issues for LL byval params 32-bit x86: Alignment issues for LL byval params Feb 28, 2022
@kinke
Copy link
Member Author

kinke commented Feb 28, 2022

druntime module core.int128 introduced in v2.099 uses an align(16) struct Cent, where we're hitting this issue on 32-bit MSVC. When not reversing the args (#3873), according crashes (aligned SSE instructions…) surface for i686 Linux as well, at least with LLVM 13. So byval alignments might not work properly for 32-bit x86 in general.

@p0nce
Copy link
Contributor

p0nce commented Nov 18, 2022

Is there a need to workaround this in intel-intrinsics? What should I avoid?

kinke added a commit that referenced this issue Nov 25, 2022
ssvb added a commit to competitive-dlang/speedy-int128 that referenced this issue Jan 5, 2023
CI tests fail on Ubuntu with LDC 1.30.0 for the compiled 32-bit
binaries. And the culprit is the Cent alignment. The problem is not
reproducible with the older LDC 1.20.1

Looks like all this alignment stuff is rather fragile:
  https://issues.dlang.org/show_bug.cgi?id=22980
  ldc-developers/ldc#1356 (comment)

The 16 byte alignment is not necessary in principle. Except for maybe
some C library bindings, which require compatibility with the '__int128'
type. C compilers don't support '__int128' for 32-bit code though.

For now just use the default struct alignment for Cent. This problem
can be investigated later and the 16 byte alignment may possibly
return at least for the 64-bit binaries.
ssvb added a commit to competitive-dlang/speedy-int128 that referenced this issue Jan 5, 2023
CI tests fail on Ubuntu with LDC 1.30.0 for the compiled 32-bit
binaries. And the culprit is the Cent alignment. The problem is not
reproducible with the older LDC 1.20.1

Looks like all this alignment stuff is rather fragile:
  https://issues.dlang.org/show_bug.cgi?id=22980
  ldc-developers/ldc#1356 (comment)

The 16 byte alignment is not necessary in principle. Except for maybe
some C library bindings, which require compatibility with the '__int128'
type. C compilers don't support '__int128' for 32-bit code though.

For now just use the default struct alignment for Cent. This problem
can be investigated later and the 16 byte alignment may possibly
return at least for the 64-bit binaries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants