Bug 5620 - sigaddset/sigdelset may not work as expected with gcc optimizer
Summary: sigaddset/sigdelset may not work as expected with gcc optimizer
Status: CLOSED CODE_FIX
Alias: None
Product: Platform Specific/Hardware
Classification: Unclassified
Component: i386 (show other bugs)
Hardware: i386 Linux
: P2 high
Assignee: platform_i386
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-11-17 00:41 UTC by Constantine Gavrilov
Modified: 2006-02-22 06:27 UTC (History)
1 user (show)

See Also:
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 (1.25 KB, patch)
2005-11-17 00:42 UTC, Constantine Gavrilov
Details | Diff
A sample of code using sigaddset that shows the problem (4.15 KB, text/plain)
2005-11-17 00:44 UTC, Constantine Gavrilov
Details
fix to sigaddset sigdelset for i386 (1.33 KB, patch)
2005-11-17 01:24 UTC, Constantine Gavrilov
Details | Diff
fix to sigaddset sigdelset for i386 (1.33 KB, patch)
2005-11-17 02:03 UTC, Constantine Gavrilov
Details | Diff

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.

Note You need to log in before you can comment on or make changes to this bug.