lkml.org 
[lkml]   [2009]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [Bug 13012] 2.6.28.9 causes init to segfault on Debian etch; 2.6.28.8 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
-fno-strict-overflow.

Will commit the following (which also fits naming-wise with our use of
'-fno-strict-aliasing').

Linus

---
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.201 / U:0.508 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site