Revert change to stop zeroing the exchange heap

[brson]

This fixes another valgrind problem, but still does not fix all the errors in rustdoc.

[brson]

Ah, right. Will fix tomorrow.

[thestinger]

@brson: Here's a simpler test case that also triggers the issue:

enum Variant { A, B(~u16) }

pub struct Blob {
    desc: Variant,
    sig: Variant
}

#[inline(never)]
fn bar() -> Variant { A }

fn main() {
    let a = bar();
    let _b = Blob {
        desc: B(~5),
        sig: copy a
    };
}

Removing the copy or allowing bar() to be inlined causes valid code to be generated, but I really don't understand what goes wrong when the copy happens...

[pcwalton]

Let me copy and paste my email to the core team:

OK, I think I've figured it out. It seems to be a slightly sketchy, but harmless in this case, LLVM optimization. Here is the testcase:

pub struct VariantDoc {
    desc: Option<~str>,
    sig: Option<~str>
}

fn main() {
    let variants = ~[
        VariantDoc {
            desc: None,
            sig: None
        }
    ];

    let _ = do vec::map(variants) |variant| {
        let new_variant = copy *variant;
        let result = VariantDoc {
            desc: None,
            sig: None,
        };
        io::println("vvv");
        result
    };
    io::println("^^^");
}

(The error occurs in between 'vvv' and '^^^').

The conditional uninitialized branch occurs as part of inlined drop glue. Here is the annotated assembly:

0x0000000100000da6 <_rust_main+54>:    lea    rdi,[rbp-0x70]
0x0000000100000daa <_rust_main+58>:    lea    r14,[rip+0x12cf]        # 0x100002080 <tydesc_1980>
0x0000000100000db1 <_rust_main+65>:    mov    rdx,r14
0x0000000100000db4 <_rust_main+68>:    mov    ecx,0x90
0x0000000100000db9 <_rust_main+73>:    call   0x100001c04 <dyld_stub__ZN2rt18rt_exchange_malloc16_63f842b316661113_06E> ; allocate
0x0000000100000dbe <_rust_main+78>:    mov    rbx,QWORD PTR [rbp-0x70] ; rbx = result of malloc
... straight line code ...
0x0000000100000ddd <_rust_main+109>:    mov    QWORD PTR [rbx+0x30],0x0 ; store None tag
... straight line code ...
0x0000000100000dfa <_rust_main+138>:    lea    rbx,[rbx+0x20] ; skip over allocation header
... straight line code ...
0x0000000100000e66 <_rust_main+246>:    mov    rax,QWORD PTR [rbx+0x10] ; rax = None tag
0x0000000100000e6a <_rust_main+250>:    mov    QWORD PTR [rbp-0xa0],rax ; copy None tag to new variant
0x0000000100000e71 <_rust_main+257>:    mov    QWORD PTR [rbp-0x98],rbx
0x0000000100000e78 <_rust_main+264>:    mov    rbx,QWORD PTR [rbx+0x18] ; rbx = eagerly load value of the ~str (which is undefined since the tag discriminant is None)
0x0000000100000e7c <_rust_main+268>:    cmp    rax,0x1 ; was tag Some?
0x0000000100000e80 <_rust_main+272>:    jne    0x100000ebe ; taken <_rust_main+334> ;
...
0x0000000100000ebe <_rust_main+334>:    mov    r12,rbx ; r12 = undefined value of ~str
... straight line code ...
0x0000000100000f23 <_rust_main+435>:    cmp    QWORD PTR [rbp-0xa0],0x1 ; is tag Some?
0x0000000100000f2b <_rust_main+443>:    setne  al ; invert previous check; al is now 1 iff tag is *None* (i.e. al is always 1)
0x0000000100000f2e <_rust_main+446>:    test   r12,r12 ; is tag None?
0x0000000100000f31 <_rust_main+449>:    je     0x100000f3f <_rust_main+463> ; ***valgrind error***; conditional branch depends on undefined value. Here, we are checking to see whether the pointer is NULL.
0x0000000100000f33 <_rust_main+451>:    test   al,al ; is tag None?
0x0000000100000f35 <_rust_main+453>:    jne    0x100000f3f <_rust_main+463> ; taken
0x0000000100000f37 <_rust_main+455>:    mov    rdx,r12
0x0000000100000f3a <_rust_main+458>:    call   0x100001bfe <dyld_stub__ZN2rt16rt_exchange_free17_bea361ae477736593_06E> ; free the allocation
0x0000000100000f3f <_rust_main+463>:    cmp    QWORD PTR [rbp-0x90],0x1

So, as you can see, it is undefined whether the first branch (at _rust_main+449) is taken or not taken, but even if it isn't taken the second branch (at _rust_main+453) will always be taken, and it goes to the same spot. Thus we always end up at _rust_main+463 safely and don't try to free a wild pointer.

In pseudocode, the compiler reordered "is the variant Some? if so, is the string pointer non-null?" into "is the string pointer non-null? if so, is the variant Some?" The answer to the question of whether the string pointer is non-null is undefined, since we didn't store anything there. But it doesn't matter because the code does nothing unless both conditions hold.

The reason why strcat's change triggered this is that we started inlining glue, which triggered scalar replacement of aggregates and LLVM started performing more clever optimizations regarding tag discriminants and the ~str. Basically, LLVM promoted them to registers. (Notice how ~str stayed in r12 across basic blocks above.) Because it then saw SSA values and not memory it started reordering things. This isn't harmful, but it tipped off Valgrind.

So yeah, this seems harmless. How to suppress it in valgrind is another story...

Patrick

[sanxiyn]

LLVM bug 12319 seems to be about the same Valgrind issue.

[brson]

@sanxiyn thanks for finding that!