ldecod has undefined behavior, fails in x86 jit on linux

[Quuxplusone]

Bugzilla Link	PR963
Status	RESOLVED FIXED
Importance	P normal
Reported by	Reid Spencer (rspencer@reidspencer.com)
Reported on	2006-10-22 13:50:39 -0700
Last modified on	2010-02-22 12:41:28 -0800
Version	trunk
Hardware	PC Linux
CC	anton@korobeynikov.info, clattner@nondot.org, evan.cheng@apple.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments	bugpoint.safe.bc (443299 bytes, application/octet-stream) bugpoint.test.bc (646 bytes, application/octet-stream) ldecod.jit.gdb.out (5981 bytes, text/plain) bugpoint.test.ll (2059 bytes, text/plain) bugpoint.test.bc (646 bytes, application/octet-stream) ldecod.out-nat (3205 bytes, text/plain) ldecod.out-jit (3205 bytes, text/plain)
Blocks
Blocked by
See also

The ldecod test shows a significant difference in the output of its floating
point numbers (~0.2 to ~1.3) compared to GCC. This is most notable on the JIT.
Furthermore the JIT output does not agree with the CBE, and LLC output.

Running "make bugpoint-jit" produces:

*** The following function is being miscompiled:  dpb_split_field_cond_true967
<cbe><gcc>You can reproduce the problem with the command line:
  lli -load ./bugpoint.safe.bc.cbe.c.so bugpoint.test.bc -i
/proj/llvm/llvm-test-nc/MultiSource/Applications/JM/ldecod/data/test.264 -o
/proj/llvm/llvm-test-nc/MultiSource/Applications/JM/ldecod/data/test_dec.yuv -r
/proj/llvm/llvm-test-nc/MultiSource/Applications/JM/ldecod/data/test_rec.yuv
The shared object was created with:
  llc -march=c bugpoint.safe.bc -o temporary.c
  gcc -xc temporary.c -O2 -o ./bugpoint.safe.bc.cbe.c.so -shared \
    -fno-strict-aliasing

Looking at the bugpoint.safe.bc file, the interesting bit is this block:
cond_true967:           ; preds = %newFuncRoot
        %tmp974 = getelementptr %struct.StorablePicture* %tmp957.reload, int 0,
