gcc optimizer miscompiles code with sigaddset on i386 ?

gcc optimizer miscompiles code with sigaddset on i386 ?

[Constantine Gavrilov]

[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]

Hello:

I have run into problem using sigaddset() in kernel code. The problem is 
reproducible in user space with kernel sigaddset definition.

I have a problem where a code like the following gets miscompiled:

sigset_t a;
sigset_t b;
sigset_t c;

...........
sigaddset(&a, const1);
sigaddset(&a,  const2);
................................
b =a;    /* this line causes incorrect code; any 64-bit assignment seems 
to cause a problem! */
sigaddset(&c, const2);
sigaddset(&c,  const3);
/* more sigaddset to c */

At some iteration of sigaddset to c, c gets the value of a!

Again, the problem is reproducible in user space. Both gcc 2.96 and gcc 
3.2.3 are affected. So, the problem should be relevant for both 2.4 and 
2.6 kernels.
Attached please find a C code that that demonstrates the problem. It can 
be compiled to user space and kernel module . When compiled without 
optimization or with "-O", the problem goes away. When replacing 
sigaddset with set_bit, the problem goes away as well. Trying to run the 
problematic code in the debugger shows totally wild behavior. I have not 
analyzed the disassembly yet, so I cannot say what goes wrong. It seems 
a workaround to this problem is required in kernel source as it 
potentially breaks signal code. Is sigaddset misses some registers in 
clobbered list? Anybody is willing to crack a workaround?

THX.

-- 
----------------------------------------
Constantine Gavrilov
Kernel Developer
Qlusters Software Ltd
1 Azrieli Center, Tel-Aviv
Phone: +972-3-6081977
Fax:   +972-3-6081841
----------------------------------------


