[lkml]   [2009]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: Linux

[ 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:


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

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

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 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: (Hangs) (OK) (OK)

and you can clearly see the 'edid_checksum' miscompilation in the objdump

Thanks Troy,


Leaving in the context for Ian:

Buggy -fno-strict-overflow compilation:

> ...
> ffffffff803b37ed <edid_checksum>:
> ffffffff803b37ed: 53 push %rbx
> ffffffff803b37ee: 48 89 fb mov %rdi,%rbx
> ffffffff803b37f1: e8 8d fd ff ff callq ffffffff803b3583 <check_edid>
> ffffffff803b37f6: 85 c0 test %eax,%eax
> ffffffff803b37f8: 89 c6 mov %eax,%esi
> ffffffff803b37fa: 74 08 je ffffffff803b3804 <edid_checksum+0x17>
> ffffffff803b37fc: 48 89 df mov %rbx,%rdi
> ffffffff803b37ff: e8 c0 fe ff ff callq ffffffff803b36c4 <fix_edid>
> ffffffff803b3804: eb fe jmp ffffffff803b3804 <edid_checksum+0x17>
> ffffffff803b3806 <fb_parse_edid>:
> ffffffff803b3806: 41 54 push %r12
> ffffffff803b3808: 48 85 ff test %rdi,%rdi
> ...
> That last insn in edid_checksum() doesn't look *quite* right to me...
> The -fnone kernel has something a lot more sensible-looking:
> ffffffff803b39dd <edid_checksum>:
> ffffffff803b39dd: 53 push %rbx
> ffffffff803b39de: 48 89 fb mov %rdi,%rbx
> ffffffff803b39e1: e8 8d fd ff ff callq ffffffff803b3773 <check_$
> ffffffff803b39e6: 85 c0 test %eax,%eax
> ffffffff803b39e8: 89 c6 mov %eax,%esi
> ffffffff803b39ea: 74 08 je ffffffff803b39f4 <edid_c$
> ffffffff803b39ec: 48 89 df mov %rbx,%rdi
> ffffffff803b39ef: e8 c0 fe ff ff callq ffffffff803b38b4 <fix_ed$
> ffffffff803b39f4: 31 c9 xor %ecx,%ecx
> ffffffff803b39f6: 31 f6 xor %esi,%esi
> ffffffff803b39f8: 31 d2 xor %edx,%edx
> ffffffff803b39fa: eb 0a jmp ffffffff803b3a06 <edid_c$
> ffffffff803b39fc: 0f b6 c0 movzbl %al,%eax
> ffffffff803b39ff: 8a 04 03 mov (%rbx,%rax,1),%al
> ffffffff803b3a02: 01 c1 add %eax,%ecx
> ffffffff803b3a04: 09 c6 or %eax,%esi
> ffffffff803b3a06: 88 d0 mov %dl,%al
> ffffffff803b3a08: ff c2 inc %edx
> ffffffff803b3a0a: 81 fa 81 00 00 00 cmp $0x81,%edx
> ffffffff803b3a10: 75 ea jne ffffffff803b39fc <edid_c$
> ffffffff803b3a12: 84 c9 test %cl,%cl
> ffffffff803b3a14: 0f 94 c0 sete %al
> ffffffff803b3a17: 40 84 f6 test %sil,%sil
> ffffffff803b3a1a: 5b pop %rbx
> ffffffff803b3a1b: 0f 95 c2 setne %dl
> ffffffff803b3a1e: 21 d0 and %edx,%eax
> ffffffff803b3a20: 0f b6 c0 movzbl %al,%eax
> ffffffff803b3a23: c3 retq
> ffffffff803b3a24 <fb_parse_edid>:
> ffffffff803b3a24: 41 54 push %r12
> ffffffff803b3a26: 48 85 ff test %rdi,%rdi
> Hope that helps narrow things down ;)
> Troy

 \ /
  Last update: 2009-10-18 23:28    [W:0.096 / U:2.576 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site