[PATCH v3 1/4] dbghelp: Avoid using inline assembly in do_x86cpuid().

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
v3: fix stack offsets

dlls/dbghelp/minidump.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/dlls/dbghelp/minidump.c b/dlls/dbghelp/minidump.c
index 975bb45c9a..eedce8138d 100644
--- a/dlls/dbghelp/minidump.c
+++ b/dlls/dbghelp/minidump.c
@@ -542,20 +542,27 @@ static  unsigned        dump_modules(struct dump_context* dc, BOOL dump_elf)
     return sz;
 }

-/* Calls cpuid with an eax of 'ax' and returns the 16 bytes in *p
- * We are compiled with -fPIC, so we can't clobber ebx.
- */
-static inline void do_x86cpuid(unsigned int ax, unsigned int *p)
+extern void do_x86cpuid(unsigned int ax, unsigned int *p);
+
+#ifdef __i386__
+__ASM_GLOBAL_FUNC( do_x86cpuid,
+                   "pushl %esi\n\t"
+                   "pushl %ebx\n\t"
+                   "movl 12(%esp),%eax\n\t"
+                   "movl 16(%esp),%esi\n\t"
+                   "cpuid\n\t"
+                   "movl %eax,(%esi)\n\t"
+                   "movl %ebx,4(%esi)\n\t"
+                   "movl %ecx,8(%esi)\n\t"
+                   "movl %edx,12(%esi)\n\t"
+                   "popl %ebx\n\t"
+                   "popl %esi\n\t"
+                   "ret" );
+#else
+void do_x86cpuid(unsigned int ax, unsigned int *p)
 {
-#if defined(__GNUC__) && defined(__i386__)
-    __asm__("pushl %%ebx\n\t"
-            "cpuid\n\t"
-            "movl %%ebx, %%esi\n\t"
-            "popl %%ebx"
-            : "=a" (p[0]), "=S" (p[1]), "=c" (p[2]), "=d" (p[3])
-            :  "0" (ax));
-#endif
 }
+#endif

/* From xf86info havecpuid.c 1.11 */
 static inline int have_x86cpuid(void)

[PATCH v3 2/4] dbghelp: Avoid using inline assembly in have_x86cpuid().

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/dbghelp/minidump.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/dlls/dbghelp/minidump.c b/dlls/dbghelp/minidump.c
index eedce8138d..dfbfbede6e 100644
--- a/dlls/dbghelp/minidump.c
+++ b/dlls/dbghelp/minidump.c
@@ -564,28 +564,27 @@ void do_x86cpuid(unsigned int ax, unsigned int *p)
 }
 #endif

