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

[Constantine Gavrilov <constg@qlusters.com>]

[-- 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)