uint 30         ; <short***> [#uses=1]
        %tmp975 = load short*** %tmp974         ; <short**> [#uses=1]
        %tmp977 = getelementptr short** %tmp975, int %tmp675.reload
; <short**> [#uses=1]
        %tmp978 = load short** %tmp977          ; <short*> [#uses=1]
        %tmp980 = getelementptr short* %tmp978, int %tmp678.reload
; <short*> [#uses=1]
        %tmp981 = load short* %tmp980           ; <short> [#uses=1]
        %tmp981 = cast short %tmp981 to int             ; <int> [#uses=1]
        %tmp986 = getelementptr %struct.StorablePicture* %tmp957.reload, int 0,
uint 5, int %tmp981, int 1, int %tmp916.reload          ; <long*> [#uses=1]
        %tmp987 = load long* %tmp986            ; <long> [#uses=1]
        br label %cond_next989.exitStub

which apparently has nothing to do with floating point calcuations.

[Quuxplusone]

Attached bugpoint.safe.bc (443299 bytes, application/octet-stream): Safe portion of ldecod that bugpoint found.

[Quuxplusone]

Oops. There's a typo in the initial comment. It should say:

Looking at the bugpoint.test.bc file ..

instead of:

Looking at the bugpoint.safe.bc file ..

[Quuxplusone]

Hi Reid, thanks for filing this.  In order to debug this, please do the
following steps:

1. run with lli in GDB with -debug-only=jit:
   gdb --args lli -debug-only=jit -load ./bugpoint.safe.bc.cbe.c.so bugpoint.test.bc -i
/proj/llvm/llvm-test-nc/MultiSource/Applications/JM/ldecod/data/test.264 -o
/proj/llvm/llvm-test-nc/MultiSource/Applications/JM/ldecod/data/test_dec.yuv -r
/proj/llvm/llvm-test-nc/MultiSource/Applications/JM/ldecod/data/test_rec.yuv

2. Set a breakpoint at JITEmitter.cpp:856, run until it.
3. You should hit it twice, and it should print out information about the
start/size of each function jit'd.
For each one, please tell gdb "disassemble start start+size"  Since size (IIRC)
is printed in hex, give it a
0x prefix  to gdb.

Please attach the output of #3 along with the output of llc on the test .bc
file to this bug.

Thanks,

-Chris

[Quuxplusone]

Attached ldecod.jit.gdb.out (5981 bytes, text/plain): Output from gdb session on ldecod in JIT as Chris Suggested

[Quuxplusone]

JIT Code:
Dump of assembler code from 0xb64038 to 0xb6409b:
0x00b64038:     sub    $0xc,%esp
0x00b6403b:     mov    %esi,0x8(%esp)
0x00b6403f:     mov    %edi,0x4(%esp)
0x00b64043:     mov    %ebx,(%esp)
0x00b64046:     mov    0x1c(%esp),%eax
0x00b6404a:     mov    0x14(%esp),%ecx
0x00b6404e:     mov    0x20(%esp),%edx
0x00b64052:     mov    0x18(%esp),%esi
0x00b64056:     mov    0x10(%esp),%edi
0x00b6405a:     jmp    0xb64073
0x00b6405f:     mov    %ecx,0x4(%edx)
0x00b64062:     mov    %eax,(%edx)
0x00b64064:     mov    (%esp),%ebx
0x00b64067:     mov    0x4(%esp),%edi
0x00b6406b:     mov    0x8(%esp),%esi
0x00b6406f:     add    $0xc,%esp
0x00b64072:     ret
0x00b64073:     mov    0x4d5e8(%eax),%ebx
0x00b64079:     mov    (%ebx,%ecx,4),%ecx
0x00b6407c:     movswl (%ecx,%edi,2),%ecx
0x00b64080:     imul   $0x630,%ecx,%ecx
0x00b64086:     add    %ecx,%eax
0x00b64088:     mov    0x120(%eax,%esi,8),%ecx
0x00b6408f:     mov    0x11c(%eax,%esi,8),%eax
0x00b64096:     jmp    0xb6405f
End of assembler dump.

[Quuxplusone]

LLC Generated Code:
.text
        .align  16
        .globl  dpb_split_field_cond_true967
dpb_split_field_cond_true967:
        subl $12, %esp
        movl %esi, 8(%esp)
        movl %edi, 4(%esp)
        movl %ebx, (%esp)
        movl 28(%esp), %eax
        movl 20(%esp), %ecx
        movl 32(%esp), %edx
        movl 24(%esp), %esi
        movl 16(%esp), %edi
.BB1_2: #cond_true967
        movl 316904(%eax), %ebx
        movl (%ebx,%ecx,4), %ecx
        movswl (%ecx,%edi,2), %ecx
        imull $1584, %ecx, %ecx
        addl %ecx, %eax
        movl 288(%eax,%esi,8), %ecx
        movl 284(%eax,%esi,8), %eax
.BB1_1: #cond_next989.exitStub
        movl %ecx, 4(%edx)
        movl %eax, (%edx)
        movl (%esp), %ebx
        movl 4(%esp), %edi
        movl 8(%esp), %esi
        addl $12, %esp
        ret
        .size dpb_split_field_cond_true967, .-dpb_split_field_cond_true967

        .align  16
        .globl  main
main:
        subl $12, %esp
        fnstcw 10(%esp)
        movb $2, 11(%esp)
        fldcw 10(%esp)
        movl 20(%esp), %eax
        movl %eax, 4(%esp)
        movl 16(%esp), %eax
        movl %eax, (%esp)
        call llvm_bugpoint_old_main
        addl $12, %esp
        ret
        .size main, .-main

[Quuxplusone]

Attached bugpoint.test.ll (2059 bytes, text/plain): LLVM Assembly For Test Function

[Quuxplusone]

Attached bugpoint.test.bc (646 bytes, application/octet-stream): Buggy portion of ldecod that bugpoint found

[Quuxplusone]

Attached bugpoint.test.bc (646 bytes, application/octet-stream): Bytecode for test function

[Quuxplusone]

The difference between the gdb disassembly and llc output makes no sense. There
is whole chunk of
code missing from the llc version:

0x00b6405a:     jmp    0xb64073
0x00b6405f:     mov    %ecx,0x4(%edx)
0x00b64062:     mov    %eax,(%edx)
0x00b64064:     mov    (%esp),%ebx
0x00b64067:     mov    0x4(%esp),%edi
0x00b6406b:     mov    0x8(%esp),%esi
0x00b6406f:     add    $0xc,%esp
0x00b64072:     ret

That shouldn't happen.

Not that it should make that kind of difference. But Reid, are you generating
the llc output with -
relocation-mode=static?

[Quuxplusone]

Here's the llc -relocation-model=static code:
.text
        .align  16
        .globl  dpb_split_field_cond_true967
dpb_split_field_cond_true967:
        subl $12, %esp
        movl %esi, 8(%esp)
        movl %edi, 4(%esp)
        movl %ebx, (%esp)
        movl 28(%esp), %eax
        movl 20(%esp), %ecx
        movl 32(%esp), %edx
        movl 24(%esp), %esi
        movl 16(%esp), %edi
.BB1_2: #cond_true967
        movl 316904(%eax), %ebx
        movl (%ebx,%ecx,4), %ecx
        movswl (%ecx,%edi,2), %ecx
        imull $1584, %ecx, %ecx
        addl %ecx, %eax
        movl 288(%eax,%esi,8), %ecx
        movl 284(%eax,%esi,8), %eax
.BB1_1: #cond_next989.exitStub
        movl %ecx, 4(%edx)
        movl %eax, (%edx)
        movl (%esp), %ebx
        movl 4(%esp), %edi
        movl 8(%esp), %esi
        addl $12, %esp
        ret
        .size dpb_split_field_cond_true967, .-dpb_split_field_cond_true967

        .align  16
        .globl  main
main:
        subl $12, %esp
        fnstcw 10(%esp)
        movb $2, 11(%esp)
        fldcw 10(%esp)
        movl 20(%esp), %eax
        movl %eax, 4(%esp)
        movl 16(%esp), %eax
        movl %eax, (%esp)
        call llvm_bugpoint_old_main
        addl $12, %esp
        ret
        .size main, .-main

The only difference I see is the additional block and jump to it in the lli
version of the code.

[Quuxplusone]

I went through this carefully. The only differences are:

1. The LLC version puts an "l" at the end of the mnemonics. I'm assuming this is
   just a difference between gdb and llc. However, if they truly are different
   instruction codes, perhaps that's an issue.
2. Ignoring #1, the JIT code is identical to the LLC code except for the extra
   unconditional branch. While the LLC code executes straight through with this:

.BB1_2: #cond_true967
        movl 316904(%eax), %ebx
        movl (%ebx,%ecx,4), %ecx
        movswl (%ecx,%edi,2), %ecx
        imull $1584, %ecx, %ecx
        addl %ecx, %eax
        movl 288(%eax,%esi,8), %ecx
        movl 284(%eax,%esi,8), %eax
.BB1_1: #cond_next989.exitStub

  the JIT code branches to do the same thing:
0x00b6405a:     jmp    0xb64073                 # Branch down to 4073
0x00b6405f:     mov    %ecx,0x4(%edx)
... snip ...
0x00b64073:     mov    0x4d5e8(%eax),%ebx
0x00b64079:     mov    (%ebx,%ecx,4),%ecx
0x00b6407c:     movswl (%ecx,%edi,2),%ecx
0x00b64080:     imul   $0x630,%ecx,%ecx
0x00b64086:     add    %ecx,%eax
0x00b64088:     mov    0x120(%eax,%esi,8),%ecx
0x00b6408f:     mov    0x11c(%eax,%esi,8),%eax
0x00b64096:     jmp    0xb6405f                  #Branch back to 405f

Those are the only differences. Furthermore, I don't see how its affecting
floating point output. Did bugpoint find the wrong bug?

[Quuxplusone]

Evan, please look into this and try to determine if it's bogus or not.  If not,
I'd like to have this fixed for
the release (we branch on monday) but if it can't be fixed/figured out, we
should just close it.

Thanks,

-Chris

[Quuxplusone]

I can't really figure out why there is an extra block of code under JIT case.
So this is a "can't fix / figured
out" for 1.9. Should I close it?

[Quuxplusone]

No, you shouldn't. This is the reason why all the ldecod tests have been failing
on x86/Linux. It needs to be figured out. We can hold it over as a known
(unknown?) issue for the release, however.

[Quuxplusone]

An afterthought:

You may be missing the point. The "extra block" doesn't matter. The LLC and JIT
code are identical except JIT branches to that block and branches back. Weird,
but no big deal. The *real* problem is that the values produced by this code do
not conform to the values produced by llc, cbe or gcc. Here's last nights
nightly test result:

******************** TEST (jit) 'ldecod' FAILED! ********************
Execution Context Diff:
/proj/llvm/nightly/build/llvm/Debug/bin/fpcmp: Compared: 3.535600e+00 and
4.472200e+00: diff = 2.094271e-01
Out of tolerance: rel/abs: 2.000000e-02/0.000000e+00
******************** TEST (jit) 'ldecod' ****************************

I will attach the jit and nat output.

[Quuxplusone]

Attached ldecod.out-nat (3205 bytes, text/plain): Native Output

[Quuxplusone]

Attached ldecod.out-jit (3205 bytes, text/plain): JIT Output

[Quuxplusone]

Another thought:

The difference in output may not be the result of any JIT'd code at all, it
might be in the ExecutionEngine or some other factor.

[Quuxplusone]

is this still an issue?

[Quuxplusone]

Yes. I tried to fix it but I can't detect where it is producing differeing
binary output.

[Quuxplusone]

Anton, if you get a chance, can you see if this reproduces for you?

[Quuxplusone]

Ok. Will try within next 3-4 days.

[Quuxplusone]

JIT fails for me too.

[Quuxplusone]

Reid/Anton, can you see if this still fails with PR1130 fixed?

Thanks,

-Chris

[Quuxplusone]

It is still failing for me

[Quuxplusone]

If we're enable trace feature of ldecod (define.h => TRACE define) the output
for all binaries (native, llc, cbe, lli) is the same.

[Quuxplusone]

have you tried running the native version of ldecod under valgrind?

[Quuxplusone]

Fixed with

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070129/043880.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070129/043881.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070129/043883.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070129/043882.html

Reopen bug in case of any problems.

[Quuxplusone]

To be clear, the x86-jit failed because the program was reading uninitialized
memory, so it was a bug in
the program, not llvm.

[Quuxplusone]

Well, to be clearer :)  Program reads uninitialized memory, because provided
input data was wrong: sequence to decode contained 15 frames, but reference
sequence (to compare with) - just 2. So, there were 2 bugs: absence of size
checking in the program and bad input in the testcase :)