lib/49995: m68k: atomic_cas_N and __sync_bool_compare_and_swap_N doesn't work correctly

[isaki%NetBSD.org@localhost]

>Number:         49995
>Category:       lib
>Synopsis:       m68k: atomic_cas_N and __sync_bool_compare_and_swap_N doesn't work correctly
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jun 23 10:10:00 +0000 2015
>Originator:     Tetsuya Isaki
>Release:        NetBSD-7.0_RC1
>Organization:
>Environment:
NetBSD XXXXX 7.0_RC1 NetBSD 7.0_RC1 (GENERIC) #3: Sun Jun 21 14:08:10 JST 2015  isaki@XXXXX:/var/obj/7/x68k/obj/sys/arch/x68k/compile/GENERIC x68k
>Description:
On m68k, atomic_cas_{8,16}
(in src/common/lib/libc/arch/m68k/atomic/atomic_cas.S) does not
work correctly.  It's wrong that the offset retrieving the arguments
from the stack.  The arguments are pushed in Long (= 4 byte) by
caller even if the argument's size is Byte or Word.  It can be
reproduced easily by calling atomic_cas_{8,16} in equal case.

__sync_bool_compare_and_swap_{1,2} has the same problem as above.
In addition, __sync_bool_compare_and_swap_{1,2,4} break D3 register
which should be preserved in subroutines.
By the way, I could not reproduce it with gcc on netbsd-7, because
gcc 4.8 has its own built-in (and correct :)
__sync_bool_compare_and_swap_N functions and I didn't know how to
disable it (--fno-builtin seems to have no effect for this).
Instead, I reproduced it using pcc (pkgsrc/lang/pcc-current) which
does not have these built-in functions.

I confirmed it only on 7.0_BETA/RC1 but the -current has the same
problem.

The Patch is attached.  May I commit it?
>How-To-Repeat:
Here is the test code.
--------------------------------------
#include <stdio.h>
#include <stdint.h>
#include <sys/atomic.h>

#if defined(__m68k__)
#define SAVE_D3()	__asm__("movl %%d3,%0" : "=m"(d3before))
#define CHECK_D3()	do {	\
	__asm__("movl %%d3,%0" : "=m"(d3after));	\
	if (d3before != d3after) \
		printf("\tD3 is broken\n"),errcnt++;	\
} while (0)
#else
#define SAVE_D3()
#define CHECK_D3()
#endif

#define SET(r, t, o, n)	do {	\
	rv = r;	\
	target = t;	\
	oldval = o;	\
	newval = n;	\
	SAVE_D3();	\
} while (0)

#define CHECK(r, t, o, n)	do { \
	CHECK_D3();	\
	if (rv != r) \
		printf("\trv expects %d but %d\n", r, rv),errcnt++;	\
	if (target != t) \
		printf("\ttarget expects %d but %d\n", t, target),errcnt++;\
	if (oldval != o) \
		printf("\toldval expects %d but %d\n", o, oldval),errcnt++;\
	if (newval != n) \
		printf("\tnewval expects %d but %d\n", n, newval),errcnt++;\
} while (0)

volatile uint32_t d3before;
volatile uint32_t d3after;
int errcnt;

void
test32()
{
	register uint32_t oldval;
	register uint32_t newval;
	volatile uint32_t target;
	uint32_t rv;

	printf("atomic_cas_32 (EQ)\n");
	SET(0, 10, 10, 30);
	rv = atomic_cas_32(&target, oldval, newval);
	CHECK(10, 30, 10, 30);

	printf("atomic_cas_32 (NE)\n");
	SET(0, 10, 20, 30);
	rv = atomic_cas_32(&target, oldval, newval);
	CHECK(10, 10, 20, 30);

	printf("__sync_bool_compare_and_swap_4 (EQ)\n");
	SET(0, 10, 10, 30);
	rv = __sync_bool_compare_and_swap_4(&target, oldval, newval);
	CHECK(1, 30, 10, 30);

	printf("__sync_bool_compare_and_swap_4 (NE)\n");
	SET(0, 10, 20, 30);
	rv = __sync_bool_compare_and_swap_4(&target, oldval, newval);
	CHECK(0, 10, 20, 30);
}

void
test16()
{
#if defined(__m68k__)
	register uint16_t oldval;
	register uint16_t newval;
	volatile uint16_t target;
	uint16_t rv;

	printf("atomic_cas_16 (EQ)\n");
	SET(0, 10, 10, 30);
	rv = atomic_cas_16(&target, oldval, newval);
	CHECK(10, 30, 10, 30);

	printf("atomic_cas_16 (NE)\n");
	SET(0, 10, 20, 30);
	rv = atomic_cas_16(&target, oldval, newval);
	CHECK(10, 10, 20, 30);

	printf("__sync_bool_compare_and_swap_2 (EQ)\n");
	SET(0, 10, 10, 30);
	rv = __sync_bool_compare_and_swap_2(&target, oldval, newval);
	CHECK(1, 30, 10, 30);

	printf("__sync_bool_compare_and_swap_2 (NE)\n");
	SET(0, 10, 20, 30);
	rv = __sync_bool_compare_and_swap_2(&target, oldval, newval);
	CHECK(0, 10, 20, 30);
#endif
}

