[Bug target/111107] New: i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[Bug target/111107] New: i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[zfigura at codeweavers dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

            Bug ID: 111107
           Summary: i686-w64-mingw32 does not realign stack when
                    __attribute__((aligned)) or
                    __attribute__((vector_size)) are used
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Keywords: ABI, wrong-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zfigura at codeweavers dot com
  Target Milestone: ---
            Target: i686-w64-mingw32

Minimal example:

typedef int myint[4] __attribute__((aligned(16)));

extern void g(void *);

void f(void)
{
    myint a;
    g(&a);
}

The same thing happens if __attribute__((aligned(16))) is applied to the
variable instead of the typedef.

This seems to also prevent __m128 from being aligned correctly (which uses the
"vector_size" attribute rather than "aligned", but I would assume that
"vector_size" implies "aligned").


-mincoming-stack-boundary=2 works as a workaround; so does -mstackrealign.
Neither should be necessary, though.

I've seen some disagreement [1] [2] as to whether the stack alignment for
i686-w64-mingw32 *should* be 16 or 4, but as far as I can tell it really should
be 4. It's explicitly called out in a code comment [3]; it shows up when -msse2
is used [4], and, well, it reflects the actual ABI of programs that exist in
the wild.

We do regularly come across programs in Wine that don't align the stack to a
16-byte boundary before calling win32 functions, and while -mstackrealign and
similar functions exist, they imply that we either waste time and space
unnecessarily aligning *every* function, or we manually align any function that
might use an aligned type, which is in general something that's treated as the
compiler's responsibility.

[1] https://github.com/mingw-w64/mingw-w64/issues/30#issuecomment-1685487779
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110273#c5
[3]
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/i386/cygming.h;h=d539f8d0699d69b014e9d3378e78d690ea289f14;hb=HEAD#l34
[4] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110273#c6

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This on purpose, it is only callbacks (from libc) and main that needs the
realignment here.

There are other bug reports for a similar thing for mingw even.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[zfigura at codeweavers dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #2 from Zebediah Figura <zfigura at codeweavers dot com> ---
(In reply to Andrew Pinski from comment #1)
> This on purpose, it is only callbacks (from libc) and main that needs the
> realignment here.

I don't understand what you mean? It's not just libc and main that needs this.
As mentioned, this is *the* 32-bit x86 ABI on Windows. Win32 programs compiled
with MSVC don't assume 16-byte alignment (if they do now, they didn't
historically, and we do regularly run across programs in Wine that do not keep
the stack aligned to 16 bytes).

And again, gcc does not, as a blanket statement, assume 16-byte stack alignment
for i386. If we think that gcc *should* assume 16-byte stack alignment, then we
should also get rid of the existing code in gcc that assumes 4-byte stack
alignment. I think this is a bad idea, for the reasons I've been describing,
but if that's the decision then let's please at least be consistent and clear
about it.

I'm sure this is something of a canned response since, as you say, this issue
has been reported before (although I couldn't actually find any such reports,
just from searching the gcc Bugzilla?). I wouldn't report this as a bug per se
if gcc wasn't currently being *inconsistent* about what it assumes the stack
alignment is.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://inbox.sourceware.org/gcc-patches/5969976.Bvae8NF9fS@polaris/

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[zfigura at codeweavers dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #4 from Zebediah Figura <zfigura at codeweavers dot com> ---
(In reply to Andrew Pinski from comment #3)
> https://inbox.sourceware.org/gcc-patches/5969976.Bvae8NF9fS@polaris/

Again, I'm not sure what you're trying to communicate here. I'm aware that
-mstackrealign exists (and its attribute equivalent). We *do* use that in Wine.

Here is, again, what I am trying to communicate: Currently i686-w64-mingw32-gcc
effectly assumes 4-byte stack alignment in some places (when -msse2 is used),
and 16-byte alignment in others (when __attribute__((aligned)) is used). I am
trying to request that it pick one or the other and stick with it.

Now, personally, I think that assuming 4-byte stack alignment makes more
*sense*. Otherwise *every* API function needs that extra alignment, which is
wasteful when comparatively little code actually uses types aligned to 8 or
more bytes. (It obviously makes more sense if you can get the whole API to
agree on 16-bytes; then you don't have to manually align anything).

But if there's a clear consensus that gcc should assume 16 bytes, and that it's
Wine's responsibility to set -mstackrealign, or -mincoming-stack-boundary=2, or
something, fine, but I'd like GCC to be consistent about that policy. Otherwise
it looks like this behaviour is a bug. That's why I reported this as a bug.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |10walls at gmail dot com

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
I'd say that

config/i386/cygming.h:#define STACK_REALIGN_DEFAULT TARGET_SSE

is a non-working "fix".  The appropriate default would be
-mincoming-stack-boundary=2.  MIN_STACK_BOUNDARY should already be 4, so that
leaves PREFERRED_STACK_BOUNDARY_DEFAULT is the way to go here.  I also see

/* It should be MIN_STACK_BOUNDARY.  But we set it to 128 bits for
   both 32bit and 64bit, to support codes that need 128 bit stack
   alignment for SSE instructions, but can't realign the stack.  */
#define PREFERRED_STACK_BOUNDARY_DEFAULT \
  (TARGET_IAMCU ? MIN_STACK_BOUNDARY : 128)

which suggests there might be problems with SSE anyway.

So does the following work?

diff --git a/gcc/config/i386/cygming.h b/gcc/config/i386/cygming.h
index d539f8d0699..e8db548510b 100644
--- a/gcc/config/i386/cygming.h
+++ b/gcc/config/i386/cygming.h
@@ -31,10 +31,9 @@ along with GCC; see the file COPYING3.  If not see
 #undef MAX_STACK_ALIGNMENT
 #define MAX_STACK_ALIGNMENT  (TARGET_SEH ? 128 : MAX_OFILE_ALIGNMENT)

-/* 32-bit Windows aligns the stack on a 4-byte boundary but SSE instructions
-   may require 16-byte alignment.  */
-#undef STACK_REALIGN_DEFAULT
-#define STACK_REALIGN_DEFAULT TARGET_SSE
+/* 32-bit Windows aligns the stack on a 4-byte boundary.  */
+#undef PREFERRED_STACK_BOUNDARY_DEFAULT
+#define PREFERRED_STACK_BOUNDARY_DEFAULT (TARGET_64BIT ? 128 : 32)

 /* Support hooks for SEH.  */
 #undef  TARGET_ASM_UNWIND_EMIT

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[zfigura at codeweavers dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #6 from Zebediah Figura <zfigura at codeweavers dot com> ---
(In reply to Zebediah Figura from comment #4)
> (In reply to Andrew Pinski from comment #3)
> > https://inbox.sourceware.org/gcc-patches/5969976.Bvae8NF9fS@polaris/
> 
> Again, I'm not sure what you're trying to communicate here. I'm aware that
> -mstackrealign exists (and its attribute equivalent). We *do* use that in
> Wine.

Ah, I'm sorry, I think I see what you're trying to say—that it was an
intentional choice to add -mstackrealign if -msse2 is used, so it's hard to
call this a "bug" per se.

(In reply to Richard Biener from comment #5)
> I'd say that
> 
> config/i386/cygming.h:#define STACK_REALIGN_DEFAULT TARGET_SSE
> 
> is a non-working "fix".  The appropriate default would be
> -mincoming-stack-boundary=2.  MIN_STACK_BOUNDARY should already be 4, so
> that leaves PREFERRED_STACK_BOUNDARY_DEFAULT is the way to go here.  I also
> see
> 
> /* It should be MIN_STACK_BOUNDARY.  But we set it to 128 bits for
>    both 32bit and 64bit, to support codes that need 128 bit stack
>    alignment for SSE instructions, but can't realign the stack.  */
> #define PREFERRED_STACK_BOUNDARY_DEFAULT \
>   (TARGET_IAMCU ? MIN_STACK_BOUNDARY : 128)
> 
> which suggests there might be problems with SSE anyway.
>
> So does the following work?

But I would agree with this, yeah. If we're going to manually align for SSE
then we should also manually align for types that need to be manually aligned.
Which means that we should just have -mincoming-stack-boundary=2 everywhere.

In theory that patch works, although I'll have to put together a gcc build to
be sure.

I do have one question, though... from reading the documentation, I have a hard
time understanding the difference, or intended difference, between
-mincoming-stack-boundary and -mpreferred-stack-boundary. Could you by chance
try to clarify?

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[gabrielopcode at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

Gabriel Ivăncescu <gabrielopcode at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gabrielopcode at gmail dot com

--- Comment #7 from Gabriel Ivăncescu <gabrielopcode at gmail dot com> ---
So to re-iterate summary of the problem:

1) The i686 Win32 ABI has a de-facto stack alignment of 4 bytes *only*. GCC may
have set it to 16 bytes on Linux because it compiled the whole userland, but
that's not the case on Windows; the caller can be MSVC compiled code (very
likely on Windows) and MSVC only uses 4-byte alignment.

2) SSE is *not* the only thing that requires stack realignment. Sure, it does
require it, but that's more a side effect of requiring larger-than-4 alignment
in the first place.

A variable (or its type) declared with __attribute__((aligned(...))) **should**
also let GCC re-align the stack upon entry, if it's > 4 bytes and if it's
actually used on the stack and spilled (or has its address taken).

There's no reason to special-case SSE at all. It's just the alignment of the
variable or spilled vector that should matter, and GCC must know that the
incoming stack is aligned only to 4 bytes on this platform.

i686 PE targets should simply default to -mincoming-stack-boundary=2
-mpreferred-stack-boundary=2 (the latter to minimize realignments unless
necessary), as that's basically MSVC's behavior, and as such the de-facto
standard on this platform.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[alexhenrie24 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

Alex Henrie <alexhenrie24 at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |alexhenrie24 at gmail dot com

--- Comment #8 from Alex Henrie <alexhenrie24 at gmail dot com> ---
Created attachment 56685
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56685&action=edit
File that crashed my patched MinGW

I tried to compile Wine 8.21 with GCC 12 plus the patch from comment #5 and
-march=native on a Ryzen 7800X3D, but MinGW crashed with:

i686-w64-mingw32-gcc -c -o libs/tiff/i386-windows/libtiff/tif_aux.o
libs/tiff/libtiff/tif_aux.c -Ilibs/tiff -Iinclude -Iinclude/msvcrt \
  -Ilibs/tiff/libtiff -Ilibs/jpeg -Ilibs/zlib -D_UCRT -DFAR= -DZ_SOLO
-D__WINE_PE_BUILD \
  -fno-strict-aliasing -Wno-packed-not-aligned -fno-omit-frame-pointer
-march=native -freport-bug
during RTL pass: split1
libs/tiff/libtiff/tif_aux.c: In function ‘_TIFFUInt64ToFloat’:
libs/tiff/libtiff/tif_aux.c:415:1: internal compiler error: in
assign_stack_local_1, at function.cc:429
  415 | }
      | ^
0x19408f7 internal_error(char const*, ...)
        ???:0
0x6674e6 fancy_abort(char const*, int, char const*)
        ???:0
0xee3860 assign_386_stack_local(machine_mode, ix86_stack_slot)
        ???:0
0x130bfa7 gen_split_56(rtx_insn*, rtx_def**)
        ???:0
0x17066c2 split_insns(rtx_def*, rtx_insn*)
        ???:0
0x873ece try_split(rtx_def*, rtx_insn*, int)
        ???:0
0xb60f02 split_all_insns()
        ???:0
Please submit a full bug report, with preprocessed source.
Please include the complete backtrace with any bug report.
See <https://bugs.archlinux.org/> for instructions.
Preprocessed source stored into /tmp/cc824MzT.out file, please attach this to
your bugreport.
make: *** [Makefile:179744: libs/tiff/i386-windows/libtiff/tif_aux.o] Error 1

Omitting -march=native or using unpatched GCC avoids the compiler crash.

I used GCC 12 because unfortunately, Arch Linux does not yet have packaging for
GCC 13, and compiling MinGW without the help of a PKGBUILD file looked pretty
daunting. If you want to try it yourself, just clone
https://gitlab.winehq.org/wine/wine.git and run `./configure
CROSSCFLAGS='-march=native' && make -j16`.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amonakov at gcc dot gnu.org

--- Comment #9 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
-mpreferred-stack-boundary=n means that functions consume stack in increments
of 2**n. This is sufficient to ensure that once stack is aligned to that
boundary, it will keep it without the need for dynamic realignment.

-mincoming-stack-boundary specifies the guaranteed alignment on entry. If the
function needs to place local variables with greater alignment requirement on
the stack, it has perform dynamic realignment.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[gabrielopcode at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #10 from Gabriel Ivăncescu <gabrielopcode at gmail dot com> ---
(In reply to Alexander Monakov from comment #9)
> -mpreferred-stack-boundary=n means that functions consume stack in
> increments of 2**n. This is sufficient to ensure that once stack is aligned
> to that boundary, it will keep it without the need for dynamic realignment.
> 
> -mincoming-stack-boundary specifies the guaranteed alignment on entry. If
> the function needs to place local variables with greater alignment
> requirement on the stack, it has perform dynamic realignment.

Yeah, but on 32-bit x86 Windows ABI, the stack is only guaranteed to be aligned
to 4 bytes (mincoming-stack-boundary=2). We don't control the callers, and apps
compiled with MSVC (i.e. the majority of them, including Windows' own
libraries) only adhere to this alignment, nothing more than that.

Therefore -mincoming-stack-boundary=2 should be the default on MinGW for this
target.

In general, setting -mpreferred-stack-boundary=2 is also preferable because the
vast majority of functions do *not* use SSE or AVX or whatever. If you set
-mpreferred-stack-boundary=4 it will *always* align the stack on entry, and
that's simply too much overhead. It's better only for functions that actually
need it to do it.

The point is, -mpreferred-stack-boundary=2 (and consequently
-mincoming-stack-boundary=2) is the default on MSVC and Windows 32-bit x86 ABI,
and that's for a reason. MinGW should follow that. By default, at least.

I've compiled Wine with those options for years now without trouble.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[alexhenrie24 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #11 from Alex Henrie <alexhenrie24 at gmail dot com> ---
Well, this is interesting: Unpatched MinGW 12 crashes in the same way if I set
both -march=native and -mpreferred-stack-boundary=2. So the problem is not the
patch itself, it's just that the patch revealed some other bug.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[alexhenrie24 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #12 from Alex Henrie <alexhenrie24 at gmail dot com> ---
Created attachment 56687
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56687&action=edit
Minimal example to reproduce the crash

Here's a minimal example that crashes on MinGW 12 with -m32 -mavx512f
-mpreferred-stack-boundary=2. I tried it with MinGW 13 on Compiler Explorer
<https://godbolt.org/> and it does not crash, so it looks like that bug has
been fixed already.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[alexhenrie24 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #13 from Alex Henrie <alexhenrie24 at gmail dot com> ---
I should clarify that I was testing with GCC 12.2. It turns out that GCC 12.3
does not crash, and I have now confirmed that the patch from comment #5 applied
to GCC 12.3 fixes https://bugs.winehq.org/show_bug.cgi?id=55007

What will it take to get the patch accepted?

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-11-28

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> I'd say that
> 
> config/i386/cygming.h:#define STACK_REALIGN_DEFAULT TARGET_SSE
> 
> is a non-working "fix".  The appropriate default would be
> -mincoming-stack-boundary=2.  MIN_STACK_BOUNDARY should already be 4, so
> that leaves PREFERRED_STACK_BOUNDARY_DEFAULT is the way to go here.

This was a minimal fix to support SSE, but Solaris was indeed more radical:

sol2.h:#undef STACK_REALIGN_DEFAULT
sol2.h:#define STACK_REALIGN_DEFAULT (TARGET_64BIT ? 0 : 1)

so we could just mimic it for Windows.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[zfigura at codeweavers dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #15 from Zeb Figura <zfigura at codeweavers dot com> ---
(In reply to Eric Botcazou from comment #14)
> > I'd say that
> > 
> > config/i386/cygming.h:#define STACK_REALIGN_DEFAULT TARGET_SSE
> > 
> > is a non-working "fix".  The appropriate default would be
> > -mincoming-stack-boundary=2.  MIN_STACK_BOUNDARY should already be 4, so
> > that leaves PREFERRED_STACK_BOUNDARY_DEFAULT is the way to go here.
> 
> This was a minimal fix to support SSE, but Solaris was indeed more radical:
> 
> sol2.h:#undef STACK_REALIGN_DEFAULT
> sol2.h:#define STACK_REALIGN_DEFAULT (TARGET_64BIT ? 0 : 1)
> 
> so we could just mimic it for Windows.

Why use STACK_REALIGN_DEFAULT rather than PREFERRED_STACK_BOUNDARY_DEFAULT?

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #16 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Why use STACK_REALIGN_DEFAULT rather than PREFERRED_STACK_BOUNDARY_DEFAULT?

We know that it works since Solaris has used it for ages, so this alternate way
could be deemed riskier.  But no strong opinion, if the consensus is to use the
latter, then so be it.

[Bug target/111107] i686-w64-mingw32 does not realign stack when __attribute__((aligned)) or __attribute__((vector_size)) are used

[zfigura at codeweavers dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107

--- Comment #17 from Zeb Figura <zfigura at codeweavers dot com> ---
Actually, for that matter, what is the intended purpose of -mstackrealign? How
is it supposed to differ from -mincoming-stack-boundary and
-mpreferred-stack-boundary? The documentation is kind of unclear; it "realigns
the stack at entry [...] if necessary", which sounds like it could be
synonymous with -mincoming-stack-boundary or -mpreferred-stack-boundary.

But the mechanism in the code seems to be entirely separate, and it's also
broken with -mavx512f (bug 110273). From some quick testing it also seems to be
broken with aligned(8), though not aligned(16). That seems to be due to the
logic at [1]; not sure if that was intentional but I'll admit it doesn't make
much sense to me. But I also don't see why this mechanism is used instead of
whatever mechanism is used for -mincoming-stack-boundary.

[1]
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/i386/i386.cc;h=9390f525b99f0c078c912876aee8498bc3e7701b;hb=HEAD#l7768