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

[zfigura at codeweavers dot com via Gcc-bugs]

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

--- Comment #17 from Zeb Figura  ---
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

[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 via Gcc-bugs]

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

--- Comment #16 from Eric Botcazou  ---
> 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 via Gcc-bugs]

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

--- Comment #15 from Zeb Figura  ---
(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 via Gcc-bugs]

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

--- Comment #14 from Eric Botcazou  ---
> 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

[ebotcazou at gcc dot gnu.org via Gcc-bugs]

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

Eric Botcazou  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

[alexhenrie24 at gmail dot com via Gcc-bugs]

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

--- Comment #13 from Alex Henrie  ---
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

[alexhenrie24 at gmail dot com via Gcc-bugs]

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

--- Comment #12 from Alex Henrie  ---
Created attachment 56687
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56687=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
 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 via Gcc-bugs]

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

--- Comment #11 from Alex Henrie  ---
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

[gabrielopcode at gmail dot com via Gcc-bugs]

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

--- Comment #10 from Gabriel Ivăncescu  ---
(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

[amonakov at gcc dot gnu.org via Gcc-bugs]

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

Alexander Monakov  changed:

   What|Removed |Added

 CC||amonakov at gcc dot gnu.org

--- Comment #9 from Alexander Monakov  ---
-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

[alexhenrie24 at gmail dot com via Gcc-bugs]

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

Alex Henrie  changed:

   What|Removed |Added

 CC||alexhenrie24 at gmail dot com

--- Comment #8 from Alex Henrie  ---
Created attachment 56685
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56685=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  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

[gabrielopcode at gmail dot com via Gcc-bugs]

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

Gabriel Ivăncescu  changed:

   What|Removed |Added

 CC||gabrielopcode at gmail dot com

--- Comment #7 from Gabriel Ivăncescu  ---
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

[zfigura at codeweavers dot com via Gcc-bugs]

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

--- Comment #6 from Zebediah Figura  ---
(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

[rguenth at gcc dot gnu.org via Gcc-bugs]

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

Richard Biener  changed:

   What|Removed |Added

 CC||10walls at gmail dot com

--- Comment #5 from Richard Biener  ---
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 via Gcc-bugs]

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

--- Comment #4 from Zebediah Figura  ---
(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

[pinskia at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #3 from Andrew Pinski  ---
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 via Gcc-bugs]

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

--- Comment #2 from Zebediah Figura  ---
(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 via Gcc-bugs]

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

--- Comment #1 from Andrew Pinski  ---
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.