[PATCH 0/1] MR4030: configure: Use -mpreferred-stack-boundary=2 on i386.
From: Zebediah Figura <zfigura(a)codeweavers.com> 62173699c38453777c7d5638ed2e779790506b75 exposed multiple gcc bugs related to stack alignment. While there seems to be motion to fix these bugs in gcc, it seems prudent to work around them for now in Wine. Instead of restoring __force_align_arg_pointer__ for all entry points, however, this patch takes a simpler approach, and one that is effectively the same as how this bug will eventually be solved upstream. We do not need to align every entry point, only those functions which will actually use aligned types. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55007 --- configure | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++- configure.ac | 5 +++- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/configure b/configure index aad417e0547..788647f18db 100755 --- a/configure +++ b/configure @@ -11322,6 +11322,39 @@ printf "%s\n" "$ac_res" >&6; } if eval test \"x\$"$as_ac_var"\" = x"yes" then : as_fn_append ${wine_arch}_EXTRACFLAGS " -fno-omit-frame-pointer" +fi } + { as_ac_var=`printf "%s\n" "ac_cv_${wine_arch}_cflags_-mpreferred-stack-boundary=2" | $as_tr_sh` +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking whether $CC supports -mpreferred-stack-boundary=2" >&5 +printf %s "checking whether $CC supports -mpreferred-stack-boundary=2... " >&6; } +if eval test \${$as_ac_var+y} +then : + printf %s "(cached) " >&6 +else $as_nop + ac_wine_try_cflags_saved=$CFLAGS +ac_wine_try_cflags_saved_exeext=$ac_exeext +CFLAGS="$CFLAGS -nostdlib -nodefaultlibs -mpreferred-stack-boundary=2" +ac_exeext=".exe" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +int __cdecl mainCRTStartup(void) { return 0; } +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + eval "$as_ac_var=yes" +else $as_nop + eval "$as_ac_var=no" +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +CFLAGS=$ac_wine_try_cflags_saved +ac_exeext=$ac_wine_try_cflags_saved_exeext +fi +eval ac_res=\$$as_ac_var + { printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_res" >&5 +printf "%s\n" "$ac_res" >&6; } +if eval test \"x\$"$as_ac_var"\" = x"yes" +then : + as_fn_append ${wine_arch}_EXTRACFLAGS " -mpreferred-stack-boundary=2" fi } { as_ac_var=`printf "%s\n" "ac_cv_${wine_arch}_cflags_-Wl,--disable-stdcall-fixup" | $as_tr_sh` { printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking whether $CC supports -Wl,--disable-stdcall-fixup" >&5 @@ -19247,7 +19280,8 @@ fi ;; esac case $host_cpu in - *i[3456789]86*) { printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking whether the compiler supports -fno-omit-frame-pointer" >&5 + *i[3456789]86*) + { printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking whether the compiler supports -fno-omit-frame-pointer" >&5 printf %s "checking whether the compiler supports -fno-omit-frame-pointer... " >&6; } if test ${ac_cv_cflags__fno_omit_frame_pointer+y} then : @@ -19274,6 +19308,34 @@ printf "%s\n" "$ac_cv_cflags__fno_omit_frame_pointer" >&6; } if test "x$ac_cv_cflags__fno_omit_frame_pointer" = xyes then : MSVCRTFLAGS="$MSVCRTFLAGS -fno-omit-frame-pointer" +fi + { printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking whether the compiler supports -mpreferred-stack-boundary=2" >&5 +printf %s "checking whether the compiler supports -mpreferred-stack-boundary=2... " >&6; } +if test ${ac_cv_cflags__mpreferred_stack_boundary_2+y} +then : + printf %s "(cached) " >&6 +else $as_nop + ac_wine_try_cflags_saved=$CFLAGS +CFLAGS="$CFLAGS -mpreferred-stack-boundary=2" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +int main(int argc, char **argv) { return 0; } +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + ac_cv_cflags__mpreferred_stack_boundary_2=yes +else $as_nop + ac_cv_cflags__mpreferred_stack_boundary_2=no +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +CFLAGS=$ac_wine_try_cflags_saved +fi +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_cv_cflags__mpreferred_stack_boundary_2" >&5 +printf "%s\n" "$ac_cv_cflags__mpreferred_stack_boundary_2" >&6; } +if test "x$ac_cv_cflags__mpreferred_stack_boundary_2" = xyes +then : + MSVCRTFLAGS="$MSVCRTFLAGS -mpreferred-stack-boundary=2" fi ;; *x86_64*) case $host_os in diff --git a/configure.ac b/configure.ac index 5676e35f2b9..a1fc739dd9c 100644 --- a/configure.ac +++ b/configure.ac @@ -971,6 +971,7 @@ This is an error since --enable-archs=$wine_arch was requested.])]) case $wine_arch in i386) WINE_TRY_PE_CFLAGS([-fno-omit-frame-pointer]) + WINE_TRY_PE_CFLAGS([-mpreferred-stack-boundary=2]) WINE_TRY_PE_CFLAGS([-Wl,--disable-stdcall-fixup], [AS_VAR_APPEND([${wine_arch}_LDFLAGS],[" -Wl,--disable-stdcall-fixup"])]) ;; x86_64) WINE_TRY_PE_CFLAGS([-Wformat-overflow]) @@ -1916,7 +1917,9 @@ char*f(const char *h,char n) {return strchr(h,n);}]])],[ac_cv_c_logicalop_noisy= case $host_cpu in dnl gcc-4.6+ omits frame pointers by default, breaking some copy protections - *i[[3456789]]86*) WINE_TRY_CFLAGS([-fno-omit-frame-pointer],[MSVCRTFLAGS="$MSVCRTFLAGS -fno-omit-frame-pointer"]) ;; + *i[[3456789]]86*) + WINE_TRY_CFLAGS([-fno-omit-frame-pointer],[MSVCRTFLAGS="$MSVCRTFLAGS -fno-omit-frame-pointer"]) + WINE_TRY_CFLAGS([-mpreferred-stack-boundary=2],[MSVCRTFLAGS="$MSVCRTFLAGS -mpreferred-stack-boundary=2"]) ;; *x86_64*) case $host_os in dnl Mingw uses Windows 64-bit types, not Unix ones -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4030
Shouldn't this also include `-mincoming-stack-boundary=2`? Not sure if MinGW already defaults to it though (incoming is for the stack alignment from "external" callers that GCC can't see, which is basically every caller of the Windows APIs, and they call it with 4-byte aligned stack). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_47871
Shouldn't this also include `-mincoming-stack-boundary=2`? Not sure if MinGW already defaults to it though (incoming is for the stack alignment from "external" callers that GCC can't see, which is basically every caller of the Windows APIs, and they call it with 4-byte aligned stack).
-mpreferred-stack-boundary implies -mincoming-stack-boundary. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_47889
This has been stalled for over a month. Is it distasteful that we're working around a compiler bug in this way? I'll admit I don't understand why, given that we've worked around compiler bugs before quite often (and in uglier ways, I think). We could also restore __force_align_arg_pointer__ instead, but I don't see how that's preferable to this approach? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_51572
The concern is that this is hiding the bug, and will prevent catching similar bugs in the future. Considering that it requires exotic build options to trigger, I'm not sure that it's worth it. Also I don't see why you are doing this for the host compiler too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_51577
The concern is that this is hiding the bug, and will prevent catching similar bugs in the future. Considering that it requires exotic build options to trigger, I'm not sure that it's worth it.
I don't understand, what bug is this hiding? This patch addresses two bugs that I know of: * gcc optimizes some on-stack variables to use SSE vectors, which must be 16-bit aligned; it then assumes that they are 16-bit aligned simply based on their frame offset and uses e.g. movaps to access them (WineHQ bug 55007); * we use DECLSPEC_ALIGN to specify manually aligned variables on-stack; gcc assumes that since the stack is 16-bit aligned already, it only needs to make sure their frame offset is aligned, which is not the case (encountered in merge request 3504). In both cases the most correct solution, as far as I can tell, is for gcc to be aware that the stack isn't 16-bit aligned. I don't see a more correct solution; am I missing something?
Also I don't see why you are doing this for the host compiler too.
I'm confused by this statement, because I didn't think I was. If you mean MSVCRTFLAGS, my understanding was that MSVCRTFLAGS is the flags we use to compile old-style .dll.so's that link to msvcrt (which is all of them, now) in case we're compiling without MinGW. Assuming that's correct, I added this to MSVCRTFLAGS since it seemed correct; i.e. the issue is present there as well (and in fact will continue to be present even if the upstream bug is fixed). I know that compiling without MinGW presents unfixable problems in some cases, but it hasn't been formally deprecated or removed yet, so I assumed that maintaining it wouldn't be the wrong thing to do. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_51629
I don't understand, what bug is this hiding?
This patch addresses two bugs that I know of:
* gcc optimizes some on-stack variables to use SSE vectors, which must be 16-bit aligned; it then assumes that they are 16-bit aligned simply based on their frame offset and uses e.g. movaps to access them (WineHQ bug 55007); * we use DECLSPEC_ALIGN to specify manually aligned variables on-stack; gcc assumes that since the stack is 16-bit aligned already, it only needs to make sure their frame offset is aligned, which is not the case (encountered in merge request 3504).
In both cases the most correct solution, as far as I can tell, is for gcc to be aware that the stack isn't 16-bit aligned. I don't see a more correct solution; am I missing something?
The bug is in gcc/mingw. The Windows ABI doesn't require the stack to be aligned, so when building a PE binary, gcc/mingw should already know that the stack isn't aligned, it shouldn't need to be told explicitly. My understanding is that Mingw mostly gets it right, but there are some exotic options like `-march=znver4` that trigger a broken code path. The hope is that the compiler can be fixed, and in the meantime we can tell people to avoid using exotic options. If we simply hide the bug, more of these broken code paths will creep up.
If you mean MSVCRTFLAGS, my understanding was that MSVCRTFLAGS is the flags we use to compile old-style .dll.so's that link to msvcrt (which is all of them, now) in case we're compiling without MinGW. Assuming that's correct, I added this to MSVCRTFLAGS since it seemed correct; i.e. the issue is present there as well (and in fact will continue to be present even if the upstream bug is fixed). I know that compiling without MinGW presents unfixable problems in some cases, but it hasn't been formally deprecated or removed yet, so I assumed that maintaining it wouldn't be the wrong thing to do.
Old-style .dll.so are built with the host compiler, and thus follow the host ABI which assumes that the stack is always 16-byte aligned. That's why we have to use `force_align_arg_pointer`. It wouldn't make sense to force aligning the stack everywhere and then tell the compiler that the stack isn't aligned. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_51659
On Thu Nov 9 17:28:33 2023 +0000, Alexandre Julliard wrote:
I don't understand, what bug is this hiding?
This patch addresses two bugs that I know of:
* gcc optimizes some on-stack variables to use SSE vectors, which must be 16-bit aligned; it then assumes that they are 16-bit aligned simply based on their frame offset and uses e.g. movaps to access them (WineHQ bug 55007); * we use DECLSPEC_ALIGN to specify manually aligned variables on-stack; gcc assumes that since the stack is 16-bit aligned already, it only needs to make sure their frame offset is aligned, which is not the case (encountered in merge request 3504).
In both cases the most correct solution, as far as I can tell, is for gcc to be aware that the stack isn't 16-bit aligned. I don't see a more correct solution; am I missing something? The bug is in gcc/mingw. The Windows ABI doesn't require the stack to be aligned, so when building a PE binary, gcc/mingw should already know that the stack isn't aligned, it shouldn't need to be told explicitly. My understanding is that Mingw mostly gets it right, but there are some exotic options like `-march=znver4` that trigger a broken code path. The hope is that the compiler can be fixed, and in the meantime we can tell people to avoid using exotic options. If we simply hide the bug, more of these broken code paths will creep up. If you mean MSVCRTFLAGS, my understanding was that MSVCRTFLAGS is the flags we use to compile old-style .dll.so's that link to msvcrt (which is all of them, now) in case we're compiling without MinGW. Assuming that's correct, I added this to MSVCRTFLAGS since it seemed correct; i.e. the issue is present there as well (and in fact will continue to be present even if the upstream bug is fixed). I know that compiling without MinGW presents unfixable problems in some cases, but it hasn't been formally deprecated or removed yet, so I assumed that maintaining it wouldn't be the wrong thing to do. Old-style .dll.so are built with the host compiler, and thus follow the host ABI which assumes that the stack is always 16-byte aligned. That's why we have to use `force_align_arg_pointer`. It wouldn't make sense to force aligning the stack everywhere and then tell the compiler that the stack isn't aligned. With respect to the gcc/mingw bug, isn't that applicable to all workarounds for bugs? Which basically means no workarounds at all. I don't think that's useful. Plus, this isn't actually anything particularly hacky, it's just making sure that those values are set to the proper ones for Windows.
And I believe this can also be triggered by -march=native if you have such CPU, which makes it more serious, no? Especially on source reliant distros. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_51735
The bug is in gcc/mingw. The Windows ABI doesn't require the stack to be aligned, so when building a PE binary, gcc/mingw should already know that the stack isn't aligned, it shouldn't need to be told explicitly.
My understanding is that Mingw mostly gets it right, but there are some exotic options like `-march=znver4` that trigger a broken code path. The hope is that the compiler can be fixed, and in the meantime we can tell people to avoid using exotic options. If we simply hide the bug, more of these broken code paths will creep up.
The problem is summarized in [1], especially comment 5. What gcc actually does is, it assumes 16-byte stack alignment for i686-w64-mingw32, but *if* SSE is enabled (as with -msse, or anything that implies it), it forces stack realignment on entry (as with -mstackrealign = __force_align_arg_pointer__). This addresses most cases, but for some reason -mstackrealign is broken with -mavx512f (implied by znver4). But it doesn't address the case where type alignment is manually specified, described in [1]. The right fix in that case, proposed by a gcc developer, is to tell gcc that the stack is not 16 byte aligned. This is what -mpreferred-stack-boundary=2 does. The proposed patch to gcc would simply make this the default. The broken interaction between -mstackrealign and -mavx512f is a bug that this would hide, and should absolutely be fixed independently. However, at the same time, the rest of the bug is something for which the right fix is to replace the -mstackrealign (which simply papers over the bug) with -mpreferred-stack-boundary=2. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111107
Old-style .dll.so are built with the host compiler, and thus follow the host ABI which assumes that the stack is always 16-byte aligned. That's why we have to use `force_align_arg_pointer`.
It wouldn't make sense to force aligning the stack everywhere and then tell the compiler that the stack isn't aligned.
Oh I see, I forgot that this problem is already fixed in that way. I think that -mpreferred-stack-boundary=2 is a better solution than force_align_arg_pointer, since the former should only force stack realignment where we actually need it, but I'll leave that for a separate change. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_51739
The broken interaction between -mstackrealign and -mavx512f is a bug that this would hide, and should absolutely be fixed independently. However, at the same time, the rest of the bug is something for which the right fix is to replace the -mstackrealign (which simply papers over the bug) with -mpreferred-stack-boundary=2.
Definitely, but it should be replaced in the gcc code, like the proposed patch does.
I think that -mpreferred-stack-boundary=2 is a better solution than force_align_arg_pointer, since the former should only force stack realignment where we actually need it, but I'll leave that for a separate change.
We can't know when it's needed, since .dll.so files can call host functions that require the stack to be aligned. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_51757
The broken interaction between -mstackrealign and -mavx512f is a bug that this would hide, and should absolutely be fixed independently. However, at the same time, the rest of the bug is something for which the right fix is to replace the -mstackrealign (which simply papers over the bug) with -mpreferred-stack-boundary=2.
Definitely, but it should be replaced in the gcc code, like the proposed patch does.
Sure. My reason for proposing this patch is that the gcc bug may take some time to be solved and propagate, and in the meantime, working around the bug using the same logic seems anodyne enough.
I think that -mpreferred-stack-boundary=2 is a better solution than force_align_arg_pointer, since the former should only force stack realignment where we actually need it, but I'll leave that for a separate change.
We can't know when it's needed, since .dll.so files can call host functions that require the stack to be aligned.
Ah, that makes sense. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_51768
On Thu Nov 16 20:24:40 2023 +0000, Zebediah Figura wrote:
The broken interaction between -mstackrealign and -mavx512f is a bug that this would hide, and should absolutely be fixed independently. However, at the same time, the rest of the bug is something for which the right fix is to replace the -mstackrealign (which simply papers over the bug) with -mpreferred-stack-boundary=2.
Definitely, but it should be replaced in the gcc code, like the proposed patch does. Sure. My reason for proposing this patch is that the gcc bug may take some time to be solved and propagate, and in the meantime, working around the bug using the same logic seems anodyne enough.
I think that -mpreferred-stack-boundary=2 is a better solution than force_align_arg_pointer, since the former should only force stack realignment where we actually need it, but I'll leave that for a separate change.
We can't know when it's needed, since .dll.so files can call host functions that require the stack to be aligned. Ah, that makes sense. See: https://bugs.winehq.org/show_bug.cgi?id=55899
I think this is another argument why we'd want to have this MR in, instead of waiting for upstream to fix it (if ever) on MinGW. It doesn't harm anything to explicitly mention the ABI… -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_52587
Do I understand correctly that the problem with this patch is that it adds -mpreferred-stack-boundary=2 to both PE and non-PE builds, but to avoid breaking Winelib apps, we only want to use the flag on PE builds, and non-PE builds should continue to use force_align_arg_pointer instead? MinGW should certainly be fixed to have a sane default stack alignment, but I agree with Zeb and Gabriel, there is no harm in explicitly requesting the correct stack alignment. Moreover, commit 62173699c38453777c7d5638ed2e779790506b75 caused several regressions, and it would be nice to fix them before releasing Wine 9.0. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4030#note_52639
participants (5)
-
Alex Henrie (＠alexhenrie) -
Alexandre Julliard (＠julliard)
-
Gabriel Ivăncescu
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)