[PATCH v3 1/4] dbghelp: Avoid using inline assembly in do_x86cpuid().
Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- 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) -- 2.14.1
Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- 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 -- 2.14.1
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
Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- 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) -- 2.14.1
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
Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- 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. * -- 2.14.1
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
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
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 :-)
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.
On Mon, Nov 26, 2018 at 9:00 PM Zebediah Figura <z.figura12(a)gmail.com> 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)
participants (3)
-
Gabriel Ivăncescu -
Marvin -
Zebediah Figura