New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use `objc_msgSend_fpret` for `backingStoreFactor`. #74

Merged
merged 1 commit into from Feb 21, 2015

Conversation

Projects
None yet
3 participants
@pcwalton
Contributor

pcwalton commented Feb 21, 2015

This mistake led to a bizarre chain reaction in Servo that ultimately
led to a hang only when optimization was enabled.

r? @bjz

Use `objc_msgSend_fpret` for `backingStoreFactor`.
This mistake led to a bizarre chain reaction in Servo that ultimately
led to a hang only when optimization was enabled.
@SSheldon

This comment has been minimized.

Show comment
Hide comment
@SSheldon

SSheldon Feb 21, 2015

Contributor

@pcwalton, did this hang happen on 32-bit OSX? I was under the impression that, for x86_64, msg_send is supposed to be used for double, and msg_send_fpret is only necessary for long double (which ObjC doesn't support). http://www.sealiesoftware.com/blog/archive/2008/11/16/objc_explain_objc_msgSend_fpret.html

Contributor

SSheldon commented Feb 21, 2015

@pcwalton, did this hang happen on 32-bit OSX? I was under the impression that, for x86_64, msg_send is supposed to be used for double, and msg_send_fpret is only necessary for long double (which ObjC doesn't support). http://www.sealiesoftware.com/blog/archive/2008/11/16/objc_explain_objc_msgSend_fpret.html

@brendanzab

This comment has been minimized.

Show comment
Hide comment

r+

brendanzab added a commit that referenced this pull request Feb 21, 2015

Merge pull request #74 from pcwalton/fpret
Use `objc_msgSend_fpret` for `backingStoreFactor`.

@brendanzab brendanzab merged commit bb12c9c into servo:master Feb 21, 2015

@pcwalton

This comment has been minimized.

Show comment
Hide comment
@pcwalton

pcwalton Feb 21, 2015

Contributor

@SSheldon It was on 64-bit OS X.

Contributor

pcwalton commented Feb 21, 2015

@SSheldon It was on 64-bit OS X.

@SSheldon

This comment has been minimized.

Show comment
Hide comment
@SSheldon

SSheldon Feb 22, 2015

Contributor

@pcwalton, interesting! I'd like to test this in my own ObjC crate, any info about the case where you were seeing weird behavior? msg_send was just always returning the wrong value? Or maybe was this when messaging nil?

Contributor

SSheldon commented Feb 22, 2015

@pcwalton, interesting! I'd like to test this in my own ObjC crate, any info about the case where you were seeing weird behavior? msg_send was just always returning the wrong value? Or maybe was this when messaging nil?

@pcwalton

This comment has been minimized.

Show comment
Hide comment
@pcwalton

pcwalton Feb 22, 2015

Contributor

@SSheldon What I saw was mov xmm0,rax after a call to call objc_msgSend that returned the double value in xmm0. This clobbered the xmm0 return value with the garbage value in rax and caused incorrect results. Switching to objc_msgSend_fpret caused xmm0 to be preserved properly.

Contributor

pcwalton commented Feb 22, 2015

@SSheldon What I saw was mov xmm0,rax after a call to call objc_msgSend that returned the double value in xmm0. This clobbered the xmm0 return value with the garbage value in rax and caused incorrect results. Switching to objc_msgSend_fpret caused xmm0 to be preserved properly.

@SSheldon

This comment has been minimized.

Show comment
Hide comment
@SSheldon

SSheldon Feb 23, 2015

Contributor

@pcwalton and @bjz, I don't think objc_msgSend_fpret is the culprit here. I was able to come up with a simple repro case:

extern crate cocoa;

use std::mem;
use cocoa::base::{class, id, msg_send, objc_msgSend, SEL, selector};

fn main() {
    unsafe {
        let number = msg_send()(class("NSNumber"), selector("numberWithDouble:"), 3.14f64);

        let f: extern fn(id, SEL, ...) -> f64 = mem::transmute(objc_msgSend);
        let value = f(number, selector("doubleValue"));

        assert!(value == 3.14);
    }
}

This fails when compiled with optimizations. However, if you change the type of f from extern fn(id, SEL, ...) -> f64 to extern fn(id, SEL) -> f64? It stops failing.

I think the true problem here is that this crate treats objc_msgSend as a function that accepts variadic arguments, when that isn't true; objc_msgSend needs to be cast to the true type of the method.

Contributor

SSheldon commented Feb 23, 2015

@pcwalton and @bjz, I don't think objc_msgSend_fpret is the culprit here. I was able to come up with a simple repro case:

extern crate cocoa;

use std::mem;
use cocoa::base::{class, id, msg_send, objc_msgSend, SEL, selector};

fn main() {
    unsafe {
        let number = msg_send()(class("NSNumber"), selector("numberWithDouble:"), 3.14f64);

        let f: extern fn(id, SEL, ...) -> f64 = mem::transmute(objc_msgSend);
        let value = f(number, selector("doubleValue"));

        assert!(value == 3.14);
    }
}

This fails when compiled with optimizations. However, if you change the type of f from extern fn(id, SEL, ...) -> f64 to extern fn(id, SEL) -> f64? It stops failing.

I think the true problem here is that this crate treats objc_msgSend as a function that accepts variadic arguments, when that isn't true; objc_msgSend needs to be cast to the true type of the method.

@SSheldon

This comment has been minimized.

Show comment
Hide comment
@SSheldon

SSheldon Feb 23, 2015

Contributor

Here's how the assembly changes when you switch from extern fn(id, SEL, ...) -> f64 to extern fn(id, SEL) -> f64:

        movq    %rax, -32(%rbp)
        leaq    -32(%rbp), %rdi
        callq   __ZN4base8selector20hdba17a2bd7f3a148eHeE
-       movq    %rax, %rcx
-       xorl    %eax, %eax
        movq    %rbx, %rdi
-       movq    %rcx, %rsi
+       movq    %rax, %rsi
        callq   _objc_msgSend
-       movd    %rax, %xmm0
        ucomisd LCPI0_0(%rip), %xmm0
        jne     LBB0_10
        jnp     LBB0_11

(I'm not much of an assembly programmer, so I'm not sure why this fixes the bug, just thought it might be informative.)

Contributor

SSheldon commented Feb 23, 2015

Here's how the assembly changes when you switch from extern fn(id, SEL, ...) -> f64 to extern fn(id, SEL) -> f64:

        movq    %rax, -32(%rbp)
        leaq    -32(%rbp), %rdi
        callq   __ZN4base8selector20hdba17a2bd7f3a148eHeE
-       movq    %rax, %rcx
-       xorl    %eax, %eax
        movq    %rbx, %rdi
-       movq    %rcx, %rsi
+       movq    %rax, %rsi
        callq   _objc_msgSend
-       movd    %rax, %xmm0
        ucomisd LCPI0_0(%rip), %xmm0
        jne     LBB0_10
        jnp     LBB0_11

(I'm not much of an assembly programmer, so I'm not sure why this fixes the bug, just thought it might be informative.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment