Bug 592222 – sys-devel/gcc-5.3.0, 5.4.0 produces stupid code in wine (force_align_arg_pointer, -mstackrealign breakage)

• Status: RESOLVED UPSTREAM
• Product: Gentoo Linux

[Description Daniel Santos 2016-08-27 03:46:46 UTC]

I am not aware of any crashes caused by this, but all of those massive ms x64 abi to sysv abi pro/epilogues get bigger are using unaligned SSE moves because of this. It doesn't matter if you specify -mincoming-stack-boundary=4 because it overrides it. However, if you do NOT specify -mstackrealign or mark a function with __attribute__((force_align_arg_pointer)) then like magic, you get good code. This doesn't affect 32-bit x86 code.

The patch is here: https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=228728 and it would be very nice if we can suck that one into gcc-5.4.0's patch set. The bug report is misleading as it's labeled a "feature request." https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66697 but it didn't start doing this until some recent release (maybe 5.3, not sure).

Here's an example prologue:

        pushq   %rbp
        movq    %rsp, %rbp
        pushq   %rdi
        pushq   %rsi
        subq    $160, %rsp
        movups  %xmm6, -176(%rbp)
        andq    $-16, %rsp
        movups  %xmm7, -160(%rbp)
        movups  %xmm8, -144(%rbp)
        movups  %xmm9, -128(%rbp)
        movups  %xmm10, -112(%rbp)
        movups  %xmm11, -96(%rbp)
        movups  %xmm12, -80(%rbp)
        movups  %xmm13, -64(%rbp)
        movups  %xmm14, -48(%rbp)
        movups  %xmm15, -32(%rbp)

And this is what it should be:

        pushq   %rdi
        pushq   %rsi
        subq    $168, %rsp
        movaps  %xmm6, (%rsp)
        movaps  %xmm7, 16(%rsp)
        movaps  %xmm8, 32(%rsp)
        movaps  %xmm9, 48(%rsp)
        movaps  %xmm10, 64(%rsp)
        movaps  %xmm11, 80(%rsp)
        movaps  %xmm12, 96(%rsp)
        movaps  %xmm13, 112(%rsp)
        movaps  %xmm14, 128(%rsp)
        movaps  %xmm15, 144(%rsp)

I haven't measured it, but I promise this is making wine slower for 64-bit apps. In case you're interested in a test case:

extern void b(void);
__attribute__((ms_abi)) void a(void)
{
    b();
}

build that with:

gcc -m64 -O2 -mstackrealign -mincoming-stack-boundary=4 

or add the force_align_arg_pointer attribute to a();

[Comment 1 Arfrever Frehtes Taifersar Arahesis 2016-08-27 05:58:00 UTC]

(In reply to Daniel Santos from comment #0)
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=228728

This commit is from 2015-10-12 and it is already included in GCC 5.3.0 (released on 2015-12-04) and GCC 5.4.0 (released on 2016-06-03).
Maybe you pasted wrong URL?

[Comment 2 Daniel Santos 2016-08-27 20:53:16 UTC]

(In reply to Arfrever Frehtes Taifersar Arahesis from comment #1)
> (In reply to Daniel Santos from comment #0)
> > https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=228728
> 
> This commit is from 2015-10-12 and it is already included in GCC 5.3.0
> (released on 2015-12-04) and GCC 5.4.0 (released on 2016-06-03).
> Maybe you pasted wrong URL?

Oh poo! You are right. I guess I misread the patch then, as I thought it should have been in 5.3.0 as well. I guess I need to file a new gcc bug. I'm working on gcc right now and I think I hacked it to fix this case (potentially breaking others) just so I can get finished with my project.

I suppose that as a temporary work-around for Wine, these headers can be changed, but I have *not* verified that it solves the problem (as I hacked the compiler too). Also the selection of gcc 5.5.0 was just laziness, probably the whole #if/else can be replaced with the body of the #else:

diff --git a/include/msvcrt/crtdefs.h b/include/msvcrt/crtdefs.h
index dde5ea1..dc464a9 100644
--- a/include/msvcrt/crtdefs.h
+++ b/include/msvcrt/crtdefs.h
@@ -55,7 +55,7 @@
 #   error You need to define __stdcall for your compiler
 #  endif
 # elif defined(__x86_64__) && defined (__GNUC__)
-#  if (__GNUC__ > 5) || ((__GNUC__ == 5) && (__GNUC_MINOR__ >= 3))
+#  if (__GNUC__ > 5) || ((__GNUC__ == 5) && (__GNUC_MINOR__ >= 5))
 #   define __stdcall __attribute__((ms_abi)) __attribute__((__force_align_arg_pointer__))
 #  else
 #   define __stdcall __attribute__((ms_abi))
@@ -73,7 +73,7 @@
 #   define __cdecl __attribute__((__cdecl__))
 #  endif
 # elif defined(__x86_64__) && defined (__GNUC__)
-#  if (__GNUC__ > 5) || ((__GNUC__ == 5) && (__GNUC_MINOR__ >= 3))
+#  if (__GNUC__ > 5) || ((__GNUC__ == 5) && (__GNUC_MINOR__ >= 5))
 #   define __cdecl __attribute__((ms_abi)) __attribute__((__force_align_arg_pointer__))
 #  else
 #   define __cdecl __attribute__((ms_abi))
diff --git a/include/windef.h b/include/windef.h
index 1e54fcb..0cd9683 100644
--- a/include/windef.h
+++ b/include/windef.h
@@ -64,7 +64,7 @@ extern "C" {
 #   error You need to define __stdcall for your compiler
 #  endif
 # elif defined(__x86_64__) && defined (__GNUC__)
-#  if (__GNUC__ > 5) || ((__GNUC__ == 5) && (__GNUC_MINOR__ >= 3))
+#  if (__GNUC__ > 5) || ((__GNUC__ == 5) && (__GNUC_MINOR__ >= 5))
 #   define __stdcall __attribute__((ms_abi)) __attribute__((__force_align_arg_pointer__))
 #  else
 #   define __stdcall __attribute__((ms_abi))
@@ -82,7 +82,7 @@ extern "C" {
 #   define __cdecl __attribute__((__cdecl__))
 #  endif
 # elif defined(__x86_64__) && defined (__GNUC__)
-#  if (__GNUC__ > 5) || ((__GNUC__ == 5) && (__GNUC_MINOR__ >= 3))
+#  if (__GNUC__ > 5) || ((__GNUC__ == 5) && (__GNUC_MINOR__ >= 5))
 #   define __cdecl __attribute__((ms_abi)) __attribute__((__force_align_arg_pointer__))
 #  else
 #   define __cdecl __attribute__((ms_abi))

However, that probably belongs in a different bug report, and it only affects performance.

[Comment 3 Ryan Hill (RETIRED) 2016-08-28 00:11:02 UTC]

Feel free to take this upstream.