-/* From xf86info havecpuid.c 1.11 */
-static inline int have_x86cpuid(void)
-{
-#if defined(__GNUC__) && defined(__i386__)
-    unsigned int f1, f2;
-    __asm__("pushfl\n\t"
-            "pushfl\n\t"
-            "popl %0\n\t"
-            "movl %0,%1\n\t"
-            "xorl %2,%0\n\t"
-            "pushl %0\n\t"
-            "popfl\n\t"
-            "pushfl\n\t"
-            "popl %0\n\t"
-            "popfl"
-            : "=&r" (f1), "=&r" (f2)
-            : "ir" (0x00200000));
-    return ((f1^f2) & 0x00200000) != 0;
+extern int have_x86cpuid(void);
+
+#ifdef __i386__
+__ASM_GLOBAL_FUNC( have_x86cpuid,
+                   "pushfl\n\t"
+                   "pushfl\n\t"
+                   "movl (%esp),%ecx\n\t"
+                   "xorl $0x00200000,(%esp)\n\t"
+                   "popfl\n\t"
+                   "pushfl\n\t"
+                   "popl %eax\n\t"
+                   "popfl\n\t"
+                   "xorl %ecx,%eax\n\t"
+                   "andl $0x00200000,%eax\n\t"
+                   "ret" );
 #else
+int have_x86cpuid(void)
+{
     return 0;
-#endif
 }
+#endif

/******************************************************************
  *		dump_system_info

[PATCH v3 2/4] dbghelp: Avoid using inline assembly in have_x86cpuid().

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=45034

Your paranoid android.

=== debian9 (build log) ===

../../../wine/dlls/dbghelp/minidump.c:549:20: error: expected ‘)’ before string constant
../../../wine/dlls/dbghelp/minidump.c:571:20: error: expected ‘)’ before string constant
Makefile:642: recipe for target 'minidump.o' failed
Makefile:8635: recipe for target 'dlls/dbghelp' failed
Task: The win32 build failed

=== debian9 (build log) ===

../../../wine/dlls/dbghelp/minidump.c:549:20: error: expected ‘)’ before string constant
../../../wine/dlls/dbghelp/minidump.c:571:20: error: expected ‘)’ before string constant
Makefile:642: recipe for target 'minidump.o' failed
Makefile:8831: recipe for target 'dlls/dbghelp' failed
Task: The wow32 build failed

[PATCH v3 3/4] ntdll: Avoid inline assembly in do_cpuid().

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
v3: fix stack offsets

dlls/ntdll/nt.c | 47 +++++++++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/dlls/ntdll/nt.c b/dlls/ntdll/nt.c
index 5c711ef25b..288f4a4f16 100644
--- a/dlls/ntdll/nt.c
+++ b/dlls/ntdll/nt.c
@@ -944,27 +944,38 @@ static  SYSTEM_CPU_INFORMATION cached_sci;
 #define INEI	0x49656e69	/* "ineI" */
 #define NTEL	0x6c65746e	/* "ntel" */

-/* Calls cpuid with an eax of 'ax' and returns the 16 bytes in *p
- * We are compiled with -fPIC, so we can't clobber ebx.
- */
-static inline void do_cpuid(unsigned int ax, unsigned int *p)
-{
+extern void do_cpuid(unsigned int ax, unsigned int *p);
+
 #ifdef __i386__
-	__asm__("pushl %%ebx\n\t"
-                "cpuid\n\t"
-                "movl %%ebx, %%esi\n\t"
-                "popl %%ebx"
-                : "=a" (p[0]), "=S" (p[1]), "=c" (p[2]), "=d" (p[3])
-                :  "0" (ax));
+__ASM_GLOBAL_FUNC( do_cpuid,
+                   "pushl %esi\n\t"
+                   "pushl %ebx\n\t"
+                   "movl 12(%esp),%eax\n\t"
+                   "movl 16(%esp),%esi\n\t"
+                   "cpuid\n\t"
+                   "movl %eax,(%esi)\n\t"
+                   "movl %ebx,4(%esi)\n\t"
+                   "movl %ecx,8(%esi)\n\t"
+                   "movl %edx,12(%esi)\n\t"
+                   "popl %ebx\n\t"
+                   "popl %esi\n\t"
+                   "ret" );
 #elif defined(__x86_64__)
-	__asm__("push %%rbx\n\t"
-                "cpuid\n\t"
-                "movq %%rbx, %%rsi\n\t"
-                "pop %%rbx"
-                : "=a" (p[0]), "=S" (p[1]), "=c" (p[2]), "=d" (p[3])
-                :  "0" (ax));
-#endif
+__ASM_GLOBAL_FUNC( do_cpuid,
+                   "pushq %rbx\n\t"
+                   "movl %edi,%eax\n\t"
+                   "cpuid\n\t"
+                   "movl %eax,(%rsi)\n\t"
+                   "movl %ebx,4(%rsi)\n\t"
+                   "movl %ecx,8(%rsi)\n\t"
+                   "movl %edx,12(%rsi)\n\t"
+                   "popq %rbx\n\t"
+                   "ret" );
+#else
+void do_cpuid(unsigned int ax, unsigned int *p)
+{
 }
+#endif

/* From xf86info havecpuid.c 1.11 */
 static inline BOOL have_cpuid(void)

[PATCH v3 3/4] ntdll: Avoid inline assembly in do_cpuid().

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=45035

Your paranoid android.

=== debian9 (build log) ===

../../../wine/dlls/dbghelp/minidump.c:549:20: error: expected ‘)’ before string constant
../../../wine/dlls/dbghelp/minidump.c:571:20: error: expected ‘)’ before string constant
Makefile:642: recipe for target 'minidump.o' failed
Makefile:8635: recipe for target 'dlls/dbghelp' failed
Task: The win32 build failed

=== debian9 (build log) ===

../../../wine/dlls/dbghelp/minidump.c:549:20: error: expected ‘)’ before string constant
../../../wine/dlls/dbghelp/minidump.c:571:20: error: expected ‘)’ before string constant
Makefile:642: recipe for target 'minidump.o' failed
Makefile:8831: recipe for target 'dlls/dbghelp' failed
Task: The wow32 build failed

[PATCH v3 4/4] ntdll: Avoid inline assembly in have_cpuid().

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/ntdll/nt.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/dlls/ntdll/nt.c b/dlls/ntdll/nt.c
index 288f4a4f16..04717f04d9 100644
--- a/dlls/ntdll/nt.c
+++ b/dlls/ntdll/nt.c
@@ -977,30 +977,32 @@ void do_cpuid(unsigned int ax, unsigned int *p)
 }
 #endif

