From: Ivan Maidanski <ivmai@mail.ru>
Date: Fri, 20 Jan 2017 08:58:13 +0000 (+0300)
Subject: Fix test_atomic failure caused unaligned AO_double_t access on x86 (VC++)
X-Git-Tag: v7.6.0~131
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=69c4010c1b3d43e2e179421e27379aaa5aeeb6d8;p=libatomic_ops

Fix test_atomic failure caused unaligned AO_double_t access on x86 (VC++)

Passing an unaligned AO_double_t pointer to AO double-wide primitives
results in an undefined behavior of the latter when running on x86 (or
violation of the corresponding assertion on the alignment).
MS VC++/x86 aligns AO_double_t values on a 4-byte boundary by default.
For the proper alignment, __declspec(align(8)) should be applied to
variables defined in the client code (which uses double-wide AO
primitives).  Unfortunately, the attribute cannot be added to
AO_double_t definition itself because the compiler does not allow the
attribute for function arguments.

This patch introduces AO_DOUBLE_ALIGN attribute for use by clients of
the double-wide AO primitives (and, thus, AO_stack clients).
Matters only Visual Studio compiler for X86.
The inner clients (atomic_ops_malloc, test_atomic, test_stack) are
updated to use this attribute.

* doc/README_win32.txt [x86] (AO_DOUBLE_ALIGN): Document.
* src/atomic_ops/sysdeps/generic_pthread.h (AO_DOUBLE_ALIGN): Define
(as empty).
* src/atomic_ops/sysdeps/standard_ao_double_t.h [!_WIN64 && _WIN32
&& !__GNUC__ && _MSC_VER] (AO_DOUBLE_ALIGN): Define as declspec
align(8); document it.
* src/atomic_ops/sysdeps/standard_ao_double_t.h [!AO_DOUBLE_ALIGN]
(AO_DOUBLE_ALIGN): Define as empty (otherwise).
* src/atomic_ops_stack.h [AO_USE_ALMOST_LOCK_FREE && !AO_DOUBLE_ALIGN]
(AO_DOUBLE_ALIGN): Likewise.
* src/atomic_ops_malloc.c (AO_free_list): Use AO_DOUBLE_ALIGN attribute.
* tests/test_stack.c (the_list): Likewise.
* src/atomic_ops_stack.h [!AO_USE_ALMOST_LOCK_FREE] (AO_stack_t):
Document AO_DOUBLE_ALIGN usage (by clients).
* tests/test_atomic_include.template (test_atomicXX): Use
AO_DOUBLE_ALIGN attribute for old_w and w double-wide local variables
(to avoid alignment assertion violation or AO primitives undefined
behavior on x86 if the test code is compiled by VC++).
---

diff --git a/doc/README_win32.txt b/doc/README_win32.txt
index e2b87ad..812006c 100644
--- a/doc/README_win32.txt
+++ b/doc/README_win32.txt
@@ -26,6 +26,11 @@ Most clients of atomic_ops.h will need to define AO_ASSUME_WINDOWS98 before
 including it.  Compare_and_swap is otherwise not available.
 Defining AO_ASSUME_VISTA will make compare_double_and_swap_double available
 as well.
+Please note that MS compiler for x86 does not align AO_double_t on an 8-byte
+boundary, thus to avoid an undefined behavior, an AO_double_t (volatile)
+variable should be declared with AO_DOUBLE_ALIGN attribute if the variable
+reference is passed to an AO primitive (the attribute is not applicable to
+arguments and pointers).
 
 Note that the library is covered by the GNU General Public License, while
 the top 2 of these pieces allow use in proprietary code.
diff --git a/src/atomic_ops/sysdeps/generic_pthread.h b/src/atomic_ops/sysdeps/generic_pthread.h
index 3c65624..2e70068 100644
--- a/src/atomic_ops/sysdeps/generic_pthread.h
+++ b/src/atomic_ops/sysdeps/generic_pthread.h
@@ -367,6 +367,7 @@ typedef struct {
         AO_t AO_val2;
 } AO_double_t;
 #define AO_HAVE_double_t
+#define AO_DOUBLE_ALIGN /* empty */
 
 #define AO_DOUBLE_T_INITIALIZER { (AO_t)0, (AO_t)0 }
 
diff --git a/src/atomic_ops/sysdeps/standard_ao_double_t.h b/src/atomic_ops/sysdeps/standard_ao_double_t.h
index 7d85c9d..636ba46 100644
--- a/src/atomic_ops/sysdeps/standard_ao_double_t.h
+++ b/src/atomic_ops/sysdeps/standard_ao_double_t.h
@@ -50,11 +50,25 @@
   typedef __m128 double_ptr_storage;
 #elif defined(_WIN32) && !defined(__GNUC__)
   typedef unsigned __int64 double_ptr_storage;