[-- Attachment #2: sigset_test.c --]
[-- Type: text/plain, Size: 3839 bytes --]

/***************************************************************************
                          sigset_test.c  -  description
                             -------------------
    begin                : Wed Nov 16 2005
    copyright            : (C) 2005 by Qlusters Ltd
    email                : linux@qlusters.com
 ***************************************************************************/
#ifdef  DEF_KERNEL
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/mm.h>
#include <linux/signal.h>

static int sigset_test_init(void);
static void sigset_test_exit(void);

module_init(sigset_test_init);
module_exit(sigset_test_exit);
#define SIGADDSET sigaddset
#define SIGISMEMBER sigismember
//#define  SIGADDSET(x, y) set_bit(y, (long *)x)

#else
extern int printf(const char *format, ...);
#define printk printf
#define KERN_INFO
typedef struct {
	unsigned long sig[2];
} sigset_t;
#include <asm/signal.h>
#define _NSIG (64)
#define _NSIG_BPW (32)

static __inline__ void SIGADDSET(sigset_t *set, int _sig)
{
	__asm__("btsl %1,%0" : "=m"(*set) : "Ir"(_sig - 1) : "cc");
}

static __inline__ int __const_sigismember(sigset_t *set, int _sig)
{
	unsigned long sig = _sig - 1;
	return 1 & (set->sig[sig / _NSIG_BPW] >> (sig % _NSIG_BPW));
}
static __inline__ int __gen_sigismember(sigset_t *set, int _sig)
{
	int ret;
	__asm__("btl %2,%1\n\tsbbl %0,%0"
		: "=r"(ret) : "m"(*set), "Ir"(_sig-1) : "cc");
	return ret;
}

#define SIGISMEMBER(set,sig)               \
	(__builtin_constant_p(sig) ?           \
	__const_sigismember((set),(sig)) :     \
	__gen_sigismember((set),(sig)))

#endif



sigset_t ssi_fatal_sigset;
static sigset_t ssi_stop_set;
static void print_pending_signals(sigset_t *pending, sigset_t *blocked);

void sigset_test_exit(void)
{
	return;
}

#ifdef  DEF_KERNEL
static int sigset_test_init()
#else
int main()
#endif
{
	sigset_t cleaned;

	cleaned = (sigset_t) { {0,  0} };
	/* these are non-fatal signals.  Other signals are if their action is SIG_DFL */
	SIGADDSET(&cleaned, SIGCONT);
	SIGADDSET(&cleaned, SIGCHLD);
	SIGADDSET(&cleaned, SIGWINCH);
	SIGADDSET(&cleaned, SIGURG);
	SIGADDSET(&cleaned, SIGTSTP);
	SIGADDSET(&cleaned, SIGTTIN);
	SIGADDSET(&cleaned, SIGTTOU);
	SIGADDSET(&cleaned, SIGSTOP);
	ssi_fatal_sigset =  (sigset_t) { {-1,  -1} };

	//uncomenting this line fixes the problem
	//printk(KERN_INFO "sigset_test: fatal %p, stop %p\n", &ssi_fatal_sigset, &ssi_stop_set);

	/* ssi_fatal_sigset is a mask of all signals that are fatal */
	//commenting the line below fixes the problem
	ssi_fatal_sigset = cleaned;

	ssi_stop_set = (sigset_t) { {0,  0} };
	printk(KERN_INFO "sigset_test after zeroing\n");
	print_pending_signals(&ssi_stop_set, NULL);

	SIGADDSET(&ssi_stop_set, SIGTSTP);
	printk(KERN_INFO "sigset_test after %d\n", SIGTSTP);
	print_pending_signals(&ssi_stop_set, NULL);

	SIGADDSET(&ssi_stop_set, SIGTTIN);
	printk(KERN_INFO "sigset_test after %d\n", SIGTTIN);
	print_pending_signals(&ssi_stop_set, NULL);

	SIGADDSET(&ssi_stop_set, SIGTTOU);
	printk(KERN_INFO "sigset_test after %d\n", SIGTTOU);
	print_pending_signals(&ssi_stop_set, NULL);

	SIGADDSET(&ssi_stop_set, SIGSTOP);
	printk(KERN_INFO "sigset_test after %d\n", SIGSTOP);
	print_pending_signals(&ssi_stop_set, NULL);
	return 1;
}

static void print_pending_signals(sigset_t *pending, sigset_t *blocked)
{
	int i;

	if(pending->sig[0] == 0 && pending->sig[1] == 0) {
		printk(KERN_INFO "sigset_t: no set signals\n");
		return;
	}
	if(blocked) {
		for(i = 0; i < _NSIG; i++) {
			if(SIGISMEMBER(pending, i+1)) {
				printk(KERN_INFO "sigset_test: set signal %d is%sblocked\n", i+1,  SIGISMEMBER(blocked, i+1) ? " " : " not ");
			}
		}
	} else {
		 for(i = 0; i < _NSIG; i++) {
			if(SIGISMEMBER(pending, i+1)) {
				printk(KERN_INFO "sigset_test: set signal %d\n", i+1);
			}
		}
	}
}

gcc optimizer miscompiles code with sigaddset on i386 if sig arg is const -- PATCH proposed

[Constantine Gavrilov]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

I have run into problem using sigaddset() with constant signal argument 
in kernel code.

A code like the following gets miscompiled:

sigset_t a;
sigset_t b;
sigset_t c;

...........
sigaddset(&a, const1);
sigaddset(&a,  const2);
................................
b =a;    /* this line causes incorrect code; any 64-bit assignment seems 
to cause a problem! */
sigaddset(&c, const2);
sigaddset(&c,  const3);
/* more sigaddset to c */

All versions of compilers and all kernel versions are affected. The 
problem is also reproducible in user space (with kernel sigaddset 
definition). Looking at the dissassembly (or running in the debugger) 
clearly shows that gcc miscompiled the code.

Attached please find a C code (can be compiled into module or 
executable) that demonstrates the problem. Attached is also a patch that 
fixes the problem (the patch was prepared against 2.6 tree).

I defined sigaddset a macro (similar to sigismember) that calls one 
function for constant argument and another for variable. In addition to 
fixing the problem, it also makes sigaddset() faster for constant arguments.

To be on the safe side, I changed sigdelset() in the same way (I think 
the same gcc bug may apply).


-- 
----------------------------------------
Constantine Gavrilov
Kernel Developer
Qlusters Software Ltd
1 Azrieli Center, Tel-Aviv
Phone: +972-3-6081977
Fax:   +972-3-6081841
----------------------------------------


[-- Attachment #2: sigset_test.c --]
[-- Type: text/plain, Size: 4253 bytes --]

/***************************************************************************
                          sigset_test.c  -  description
                             -------------------
    begin                : Wed Nov 16 2005
    copyright            : (C) 2005 by Qlusters Ltd
    email                : linux@qlusters.com
 ***************************************************************************/
#ifdef  DEF_KERNEL
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/mm.h>
#include <linux/signal.h>

static int sigset_test_init(void);
static void sigset_test_exit(void);

module_init(sigset_test_init);
module_exit(sigset_test_exit);
#define SIGADDSET sigaddset
#define SIGISMEMBER sigismember
//#define  SIGADDSET(x, y) set_bit(y, (long *)x)

#else
extern int printf(const char *format, ...);
#define printk printf
#define KERN_INFO
typedef struct {
	unsigned long sig[2];
} sigset_t;
#include <asm/signal.h>
#define _NSIG (64)
#define _NSIG_BPW (32)


static __inline__ int __const_sigismember(sigset_t *set, int _sig)
{
	unsigned long sig = _sig - 1;
	return 1 & (set->sig[sig / _NSIG_BPW] >> (sig % _NSIG_BPW));
}
static __inline__ int __gen_sigismember(sigset_t *set, int _sig)
{
	int ret;
	__asm__("btl %2,%1\n\tsbbl %0,%0"
		: "=r"(ret) : "m"(*set), "Ir"(_sig-1) : "cc");
	return ret;
}

#define SIGISMEMBER(set,sig)               \
	(__builtin_constant_p(sig) ?           \
	__const_sigismember((set),(sig)) :     \
	__gen_sigismember((set),(sig)))

#define SIGADDSET1(set,sig)                \
	(__builtin_constant_p(sig) ?           \
	__const_sigaddset((set),(sig)) :       \
	__gen_sigaddset((set),(sig)))

static __inline__ void __gen_sigaddset(sigset_t *set, int _sig)
{
	__asm__("btsl %1,%0" : "=m"(*set) : "Ir"(_sig-1) : "cc");
}

static __inline__ void __const_sigaddset(sigset_t *set, int _sig)
{
	unsigned long sig = _sig - 1;
	set->sig[sig / _NSIG_BPW] |= 1 << (sig % _NSIG_BPW);
}

#define SIGADDSET  __gen_sigaddset
//#define SIGADDSET  SIGADDSET1 /* fixes the problem */


#endif


sigset_t ssi_fatal_sigset;
static sigset_t ssi_stop_set;
static void print_pending_signals(sigset_t *pending, sigset_t *blocked);

void sigset_test_exit(void)
{
	return;
}

#ifdef  DEF_KERNEL
static int sigset_test_init()
#else
int main()
#endif
{
	sigset_t cleaned;

	cleaned = (sigset_t) { {0,  0} };
	/* these are non-fatal signals.  Other signals are if their action is SIG_DFL */
	SIGADDSET(&cleaned, SIGCONT);
	SIGADDSET(&cleaned, SIGCHLD);
	SIGADDSET(&cleaned, SIGWINCH);
	SIGADDSET(&cleaned, SIGURG);
	SIGADDSET(&cleaned, SIGTSTP);
	SIGADDSET(&cleaned, SIGTTIN);
	SIGADDSET(&cleaned, SIGTTOU);
	SIGADDSET(&cleaned, SIGTTOU);
	ssi_fatal_sigset =  (sigset_t) { {-1,  -1} };

	//uncomenting this line fixes the problem
	//printk(KERN_INFO "sigset_test: fatal %p, stop %p\n", &ssi_fatal_sigset, &ssi_stop_set);

	/* ssi_fatal_sigset is a mask of all signals that are fatal */
	//commenting the line below fixes the problem
	ssi_fatal_sigset = cleaned;

	ssi_stop_set = (sigset_t) { {0,  0} };
	printk(KERN_INFO "sigset_test after zeroing\n");
	print_pending_signals(&ssi_stop_set, NULL);

	SIGADDSET(&ssi_stop_set, SIGTSTP);
	printk(KERN_INFO "sigset_test after %d\n", SIGTSTP);
	print_pending_signals(&ssi_stop_set, NULL);

	SIGADDSET(&ssi_stop_set, SIGTTIN);
	printk(KERN_INFO "sigset_test after %d\n", SIGTTIN);
	print_pending_signals(&ssi_stop_set, NULL);

	SIGADDSET(&ssi_stop_set, SIGTTOU);
	printk(KERN_INFO "sigset_test after %d\n", SIGTTOU);
	print_pending_signals(&ssi_stop_set, NULL);

	SIGADDSET(&ssi_stop_set, SIGSTOP);
	printk(KERN_INFO "sigset_test after %d\n", SIGSTOP);
	print_pending_signals(&ssi_stop_set, NULL);
	return 1;
}

static void print_pending_signals(sigset_t *pending, sigset_t *blocked)
{
	int i;

	if(pending->sig[0] == 0 && pending->sig[1] == 0) {
		printk(KERN_INFO "sigset_t: no set signals\n");
		return;
	}
	if(blocked) {
		for(i = 0; i < _NSIG; i++) {
			if(SIGISMEMBER(pending, i+1)) {
				printk(KERN_INFO "sigset_test: set signal %d is%sblocked\n", i+1,  SIGISMEMBER(blocked, i+1) ? " " : " not ");
			}
		}
	} else {
		 for(i = 0; i < _NSIG; i++) {
			if(SIGISMEMBER(pending, i+1)) {
				printk(KERN_INFO "sigset_test: set signal %d\n", i+1);
			}
		}
	}
}

[-- Attachment #3: sigset_ops.patch --]
[-- Type: text/plain, Size: 1275 bytes --]

--- signal.h.orig	Thu Nov 17 08:47:14 2005
+++ signal.h	Thu Nov 17 08:54:12 2005
@@ -186,16 +186,39 @@
 
 #define __HAVE_ARCH_SIG_BITOPS
 
-static __inline__ void sigaddset(sigset_t *set, int _sig)
+#define sigaddset(set,sig)                 \
+	(__builtin_constant_p(sig) ?       \
+	__const_sigaddset((set),(sig)) :   \
+	__gen_sigaddset((set),(sig)))
+
+static __inline__ void __gen_sigaddset(sigset_t *set, int _sig)
 {
 	__asm__("btsl %1,%0" : "=m"(*set) : "Ir"(_sig - 1) : "cc");
 }
 
-static __inline__ void sigdelset(sigset_t *set, int _sig)
+static __inline__ void __const_sigaddset(sigset_t *set, int _sig)
+{
+	unsigned long sig = _sig - 1;
+	set->sig[sig / _NSIG_BPW] |= 1 << (sig % _NSIG_BPW);
+}
+
+#define sigdelset(set,sig)                 \
+	(__builtin_constant_p(sig) ?       \
+	__const_sigdelset((set),(sig)) :   \
+	__gen_sigdelset((set),(sig)))
+
+
+static __inline__ void __gen_sigdelset(sigset_t *set, int _sig)
 {
 	__asm__("btrl %1,%0" : "=m"(*set) : "Ir"(_sig - 1) : "cc");
 }
 
+static __inline__ void __const_sigaddset(sigset_t *set, int _sig)
+{
+	unsigned long sig = _sig - 1;
+	set->sig[sig / _NSIG_BPW] &= ~(1 << (sig % _NSIG_BPW));
+}
+
 static __inline__ int __const_sigismember(sigset_t *set, int _sig)
 {
 	unsigned long sig = _sig - 1;

Re: gcc optimizer miscompiles code with sigaddset on i386 if sig arg is const -- PATCH proposed

[Constantine Gavrilov]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

Constantine Gavrilov wrote:

> I have run into problem using sigaddset() with constant signal 
> argument in kernel code.
> .................


According to jakub@xxxxxxxx <mailto:jakub@redhat.com>, gcc maintainer at RedHat, it is a pure kernel bug
and not a gcc problem.  I have reworked the patch but kept the constant case optimization.

A quote form Jakub to make the issue clear:

That's just buggy testcase.
You need either
__asm__("btsl %1,%0" : "+m"(*set) : "Ir"(_sig-1) : "cc");
or
__asm__("btsl %1,%0" : "=m"(*set) : "Ir"(_sig-1), "m"(*set) : "cc");
because the btsl instruction doesn't just set the memory to some value, but
needs to read its previous content as well.  If you don't tell that fact to GCC,
GCC is of course free to optimize as if the asm was just setting the value
and not depended on the previous value.

Attached please find a new patch.

-- 
----------------------------------------
Constantine Gavrilov
Kernel Developer
Qlusters Software Ltd
1 Azrieli Center, Tel-Aviv
Phone: +972-3-6081977
Fax:   +972-3-6081841
----------------------------------------


[-- Attachment #2: sigset_ops.patch --]
[-- Type: text/plain, Size: 1364 bytes --]

--- signal.h.orig	Thu Nov 17 08:47:14 2005
+++ signal.h	Thu Nov 17 11:19:55 2005
@@ -186,14 +186,37 @@
 
 #define __HAVE_ARCH_SIG_BITOPS
 
-static __inline__ void sigaddset(sigset_t *set, int _sig)
+#define sigaddset(set,sig)                 \
+	(__builtin_constant_p(sig) ?       \
+	__const_sigaddset((set),(sig)) :   \
+	__gen_sigaddset((set),(sig)))
+
+static __inline__ void __gen_sigaddset(sigset_t *set, int _sig)
+{
+	__asm__("btsl %1,%0" : "+m"(*set) : "Ir"(_sig - 1) : "cc");
+}
+
+static __inline__ void __const_sigaddset(sigset_t *set, int _sig)
+{
+	unsigned long sig = _sig - 1;
+	set->sig[sig / _NSIG_BPW] |= 1 << (sig % _NSIG_BPW);
+}
+
+#define sigdelset(set,sig)                 \
+	(__builtin_constant_p(sig) ?       \
+	__const_sigdelset((set),(sig)) :   \
+	__gen_sigdelset((set),(sig)))
+
+
+static __inline__ void __gen_sigdelset(sigset_t *set, int _sig)
 {
-	__asm__("btsl %1,%0" : "=m"(*set) : "Ir"(_sig - 1) : "cc");
+	__asm__("btrl %1,%0" : "+m"(*set) : "Ir"(_sig - 1) : "cc");
 }
 
-static __inline__ void sigdelset(sigset_t *set, int _sig)
+static __inline__ void __const_sigaddset(sigset_t *set, int _sig)
 {
-	__asm__("btrl %1,%0" : "=m"(*set) : "Ir"(_sig - 1) : "cc");
+	unsigned long sig = _sig - 1;
+	set->sig[sig / _NSIG_BPW] &= ~(1 << (sig % _NSIG_BPW));
 }
 
 static __inline__ int __const_sigismember(sigset_t *set, int _sig)