lkml.org 
[lkml]   [2009]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Linux 2.6.27.27


    On Wed, 22 Jul 2009, Krzysztof Oledzki wrote:

    >
    >
    > On Tue, 21 Jul 2009, Linus Torvalds wrote:
    >
    >>
    >> [ Added Ian Lance Taylor to the cc, he knows the background, and unlike me
    >> is competent with gcc. ]
    >>
    >> On Tue, 21 Jul 2009, Troy Moure wrote:
    >>>
    >>> I think I've found something interesting. Look at the the code generated
    >>> for edid_checksum() in driver/video/fbmon.c. This is what I see for the
    >>> -fno-strict-overflow kernel:
    >>
    >> Ooh.
    >>
    >> Bingo. You're 100% right, and you definitely found it (of course, there
    >> may be _other_ cases like this, but that's certainly _one_ of the
    >> problems, and probably the only one).
    >>
    >> Just out of curiosity, how did you find it? Now that I know where to look,
    >> it's very obvious in the assembler diffs, but I didn't notice it until you
    >> pointed it out just because there is so _much_ of the diffs...
    >>
    >> And yes, that's very much a compiler bug. And I also bet it's very easily
    >> fixed.
    >>
    >> The code in question is this loop:
    >>
    >> #define EDID_LENGTH 128
    >>
    >> unsigned char i, ...
    >>
    >> for (i = 0; i < EDID_LENGTH; i++) {
    >> csum += edid[i];
    >> all_null |= edid[i];
    >> }
    >>
    >> and gcc -fno-strict-overflow has apparently decided that that is an
    >> infinite loop, even though it clearly is not. So then the stupid and buggy
    >> compiler will compile that loop (and the whole rest of the function) to
    >> the "optimized" version that is just
    >>
    >> loop:
    >> jmp loop;
    >>
    >> I even bet I know why: it looks at "unsigned char", and sees that it is an
    >> 8-bit variable, and then it looks at "i < EDID_LENGTH" and sees that it is
    >> a _signed_ comparison (it's signed because the C type rules mean that
    >> 'unsigned char' will be extended to 'int' in an expression), and then it
    >> decides that in a signed comparison an 8-bit entry is always going to be
    >> smaller than 128.
    >>
    >> Anyway, I bet we can work around the compiler bug by just changing the
    >> type of "i" from "unsigned char" to be a plain "int".
    >>
    >> Krzysztof? Mind testing that?
    >>
    >> Ian? This is Linux 2.6.27.27 compiled with gcc-4.2.4. I'm not seeing the
    >> bug in the gcc I have on my machine (gcc-4.4.0), but the bug is very clear
    >> (once you _find_ it, which was the problem) in the binaries that Krzysztof
    >> posted. They're still at:
    >>
    >> http://noc.axelspringer.pl/no-strict-overflow-vs-wrapv/vmlinux-fno-strict-overflow.bz2
    >> (Hangs)
    >> http://noc.axelspringer.pl/no-strict-overflow-vs-wrapv/vmlinux-fwrapv.bz2
    >> (OK)
    >> http://noc.axelspringer.pl/no-strict-overflow-vs-wrapv/vmlinux-fnone.bz2
    >> (OK)
    >>
    >> and you can clearly see the 'edid_checksum' miscompilation in the objdump
    >> disassembly.
    >
    > BTW: here is a simple testcase for this bug:
    >
    > --- fno-strict-overflow-fixed-bug.c ---
    > #include <stdio.h>
    >
    > int main() {
    >
    > unsigned char i;
    >
    > for (i = 0; i < 128; i++)
    > printf("loop %u\n", i);
    >
    > return 0;
    > }
    > --- cut here ---

    Or this better one (no infinite loop):

    --- cut here ---
    #include <stdio.h>

    int main() {

    unsigned char i, j=0;

    for (i = 0; i <= 127; i++) {

    if (!i && j++) {
    printf("Buggy GCC\n");
    return 1;
    }
    }

    printf("GCC is OK\n");

    return 0;
    }
    --- cut here ---

    > The code should be compiled with:
    > cc -o fno-strict-overflow-fixed-bug -Os -fno-strict-overflow
    > fno-strict-overflow-fixed-bug.c
    > or:
    > cc -o fno-strict-overflow-fixed-bug -O2 -fno-strict-overflow
    > fno-strict-overflow-fixed-bug.c
    >
    > This bug does not exist with -O1 or if the loop is controlled by "i < 127" or
    > "i < 129".
    >
    > So, we should make sure there is no
    > unsigned char i; (...) for (i = 0; i < 128; i++)
    > somewhere inside the kernel.


    Best regards,

    Krzysztof Olędzki
    \
     
     \ /
      Last update: 2009-07-22 15:49    [W:0.050 / U:179.872 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site