[PATCH v4 0/2] MR7237: include: Fix ReadNoFence64 on i386.

[Jinoh Kang (＠iamahuman)]

64-bit volatile accesses are not atomic on i386.  Both
i686-w64-mingw32-gcc and x86 MSVC splits 64-bit loads into a pair of
load/store ops.

Fix this by using a FILD/FISTP pair, which is also used to implement C11
atomics by i686-w64-mingw32-gcc.

Fixes: f82b1c1fcf770a5d6fa02c3f286282be79a201b8

--
  v4: include: Prevent misuse of __WINE_ATOMIC_* helper macros for non-atomic large accesses.
      include: Fix ReadNoFence64 on i386.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7237

[PATCH v4 1/2] include: Fix ReadNoFence64 on i386.

[Jinoh Kang]

From: Jinoh Kang <jinoh.kang.kr(a)gmail.com>

64-bit volatile accesses are not atomic on i386.  Both
i686-w64-mingw32-gcc and x86 MSVC splits 64-bit loads into a pair of
load/store ops.

Fix this by using a FILD/FISTP pair, which is also used to implement C11
atomics by i686-w64-mingw32-gcc.

Fixes: f82b1c1fcf770a5d6fa02c3f286282be79a201b8
---
 include/winnt.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/winnt.h b/include/winnt.h
index 2547f841b7c..cdf503354e8 100644
--- a/include/winnt.h
+++ b/include/winnt.h
@@ -7122,7 +7122,16 @@ static FORCEINLINE LONG ReadNoFence( LONG const volatile *src )
 
 static FORCEINLINE LONG64 ReadNoFence64( LONG64 const volatile *src )
 {
-    LONG64 value = __WINE_LOAD64_NO_FENCE( (__int64 const volatile *)src );
+    LONG64 value;
+#if defined(__i386__) && _MSC_VER < 1700
+    __asm {
+        mov   eax, src
+        fild  qword ptr [eax]
+        fistp value
+    }
+#else
+    value = __WINE_LOAD64_NO_FENCE( (__int64 const volatile *)src );
+#endif
     return value;
 }
 
@@ -7315,7 +7324,11 @@ static FORCEINLINE LONG ReadNoFence( LONG const volatile *src )
 static FORCEINLINE LONG64 ReadNoFence64( LONG64 const volatile *src )
 {
     LONG64 value;
+#ifdef __i386__
+    __asm__ __volatile__( "fildq %1\n\tfistpq %0" : "=m" (value) : "m" (src) : "memory", "st" );
+#else
     __WINE_ATOMIC_LOAD_RELAXED( src, &value );
+#endif
     return value;
 }
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/7237

[PATCH v4 2/2] include: Prevent misuse of __WINE_ATOMIC_* helper macros for non-atomic large accesses.

[Jinoh Kang]

From: Jinoh Kang <jinoh.kang.kr(a)gmail.com>

---
 include/winnt.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/winnt.h b/include/winnt.h
index cdf503354e8..08a29127cdf 100644
--- a/include/winnt.h
+++ b/include/winnt.h
@@ -7296,10 +7296,10 @@ static FORCEINLINE void MemoryBarrier(void)
 
 #if defined(__x86_64__) || defined(__i386__)
 /* On x86, Support old GCC with either no or buggy (GCC BZ#81316) __atomic_* support */
-#define __WINE_ATOMIC_LOAD_ACQUIRE(ptr, ret) do { *(ret) = *(ptr); __asm__ __volatile__( "" ::: "memory" ); } while (0)
-#define __WINE_ATOMIC_LOAD_RELAXED(ptr, ret) do { *(ret) = *(ptr); } while (0)
-#define __WINE_ATOMIC_STORE_RELEASE(ptr, val) do { __asm__ __volatile__( "" ::: "memory" ); *(ptr) = *(val); } while (0)
-#define __WINE_ATOMIC_STORE_RELAXED(ptr, val) do { *(ptr) = *(val); } while (0)
+#define __WINE_ATOMIC_LOAD_ACQUIRE(ptr, ret) do { C_ASSERT(sizeof(*(ptr)) <= sizeof(void *)); *(ret) = *(ptr); __asm__ __volatile__( "" ::: "memory" ); } while (0)
+#define __WINE_ATOMIC_LOAD_RELAXED(ptr, ret) do { C_ASSERT(sizeof(*(ptr)) <= sizeof(void *)); *(ret) = *(ptr); } while (0)
+#define __WINE_ATOMIC_STORE_RELEASE(ptr, val) do { C_ASSERT(sizeof(*(ptr)) <= sizeof(void *)); __asm__ __volatile__( "" ::: "memory" ); *(ptr) = *(val); } while (0)
+#define __WINE_ATOMIC_STORE_RELAXED(ptr, val) do { C_ASSERT(sizeof(*(ptr)) <= sizeof(void *)); *(ptr) = *(val); } while (0)
 #else
 #define __WINE_ATOMIC_LOAD_ACQUIRE(ptr, ret) __atomic_load(ptr, ret, __ATOMIC_ACQUIRE)
 #define __WINE_ATOMIC_LOAD_RELAXED(ptr, ret) __atomic_load(ptr, ret, __ATOMIC_RELAXED)
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/7237

[PATCH v4 2/2] include: Prevent misuse of __WINE_ATOMIC_* helper macros for non-atomic large accesses.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=151179

Your paranoid android.


=== debian11 (build log) ===

../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
Task: The win32 Wine build failed

=== debian11b (build log) ===

../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
../wine/include/winnt.h:405:21: error: static assertion failed: "sizeof(*(&__obj->seq)) <= sizeof(void *)"
Task: The wow32 Wine build failed

[Jinoh Kang (＠iamahuman)]

v4: Remove erroneous extra indirection in inline assembly.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7237#note_93369

[Alfred Agrell (＠Alcaro)]

I'm also unsure if the msvc branch is correct. Is \_\_i386\_\_ defined for msvc? Isn't it _M_IX86?

...though that'll only do the wrong thing for Visual Studio 2010 and older. I don't know if we need to care about that.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7237#note_93370

[Jinoh Kang (＠iamahuman)]

> I'm also unsure if the msvc branch is correct. Is \_\_i386\_\_ defined for msvc? Isn't it \_M_IX86?

tools/winapi/msvcmaker defines it.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7237#note_93374