[lkml]   [2009]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patch in this message
    SubjectRe: [Bug 13012] causes init to segfault on Debian etch; OK

    On Sun, 12 Jul 2009, Linus Torvalds wrote:
    > From everything I have been able to find, I really prefer the second
    > version. Not only is the patch cleaner, but it looks like code generation
    > is better too (for some inexplicable reason, but I suspect it's because
    > -fno-strict-overflow is just saner).

    Hmm. I just checked. The file that caused us to do this thing in the first
    place (fs/open.c, around like 415, which does:

    /* Check for wrap through zero too */
    if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
    goto out_fput;

    to check that the resulting 'loffset_t' type is all good) has interesting
    behaviour with my version of gcc (gcc version 4.4.0 20090506 (Red Hat
    4.4.0-4) (GCC)).

    - Without any options:
    leaq (%rcx,%rdx), %rdi #, tmp73
    movq 256(%rbx), %rsi # <variable>.i_sb, <variable>.i_sb
    movl $-27, %eax #, D.29131
    cmpq 40(%rsi), %rdi # <variable>.s_maxbytes, tmp73
    ja .L148 #,

    - With -fno-strict-overflow:
    leaq (%rcx,%rdx), %rax #, D.29157
    movq 256(%rbx), %rsi # <variable>.i_sb, <variable>.i_sb
    cmpq 40(%rsi), %rax # <variable>.s_maxbytes, D.29157
    ja .L154 #,
    testq %rax, %rax # D.29157
    js .L154 #,

    - With -fwrapv:
    leaq (%rcx,%rdx), %rax #, D.29158
    movq 256(%rbx), %rsi # <variable>.i_sb, <variable>.i_sb
    cmpq 40(%rsi), %rax # <variable>.s_maxbytes, D.29158
    ja .L154 #,
    testq %rax, %rax # D.29158
    js .L154 #,

    and from this it would look like:

    - gcc on its own is actually the best version (the first comparison is
    unsigned because s_maxbytes is actually 'unsigned long long', so it
    actually does the right thing!)

    In other words, the whole '< 0' was unnecessary, but does make the
    source code way more readable, and makes the source code _correct_
    regardless of any type issues!

    - From a cursory inspection, -fno-strict-overflow and -fwrapv are both
    equivalent in this code, and both do the stupid thing (but for good
    reason - gcc doesn't know that 's_maxbytes' might not be 'negative in a
    loffset_t', so technically speaking the extraneous 'js' is not
    extraneous, because it can actually trigger some "more negative"
    entries than s_maxbyes is.

    - HOWEVER:

    [torvalds@nehalem ~]$ git diff --stat open.s open.s-fno-strict-overflow
    open.s => open.s-fno-strict-overflow | 22 +++++++++++++---------
    1 files changed, 13 insertions(+), 9 deletions(-)
    [torvalds@nehalem ~]$ git diff --stat open.s open.s-fwrapv
    open.s => open.s-fwrapv | 296 ++++++++++++++++++++++++-----------------------
    1 files changed, 150 insertions(+), 146 deletions(-)

    where the _only_ difference that '-fno-strict-overflow' introduces is
    that one small area (it's saying 22 lines changed, but that's because
    there's also the compiler option listing at the top of the file etc)

    In contrast, -fwrapv has done a lot of other changes too. Now, in both
    cases, it really only added the same four instructions (testq + js +
    branchtarget + jumparound).

    It looks like 'fwrapv' generates more temporaries (possibly for the code
    that treies to enforce the exact twos-complement behavior) that then all
    get optimized back out again. The differences seem to be in the temporary
    variable numbers etc, not in the actual code.

    So fwrapv really _is_ different from fno-strict-pverflow, and disturbs the
    code generation more.

    IOW, I'm convinced we should never use fwrapv. It's clearly a buggy piece
    of sh*t, as shown by our 4.1.x experiences. We should use

    Will commit the following (which also fits naming-wise with our use of


    Makefile | 2 +-
    1 files changed, 1 insertions(+), 1 deletions(-)

    diff --git a/Makefile b/Makefile
    index 0aeec59..bbe8453 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -565,7 +565,7 @@ KBUILD_CFLAGS += $(call cc-option,-Wdeclaration-after-statement,)
    KBUILD_CFLAGS += $(call cc-option,-Wno-pointer-sign,)

    # disable invalid "can't wrap" optimizations for signed / pointers
    -KBUILD_CFLAGS += $(call cc-option,-fwrapv)
    +KBUILD_CFLAGS += $(call cc-option,-fno-strict-overflow)

    # revert to pre-gcc-4.4 behaviour of .eh_frame
    KBUILD_CFLAGS += $(call cc-option,-fno-dwarf2-cfi-asm)

     \ /
      Last update: 2009-10-18 23:28    [W:0.025 / U:29.784 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site