Bug 5620

Summary:	sigaddset/sigdelset may not work as expected with gcc optimizer
Product:	Platform Specific/Hardware	Reporter:	Constantine Gavrilov (constg)
Component:	i386	Assignee:	platform_i386
Status:	CLOSED CODE_FIX
Severity:	high	CC:	akpm
Priority:	P2
Hardware:	i386
OS:	Linux
Kernel Version:	All 2.4 and all 2.6 versions	Subsystem:
Regression:	---	Bisected commit-id:
Attachments:	a patch to prevent miscompilation of sigaddset with const signal argument A sample of code using sigaddset that shows the problem fix to sigaddset sigdelset for i386 fix to sigaddset sigdelset for i386

Description Constantine Gavrilov 2005-11-17 00:41:25 UTC

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 */

At some iteration of sigaddset to c, c may get the value of a! Disabling
optimization, or using -O flag, or using a variable instead of constants fix the
problem.

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, when compiled with -O2 flag. 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).

Comment 1 Constantine Gavrilov 2005-11-17 00:42:48 UTC

Created attachment 6600 [details]
a patch to prevent miscompilation of sigaddset with const signal argument

Comment 2 Constantine Gavrilov 2005-11-17 00:44:20 UTC

Created attachment 6601 [details]
A sample of code using sigaddset that shows the problem

Comment 3 Constantine Gavrilov 2005-11-17 01:20:52 UTC

According to jakub@redhat.com, gcc maintainer at RedHat, it is a pure kernel bug
and not gcc problem. Instead of
__asm__("btsl %1,%0" : "=m"(*set) : "Ir"(_sig-1) : "cc");

sigsaddset must use
__asm__("btsl %1,%0" : "+m"(*set) : "Ir"(_sig-1) : "cc");

I have reworked the pacth but kept the constant case optimization.

Comment 4 Constantine Gavrilov 2005-11-17 01:22:35 UTC

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.

Comment 5 Constantine Gavrilov 2005-11-17 01:24:17 UTC

Created attachment 6602 [details]
fix to sigaddset sigdelset for i386

Comment 6 Constantine Gavrilov 2005-11-17 02:03:01 UTC

Created attachment 6603 [details]
fix to sigaddset sigdelset for i386

Previous patch had a typo.

Comment 7 Andrew Morton 2006-01-19 02:05:34 UTC

Constantine, can you please redo this patch against 2.6.15-rc1 and
mail it to me with a full description, as per 
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt, thanks.

Comment 8 Andrew Morton 2006-01-19 02:06:37 UTC

err, make that 2.6.16-rc1.

Comment 9 Constantine Gavrilov 2006-02-22 06:27:27 UTC

Linus picked up the patch (apparently from LKML) and included it into 2.6.15
(released on January, 2. I thought my patch was ignored and eventually stopped
looking in kernel Bugzilla as well since it was not acknowledged in any way.

I am closing the bug now as it is fixed as of kernel 2.6.15.