+# ifdef _MSC_VER
+    /* VC++/x86 does not align __int64 properly by default, thus,       */
+    /* causing an undefined behavior or assertions violation in         */
+    /* the double-wide atomic primitives.  For the proper alignment,    */
+    /* all variables of AO_double_t type (in the client code) those     */
+    /* address is passed to an AO primitive should be defined with the  */
+    /* given attribute.  Not a part of double_ptr_storage because the   */
+    /* attribute cannot be applied to function parameters.              */
+#   define AO_DOUBLE_ALIGN __declspec(align(8))
+# endif
 #else
   typedef unsigned long long double_ptr_storage;
 #endif
 # define AO_HAVE_DOUBLE_PTR_STORAGE
 
+#ifndef AO_DOUBLE_ALIGN
+# define AO_DOUBLE_ALIGN /* empty */
+#endif
+
 typedef union {
     struct { AO_t AO_v1; AO_t AO_v2; } AO_parts;
         /* Note that AO_v1 corresponds to the low or the high part of   */
diff --git a/src/atomic_ops_malloc.c b/src/atomic_ops_malloc.c
index aae21d0..457d635 100644
--- a/src/atomic_ops_malloc.c
+++ b/src/atomic_ops_malloc.c
@@ -223,7 +223,7 @@ get_chunk(void)
 
 /* Object free lists.  Ith entry corresponds to objects         */
 /* of total size 2**i bytes.                                    */
-AO_stack_t AO_free_list[LOG_MAX_SIZE+1];
+AO_stack_t AO_DOUBLE_ALIGN AO_free_list[LOG_MAX_SIZE+1];
 
 /* Break up the chunk, and add it to the object free list for   */
 /* the given size.  We have exclusive access to chunk.          */
diff --git a/src/atomic_ops_stack.h b/src/atomic_ops_stack.h
index 1ca5f40..de41c6b 100644
--- a/src/atomic_ops_stack.h
+++ b/src/atomic_ops_stack.h
@@ -146,6 +146,11 @@ AO_INLINE void AO_stack_init(AO_stack_t *list)
         AO_stack_pop_explicit_aux_acquire(&((l)->AO_ptr), &((l)->AO_aux))
 #define AO_HAVE_stack_pop_acquire
 
+#ifndef AO_DOUBLE_ALIGN
+  /* For AO_stack clients only. */
+# define AO_DOUBLE_ALIGN /* empty */
+#endif
+
 # else /* Use fully non-blocking data structure, wide CAS       */
 
 #ifndef AO_HAVE_double_t
@@ -156,6 +161,7 @@ AO_INLINE void AO_stack_init(AO_stack_t *list)
 
 typedef volatile AO_double_t AO_stack_t;
 /* AO_val1 is version, AO_val2 is pointer.      */
+/* AO_stack_t variables should have AO_DOUBLE_ALIGN attribute.  */
 
 #define AO_STACK_INITIALIZER AO_DOUBLE_T_INITIALIZER
 
diff --git a/tests/test_atomic_include.template b/tests/test_atomic_include.template
index a9e1895..f05aba4 100644
--- a/tests/test_atomic_include.template
+++ b/tests/test_atomic_include.template
@@ -36,13 +36,13 @@ void test_atomicXX(void)
 # if defined(AO_HAVE_double_compare_and_swapXX) \
      || defined(AO_HAVE_double_loadXX) \
      || defined(AO_HAVE_double_storeXX)
-    AO_double_t old_w;
+    AO_double_t AO_DOUBLE_ALIGN old_w;
     AO_double_t new_w;
 # endif
 # if defined(AO_HAVE_compare_and_swap_doubleXX) \
      || defined(AO_HAVE_compare_double_and_swap_doubleXX) \
      || defined(AO_HAVE_double_compare_and_swapXX)
-    AO_double_t w;
+    AO_double_t AO_DOUBLE_ALIGN w;
     w.AO_val1 = 0;
     w.AO_val2 = 0;
 # endif
diff --git a/tests/test_stack.c b/tests/test_stack.c
index 0847c11..3da075f 100644
--- a/tests/test_stack.c
+++ b/tests/test_stack.c
@@ -79,7 +79,7 @@ typedef struct le {
   int data;
 } list_element;
 
-AO_stack_t the_list = AO_STACK_INITIALIZER;
+AO_stack_t AO_DOUBLE_ALIGN the_list = AO_STACK_INITIALIZER;
 
 void add_elements(int n)
 {