void
test8()
{
#if defined(__m68k__)
	register uint8_t oldval;
	register uint8_t newval;
	volatile uint8_t target;
	uint8_t rv;

	printf("atomic_cas_8 (EQ)\n");
	SET(0, 10, 10, 30);
	rv = atomic_cas_8(&target, oldval, newval);
	CHECK(10, 30, 10, 30);

	printf("atomic_cas_8 (NE)\n");
	SET(0, 10, 20, 30);
	rv = atomic_cas_8(&target, oldval, newval);
	CHECK(10, 10, 20, 30);

	printf("__sync_bool_compare_and_swap_1 (EQ)\n");
	SET(0, 10, 10, 30);
	rv = __sync_bool_compare_and_swap_1(&target, oldval, newval);
	CHECK(1, 30, 10, 30);

	printf("__sync_bool_compare_and_swap_1 (NE)\n");
	SET(0, 10, 20, 30);
	rv = __sync_bool_compare_and_swap_1(&target, oldval, newval);
	CHECK(0, 10, 20, 30);
#endif
}

int
main()
{
	test32();
	test16();
	test8();
	printf("%d failed\n", errcnt);
	return 0;
}
--------------------------------------

And here is the log of the test code BEFORE applying the patch.
--------------------------------------
xm6i7# uname -srm
NetBSD 7.0_RC1 x68k

xm6i7# gcc -O2 cas1.c -o cas1-O2
xm6i7# ./cas1-O2
atomic_cas_32 (EQ)
atomic_cas_32 (NE)
__sync_bool_compare_and_swap_4 (EQ)
__sync_bool_compare_and_swap_4 (NE)
atomic_cas_16 (EQ)
	target expects 30 but 10
atomic_cas_16 (NE)
__sync_bool_compare_and_swap_2 (EQ)
__sync_bool_compare_and_swap_2 (NE)
atomic_cas_8 (EQ)
	target expects 30 but 10
atomic_cas_8 (NE)
__sync_bool_compare_and_swap_1 (EQ)
__sync_bool_compare_and_swap_1 (NE)
2 failed

xm6i7# pcc --version
pcc 1.2.0.DEVEL 20141228 for m68k--netbsdelf
xm6i7# pcc -O2 cas1.c -o cas1-pcc-O2
xm6i7# ./cas1-pcc-O2
atomic_cas_32 (EQ)
atomic_cas_32 (NE)
__sync_bool_compare_and_swap_4 (EQ)
	D3 is broken
	newval expects 30 but 10
__sync_bool_compare_and_swap_4 (NE)
	D3 is broken
	newval expects 30 but 10
atomic_cas_16 (EQ)
	target expects 30 but 10
atomic_cas_16 (NE)
__sync_bool_compare_and_swap_2 (EQ)
	D3 is broken
	rv expects 1 but 0
	target expects 30 but 10
	newval expects 30 but 10
__sync_bool_compare_and_swap_2 (NE)
	D3 is broken
	newval expects 30 but 10
atomic_cas_8 (EQ)
	target expects 30 but 10
atomic_cas_8 (NE)
__sync_bool_compare_and_swap_1 (EQ)
	D3 is broken
	rv expects 1 but 0
	target expects 30 but 10
	newval expects 30 but 10
__sync_bool_compare_and_swap_1 (NE)
	D3 is broken
	newval expects 30 but 10
18 failed
--------------------------------------

>Fix:
--- common/lib/libc/arch/m68k/atomic/atomic_cas.S.ORG	2014-08-17 14:16:52.000000000 +0900
+++ common/lib/libc/arch/m68k/atomic/atomic_cas.S	2015-06-22 18:09:46.000000000 +0900
@@ -63,11 +63,9 @@
 
 ENTRY(__sync_bool_compare_and_swap_4)
 	movl	4(%sp), %a0
-	movl	8(%sp), %d3
-	movl	%d3, %d2
+	movl	8(%sp), %d0
 	movl	12(%sp), %d1
-	casl	%d3, %d1, (%a0)
-	/* %d3 now contains the old value */
+	casl	%d0, %d1, (%a0)
 	beq	1f
 	clrl	%d0	/* return false */
 	rts
@@ -77,8 +75,8 @@
 
 ENTRY(_atomic_cas_16)
 	movl	4(%sp), %a0
-	movw	8(%sp), %d0
-	movw	10(%sp), %d1
+	movw	8+2(%sp), %d0		/* lower word */
+	movw	12+2(%sp), %d1		/* lower word */
 	casw	%d0, %d1, (%a0)
 	/* %d0 now contains the old value */
 	rts
@@ -89,10 +87,9 @@
 
 ENTRY(__sync_bool_compare_and_swap_2)
 	movl	4(%sp), %a0
-	movw	8(%sp), %d3
-	movw	%d3, %d2
-	movw	10(%sp), %d1
-	casw	%d3, %d1, (%a0)
+	movw	8+2(%sp), %d0		/* lower word */
+	movw	12+2(%sp), %d1		/* lower word */
+	casw	%d0, %d1, (%a0)
 	/* %d3 now contains the old value */
 	beq	1f
 	clrl	%d0	/* return failure */
@@ -103,8 +100,8 @@
 
 ENTRY(_atomic_cas_8)
 	movl	4(%sp), %a0
-	movb	8(%sp), %d0
-	movb	9(%sp), %d1
+	movb	8+3(%sp), %d0		/* lower byte */
+	movb	12+3(%sp), %d1		/* lower byte */
 	casb	%d0, %d1, (%a0)
 	/* %d0 now contains the old value */
 	rts
@@ -116,10 +113,9 @@
 
 ENTRY(__sync_bool_compare_and_swap_1)
 	movl	4(%sp), %a0
-	movb	8(%sp), %d3
-	movb	%d3, %d2
-	movb	9(%sp), %d1
-	casb	%d3, %d1, (%a0)
+	movb	8+3(%sp), %d0		/* lower byte */
+	movb	12+3(%sp), %d1		/* lower byte */
+	casb	%d0, %d1, (%a0)
 	/* %d3 now contains the old value */
 	beq	1f
 	clrl	%d0	/* return failure */