NetBSD Problem Report #49995
From www@NetBSD.org Tue Jun 23 10:08:14 2015
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
by mollari.NetBSD.org (Postfix) with ESMTPS id F0EB0A65BC
for <gnats-bugs@gnats.NetBSD.org>; Tue, 23 Jun 2015 10:08:14 +0000 (UTC)
Message-Id: <20150623100813.73883A65BE@mollari.NetBSD.org>
Date: Tue, 23 Jun 2015 10:08:13 +0000 (UTC)
From: isaki@NetBSD.org
Reply-To: isaki@NetBSD.org
To: gnats-bugs@NetBSD.org
Subject: m68k: atomic_cas_N and __sync_bool_compare_and_swap_N doesn't work correctly
X-Send-Pr-Version: www-1.0
>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: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Tue Jun 23 10:10:00 +0000 2015
>Closed-Date: Sat Jul 04 07:12:28 +0000 2015
>Last-Modified: Thu Jul 16 21:50:01 +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 */
>Release-Note:
>Audit-Trail:
From: "Tetsuya Isaki" <isaki@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/49995 CVS commit: src/common/lib/libc/arch/m68k/atomic
Date: Sat, 4 Jul 2015 06:56:29 +0000
Module Name: src
Committed By: isaki
Date: Sat Jul 4 06:56:29 UTC 2015
Modified Files:
src/common/lib/libc/arch/m68k/atomic: atomic_cas.S
Log Message:
atomic_cas_{8,16}:
- Correct the wrong offset in stack.
__sync_bool_compare_and_swap_{1,2,4}:
- Correct the wrong offset in stack.
- D3 must be preserved in subroutines.
PR/49995.
To generate a diff of this commit:
cvs rdiff -u -r1.10 -r1.11 src/common/lib/libc/arch/m68k/atomic/atomic_cas.S
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->closed
State-Changed-By: isaki@NetBSD.org
State-Changed-When: Sat, 04 Jul 2015 07:12:28 +0000
State-Changed-Why:
Commited.
From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/49995 CVS commit: [netbsd-7] src/common/lib/libc/arch/m68k/atomic
Date: Thu, 16 Jul 2015 21:45:52 +0000
Module Name: src
Committed By: snj
Date: Thu Jul 16 21:45:52 UTC 2015
Modified Files:
src/common/lib/libc/arch/m68k/atomic [netbsd-7]: atomic_cas.S
Log Message:
Pull up following revision(s) (requested by isaki in ticket #865):
common/lib/libc/arch/m68k/atomic/atomic_cas.S: revisions 1.11, 1.12
atomic_cas_{8,16}:
- Correct the wrong offset in stack.
__sync_bool_compare_and_swap_{1,2,4}:
- Correct the wrong offset in stack.
- D3 must be preserved in subroutines.
PR/49995.
--
Improve the code in __sync_bool_compare_and_swap_{1,2,4}.
- bccs is smaller and faster than bcc(.w) in this case.
- it can be used movql in this case (assembler optimise it though).
To generate a diff of this commit:
cvs rdiff -u -r1.10 -r1.10.4.1 \
src/common/lib/libc/arch/m68k/atomic/atomic_cas.S
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.