-/* From xf86info havecpuid.c 1.11 */
-static inline BOOL have_cpuid(void)
-{
+extern int have_cpuid(void);
+
 #ifdef __i386__
-	unsigned int f1, f2;
-	__asm__("pushfl\n\t"
-                "pushfl\n\t"
-                "popl %0\n\t"
-                "movl %0,%1\n\t"
-                "xorl %2,%0\n\t"
-                "pushl %0\n\t"
-                "popfl\n\t"
-                "pushfl\n\t"
-                "popl %0\n\t"
-                "popfl"
-                : "=&r" (f1), "=&r" (f2)
-                : "ir" (0x00200000));
-	return ((f1^f2) & 0x00200000) != 0;
+__ASM_GLOBAL_FUNC( have_cpuid,
+                   "pushfl\n\t"
+                   "pushfl\n\t"
+                   "movl (%esp),%ecx\n\t"
+                   "xorl $0x00200000,(%esp)\n\t"
+                   "popfl\n\t"
+                   "pushfl\n\t"
+                   "popl %eax\n\t"
+                   "popfl\n\t"
+                   "xorl %ecx,%eax\n\t"
+                   "andl $0x00200000,%eax\n\t"
+                   "ret" );
 #elif defined(__x86_64__)
-        return TRUE;
+int have_cpuid(void)
+{
+    return 1;
+}
 #else
-        return FALSE;
-#endif
+int have_cpuid(void)
+{
+    return 0;
 }
+#endif

/* Detect if a SSE2 processor is capable of Denormals Are Zero (DAZ) mode.
  *

[PATCH v3 4/4] ntdll: Avoid inline assembly in have_cpuid().

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=45036

Your paranoid android.

=== debian9 (build log) ===

../../../wine/dlls/dbghelp/minidump.c:549:20: error: expected ‘)’ before string constant
../../../wine/dlls/dbghelp/minidump.c:571:20: error: expected ‘)’ before string constant
Makefile:642: recipe for target 'minidump.o' failed
Makefile:8635: recipe for target 'dlls/dbghelp' failed
Task: The win32 build failed

=== debian9 (build log) ===

../../../wine/dlls/dbghelp/minidump.c:549:20: error: expected ‘)’ before string constant
../../../wine/dlls/dbghelp/minidump.c:571:20: error: expected ‘)’ before string constant
Makefile:642: recipe for target 'minidump.o' failed
Makefile:8831: recipe for target 'dlls/dbghelp' failed
Task: The wow32 build failed

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=45033

Your paranoid android.

=== debian9 (build log) ===

../../../wine/dlls/dbghelp/minidump.c:549:20: error: expected ‘)’ before string constant
Makefile:642: recipe for target 'minidump.o' failed
Makefile:8635: recipe for target 'dlls/dbghelp' failed
Task: The win32 build failed

=== debian9 (build log) ===

../../../wine/dlls/dbghelp/minidump.c:549:20: error: expected ‘)’ before string constant
Makefile:642: recipe for target 'minidump.o' failed
Makefile:8831: recipe for target 'dlls/dbghelp' failed
Task: The wow32 build failed

[Gabriel Ivăncescu]

Just out of curiosity, what is wrong with the inline assembly here
(and the other patches)? To me it looks easier to maintain and
generates better code.

I know there's a reason, that's why I'm asking :-)

[Zebediah Figura]

On 11/26/2018 12:35 PM, Gabriel Ivăncescu wrote:
> Just out of curiosity, what is wrong with the inline assembly here
> (and the other patches)? To me it looks easier to maintain and
> generates better code.
>
> I know there's a reason, that's why I'm asking :-)

"Easier to maintain" may be subjective; I personally find the constraint 
mnemonics harder to read. In any case I doubt that performance should be 
a concern here.

The first such patch I sent was 0c216b8ca, it was motivated by clang 
generating references to %esp despite the use of stack instructions (and 
despite -fno-omit-frame-pointer). Alexandre prescribed avoiding inline 
assembly for all but the most simple functions, IIRC because constraints 
are generally trickier to get right.

[Gabriel Ivăncescu]

On Mon, Nov 26, 2018 at 9:00 PM Zebediah Figura  wrote:
> "Easier to maintain" may be subjective; I personally find the constraint
> mnemonics harder to read. In any case I doubt that performance should be
> a concern here.
>
> The first such patch I sent was 0c216b8ca, it was motivated by clang
> generating references to %esp despite the use of stack instructions (and
> despite -fno-omit-frame-pointer). Alexandre prescribed avoiding inline
> assembly for all but the most simple functions, IIRC because constraints
> are generally trickier to get right.

I see, thanks for the explanation. By easier to maintain, I was
thinking more along the lines of: less code to sift through, easier to
change (no hardcoded offsets), not having to take into account calling
conventions, etc.

But I'm probably biased here due to my experience with inline assembly
and a little bit of GCC's internals, so asm operands and constraints
are sort of second nature to me and I find them very straightforward
(I just think of the asm statements as a black box, with inputs and
outputs). But that's most likely not the case for most people, so I
didn't realize it, and I thought there were other reasons.  (well,
except for x87 reg constraints, those are hard to understand by
anyone)