Daniel proposed in [1] that we introduce apr_atomic_casptr2() to
resolve the defect in the apr_atomic_casptr() API.
Namely the correct:
1. APR_DECLARE(void*) apr_atomic_casptr(void *volatile *mem, ...);
Versus the broken:
2. APR_DECLARE(void*) apr_atomic_casptr(volatile void **mem, ...);
The problem with 2 is that it does not prevent the compiler from
caching *mem into a local variable, which would break atomicity.
This is fixed in trunk already where we could change the API to 1, but
not in 1.x.
Anyone opposed to the attached patch (for 1.8.x only then)?
Regards;
Yann.
[1] https://bz.apache.org/bugzilla/show_bug.cgi?id=50731#c2
Index: atomic/netware/apr_atomic.c
===================================================================
--- atomic/netware/apr_atomic.c (revision 1902233)
+++ atomic/netware/apr_atomic.c (working copy)
@@ -72,6 +72,11 @@ APR_DECLARE(void *) apr_atomic_casptr(volatile voi
return (void*)atomic_cmpxchg((unsigned long *)mem,(unsigned long)cmp,(unsigned long)with);
}
+APR_DECLARE(void *) apr_atomic_casptr2(void *volatile *mem, void *with, const void *cmp)
+{
+ return (void*)atomic_cmpxchg((unsigned long *)mem,(unsigned long)cmp,(unsigned long)with);
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)
{
return (void*)atomic_xchg((unsigned long *)mem,(unsigned long)with);
Index: atomic/os390/atomic.c
===================================================================
--- atomic/os390/atomic.c (revision 1902233)
+++ atomic/os390/atomic.c (working copy)
@@ -94,6 +94,15 @@ void *apr_atomic_casptr(volatile void **mem_ptr,
&swap_ptr);
return (void *)cmp_ptr;
}
+void *apr_atomic_casptr2(void *volatile *mem_ptr,
+ void *swap_ptr,
+ const void *cmp_ptr)
+{
+ __cs1(&cmp_ptr, /* automatically updated from mem on __cs1 failure */
+ mem_ptr, /* set from swap when __cs1 succeeds */
+ &swap_ptr);
+ return (void *)cmp_ptr;
+}
#elif APR_SIZEOF_VOIDP == 8
void *apr_atomic_casptr(volatile void **mem_ptr,
void *swap_ptr,
@@ -104,6 +113,15 @@ void *apr_atomic_casptr(volatile void **mem_ptr,
&swap_ptr);
return (void *)cmp_ptr;
}
+void *apr_atomic_casptr2(void *volatile *mem_ptr,
+ void *swap_ptr,
+ const void *cmp_ptr)
+{
+ __csg(&cmp_ptr, /* automatically updated from mem on __csg failure */
+ mem_ptr, /* set from swap when __csg succeeds */
+ &swap_ptr);
+ return (void *)cmp_ptr;
+}
#else
#error APR_SIZEOF_VOIDP value not supported
#endif /* APR_SIZEOF_VOIDP */
Index: atomic/unix/builtins.c
===================================================================
--- atomic/unix/builtins.c (revision 1902234)
+++ atomic/unix/builtins.c (working copy)
@@ -121,6 +121,16 @@ APR_DECLARE(void*) apr_atomic_casptr(volatile void
#endif
}
+APR_DECLARE(void*) apr_atomic_casptr2(void *volatile *mem, void *ptr, const void *cmp)
+{
+#if HAVE__ATOMIC_BUILTINS
+ __atomic_compare_exchange_n(mem, (void *)&cmp, ptr, 0, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
+ return (void *)cmp;
+#else
+ return (void *)__sync_val_compare_and_swap(mem, (void *)cmp, ptr);
+#endif
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *ptr)
{
#if HAVE__ATOMIC_BUILTINS
Index: atomic/unix/ia32.c
===================================================================
--- atomic/unix/ia32.c (revision 1902233)
+++ atomic/unix/ia32.c (working copy)
@@ -111,6 +111,24 @@ APR_DECLARE(void*) apr_atomic_casptr(volatile void
return prev;
}
+APR_DECLARE(void*) apr_atomic_casptr2(void *volatile *mem, void *with, const void *cmp)
+{
+ void *prev;
+#if APR_SIZEOF_VOIDP == 4
+ asm volatile ("lock; cmpxchgl %2, %1"
+ : "=a" (prev), "=m" (*mem)
+ : "r" (with), "m" (*mem), "0" (cmp));
+#elif APR_SIZEOF_VOIDP == 8
+ asm volatile ("lock; cmpxchgq %q2, %1"
+ : "=a" (prev), "=m" (*mem)
+ : "r" ((unsigned long)with), "m" (*mem),
+ "0" ((unsigned long)cmp));
+#else
+#error APR_SIZEOF_VOIDP value not supported
+#endif
+ return prev;
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)
{
void *prev;
Index: atomic/unix/mutex.c
===================================================================
--- atomic/unix/mutex.c (revision 1902233)
+++ atomic/unix/mutex.c (working copy)
@@ -190,6 +190,21 @@ APR_DECLARE(void*) apr_atomic_casptr(volatile void
return prev;
}
+APR_DECLARE(void*) apr_atomic_casptr(void *volatile *mem, void *with, const void *cmp)
+{
+ void *prev;
+ DECLARE_MUTEX_LOCKED(mutex, *mem);
+
+ prev = *(void **)mem;
+ if (prev == cmp) {
+ *mem = with;
+ }
+
+ MUTEX_UNLOCK(mutex);
+
+ return prev;
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)
{
void *prev;
Index: atomic/unix/ppc.c
===================================================================
--- atomic/unix/ppc.c (revision 1902233)
+++ atomic/unix/ppc.c (working copy)
@@ -208,6 +208,43 @@ APR_DECLARE(void*) apr_atomic_casptr(volatile void
return prev;
}
+APR_DECLARE(void*) apr_atomic_casptr2(void *volatile *mem, void *with, const void *cmp)
+{
+ void *prev;
+#if APR_SIZEOF_VOIDP == 4
+ asm volatile (" sync\n" /* full barrier */
+ "1:\n" /* lost reservation */
+ " lwarx %0,0,%1\n" /* load and reserve */
+ " cmpw %0,%3\n" /* compare operands */
+ " bne- 2f\n" /* skip if not equal */
+ PPC405_ERR77_SYNC /* ppc405 Erratum 77 */
+ " stwcx. %2,0,%1\n" /* store new value */
+ " bne- 1b\n" /* loop if lost */
+ "2:\n" /* not equal */
+ " isync\n" /* acquire barrier (bc+isync) */
+ : "=&r" (prev)
+ : "b" (mem), "r" (with), "r" (cmp)
+ : "cc", "memory");
+#elif APR_SIZEOF_VOIDP == 8
+ asm volatile (" sync\n" /* full barrier */
+ "1:\n" /* lost reservation */
+ " ldarx %0,0,%1\n" /* load and reserve */
+ " cmpd %0,%3\n" /* compare operands */
+ " bne- 2f\n" /* skip if not equal */
+ PPC405_ERR77_SYNC /* ppc405 Erratum 77 */
+ " stdcx. %2,0,%1\n" /* store new value */
+ " bne- 1b\n" /* loop if lost */
+ "2:\n" /* not equal */
+ " isync\n" /* acquire barrier (bc+isync) */
+ : "=&r" (prev)
+ : "b" (mem), "r" (with), "r" (cmp)
+ : "cc", "memory");
+#else
+#error APR_SIZEOF_VOIDP value not supported
+#endif
+ return prev;
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)
{
void *prev;
Index: atomic/unix/s390.c
===================================================================
--- atomic/unix/s390.c (revision 1902233)
+++ atomic/unix/s390.c (working copy)
@@ -133,6 +133,25 @@ APR_DECLARE(void*) apr_atomic_casptr(volatile void
return prev;
}
+APR_DECLARE(void*) apr_atomic_casptr2(void *volatile *mem, void *with, const void *cmp)
+{
+ void *prev = (void *) cmp;
+#if APR_SIZEOF_VOIDP == 4
+ asm volatile (" cs %0,%2,%1\n"
+ : "+d" (prev), "=Q" (*mem)
+ : "d" (with), "m" (*mem)
+ : "cc", "memory");
+#elif APR_SIZEOF_VOIDP == 8
+ asm volatile (" csg %0,%2,%1\n"
+ : "+d" (prev), "=Q" (*mem)
+ : "d" (with), "m" (*mem)
+ : "cc", "memory");
+#else
+#error APR_SIZEOF_VOIDP value not supported
+#endif
+ return prev;
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)
{
void *prev = (void *) *mem;
Index: atomic/unix/solaris.c
===================================================================
--- atomic/unix/solaris.c (revision 1902233)
+++ atomic/unix/solaris.c (working copy)
@@ -75,6 +75,11 @@ APR_DECLARE(void*) apr_atomic_casptr(volatile void
return atomic_cas_ptr(mem, (void*) cmp, with);
}
+APR_DECLARE(void*) apr_atomic_casptr2(void *volatile *mem, void *with, const void *cmp)
+{
+ return atomic_cas_ptr(mem, (void*) cmp, with);
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)
{
return atomic_swap_ptr(mem, with);
Index: atomic/win32/apr_atomic.c
===================================================================
--- atomic/win32/apr_atomic.c (revision 1902233)
+++ atomic/win32/apr_atomic.c (working copy)
@@ -100,6 +100,15 @@ APR_DECLARE(void *) apr_atomic_casptr(volatile voi
#endif
}
+APR_DECLARE(void *) apr_atomic_casptr2(void *volatile *mem, void *with, const void *cmp)
+{
+#if (defined(_M_IA64) || defined(_M_AMD64)) && !defined(RC_INVOKED)
+ return InterlockedCompareExchangePointer(mem, with, (void*)cmp);
+#else
+ return InterlockedCompareExchangePointer((void**)mem, with, (void*)cmp);
+#endif
+}
+
APR_DECLARE(apr_uint32_t) apr_atomic_xchg32(volatile apr_uint32_t *mem, apr_uint32_t val)
{
#if (defined(_M_IA64) || defined(_M_AMD64)) && !defined(RC_INVOKED)
Index: include/apr_atomic.h
===================================================================
--- include/apr_atomic.h (revision 1902233)
+++ include/apr_atomic.h (working copy)
@@ -187,10 +187,23 @@ APR_DECLARE(apr_uint64_t) apr_atomic_xchg64(volati
* @param with what to swap it with
* @param cmp the value to compare it to
* @return the old value of the pointer
+ * @warning The API of this function is not correct, it does not prevent the
+ * compiler from caching the pointer in *mem, possibly breaking the
+ * atomic garantees. Use apr_atomic_casptr2() instead.
*/
APR_DECLARE(void*) apr_atomic_casptr(volatile void **mem, void *with, const void *cmp);
/**
+ * compare the pointer's value with cmp.
+ * If they are the same swap the value with 'with'
+ * @param mem pointer to the pointer
+ * @param with what to swap it with
+ * @param cmp the value to compare it to
+ * @return the old value of the pointer
+ */
+APR_DECLARE(void*) apr_atomic_casptr2(void *volatile *mem, void *with, const void *cmp);
+
+/**
* exchange a pair of pointer values
* @param mem pointer to the pointer
* @param with what to swap it with
Index: test/testatomic.c
===================================================================
--- test/testatomic.c (revision 1902233)
+++ test/testatomic.c (working copy)
@@ -126,10 +126,15 @@ static void test_cas_notequal(abts_case *tc, void
static void test_casptr_equal(abts_case *tc, void *data)
{
int a = 0;
- volatile void *target_ptr = NULL;
+ void *target_ptr = NULL;
void *old_ptr;
- old_ptr = apr_atomic_casptr(&target_ptr, &a, NULL);
+ if (data) {
+ old_ptr = apr_atomic_casptr2(&target_ptr, &a, NULL);
+ }
+ else {
+ old_ptr = apr_atomic_casptr((void *)&target_ptr, &a, NULL);
+ }
ABTS_PTR_EQUAL(tc, NULL, old_ptr);
ABTS_PTR_EQUAL(tc, (void *)&a, (void *)target_ptr);
}
@@ -137,23 +142,33 @@ static void test_casptr_equal(abts_case *tc, void
static void test_casptr_equal_nonnull(abts_case *tc, void *data)
{
int a = 0, b = 0;
- volatile void *target_ptr = &a;
+ void *target_ptr = &a;
void *old_ptr;
- old_ptr = apr_atomic_casptr(&target_ptr, &b, &a);
+ if (data) {
+ old_ptr = apr_atomic_casptr2(&target_ptr, &b, &a);
+ }
+ else {
+ old_ptr = apr_atomic_casptr((void *)&target_ptr, &b, &a);
+ }
ABTS_PTR_EQUAL(tc, (void *)&a, old_ptr);
- ABTS_PTR_EQUAL(tc, (void *)&b, (void *)target_ptr);
+ ABTS_PTR_EQUAL(tc, (void *)&b, target_ptr);
}
static void test_casptr_notequal(abts_case *tc, void *data)
{
int a = 0, b = 0;
- volatile void *target_ptr = &a;
+ void *target_ptr = &a;
void *old_ptr;
- old_ptr = apr_atomic_casptr(&target_ptr, &a, &b);
+ if (data) {
+ old_ptr = apr_atomic_casptr2(&target_ptr, &a, &b);
+ }
+ else {
+ old_ptr = apr_atomic_casptr((void *)&target_ptr, &a, &b);
+ }
ABTS_PTR_EQUAL(tc, (void *)&a, old_ptr);
- ABTS_PTR_EQUAL(tc, (void *)&a, (void *)target_ptr);
+ ABTS_PTR_EQUAL(tc, (void *)&a, target_ptr);
}
static void test_add32(abts_case *tc, void *data)
@@ -929,8 +944,11 @@ abts_suite *testatomic(abts_suite *suite)
abts_run_test(suite, test_cas_equal_nonnull, NULL);
abts_run_test(suite, test_cas_notequal, NULL);
abts_run_test(suite, test_casptr_equal, NULL);
+ abts_run_test(suite, test_casptr_equal, (void *)(apr_uintptr_t)2);
abts_run_test(suite, test_casptr_equal_nonnull, NULL);
+ abts_run_test(suite, test_casptr_equal_nonnull, (void *)(apr_uintptr_t)2);
abts_run_test(suite, test_casptr_notequal, NULL);
+ abts_run_test(suite, test_casptr_notequal, (void *)(apr_uintptr_t)2);
abts_run_test(suite, test_add32, NULL);
abts_run_test(suite, test_add32_neg, NULL);
abts_run_test(suite, test_inc32, NULL);
