Draw/Cairo broken

[ptersilie]

It seems like cairo (or at least connecting the draw function to a drawing area) is broken since the last big update. The example cairotest crashes instantly unless

Connect::connect(&drawing_area, Draw::new(draw_fn));

is commented out.

[gkoz]

FWIW it's been crashing for me before any of my changes.

[ptersilie]

I wasn't suggesting it was your patch. :) I just rememberd it working before I pulled yesterday evening, but I'm not sure what commit I was at before.

[ptersilie]

This is the segfault I get with gdb:

Program received signal SIGSEGV, Segmentation fault.
0x0000555555563f3c in rgtk::gtk::signals::draw::trampoline (widget=0x555555870210, ctx=..., signal=0x0)
    at /home/lukas/research/rgtk_fork/src/gtk/signals.rs:90
90                      let t : &'a Box<super::Signal> = ::std::mem::transmute(signal);

[ptersilie]

And here's a backtrace. Anyone an idea what I should be looking for?

#0  0x000055555556512c in rgtk::gtk::signals::draw::trampoline (widget=0x555555878210, ctx=..., signal=0x0)
    at /home/lukas/research/rgtk/src/gtk/signals.rs:90
#1  0x0000555555564ffe in gtk::signals::draw::trampoline::hac039c50530d7131oxe ()
#2  0x00007ffff77161ee in ?? () from /usr/lib/libgtk-3.so.0
#3  0x00007ffff7842ed0 in ?? () from /usr/lib/libgtk-3.so.0
#4  0x00007ffff6762484 in ?? () from /usr/lib/libgobject-2.0.so.0
#5  0x00007ffff677bb20 in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#6  0x00007ffff677c9cf in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
#7  0x00007ffff78513f6 in ?? () from /usr/lib/libgtk-3.so.0
#8  0x00007ffff7852a6f in ?? () from /usr/lib/libgtk-3.so.0
#9  0x00007ffff7852cef in ?? () from /usr/lib/libgtk-3.so.0
#10 0x00007ffff766ad25 in gtk_container_propagate_draw () from /usr/lib/libgtk-3.so.0
#11 0x00007ffff766adf2 in ?? () from /usr/lib/libgtk-3.so.0
#12 0x00007ffff785bcbd in ?? () from /usr/lib/libgtk-3.so.0
#13 0x00007ffff77161ee in ?? () from /usr/lib/libgtk-3.so.0
#14 0x00007ffff7842ed0 in ?? () from /usr/lib/libgtk-3.so.0
#15 0x00007ffff6762484 in ?? () from /usr/lib/libgobject-2.0.so.0
#16 0x00007ffff677bb20 in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#17 0x00007ffff677c9cf in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
#18 0x00007ffff78513f6 in ?? () from /usr/lib/libgtk-3.so.0
#19 0x00007ffff7852a6f in ?? () from /usr/lib/libgtk-3.so.0
#20 0x00007ffff7852cef in ?? () from /usr/lib/libgtk-3.so.0
#21 0x00007ffff785302b in gtk_widget_send_expose () from /usr/lib/libgtk-3.so.0
#22 0x00007ffff7715360 in gtk_main_do_event () from /usr/lib/libgtk-3.so.0
#23 0x00007ffff72a343f in ?? () from /usr/lib/libgdk-3.so.0
#24 0x00007ffff72a3ccc in ?? () from /usr/lib/libgdk-3.so.0
#25 0x00007ffff72a3de3 in ?? () from /usr/lib/libgdk-3.so.0
#26 0x00007ffff6762484 in ?? () from /usr/lib/libgobject-2.0.so.0
#27 0x00007ffff677c077 in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#28 0x00007ffff677cf1a in g_signal_emit_by_name () from /usr/lib/libgobject-2.0.so.0
#29 0x00007ffff729d9ea in ?? () from /usr/lib/libgdk-3.so.0
#30 0x00007ffff728fe78 in ?? () from /usr/lib/libgdk-3.so.0
#31 0x00007ffff648e3d3 in ?? () from /usr/lib/libglib-2.0.so.0
#32 0x00007ffff648d92d in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#33 0x00007ffff648dd08 in ?? () from /usr/lib/libglib-2.0.so.0
#34 0x00007ffff648e032 in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
#35 0x00007ffff7714a15 in gtk_main () from /usr/lib/libgtk-3.so.0
#36 0x00005555555647ae in rgtk::gtk::rt::main () at /home/lukas/research/rgtk/src/gtk/rt.rs:29
#37 0x000055555555b109 in cairotest::main () at src/cairotest.rs:90
#38 0x0000555555579729 in rust_try_inner ()
#39 0x0000555555579716 in rust_try ()
#40 0x0000555555576ce8 in rt::lang_start::h88fad42b4ac5de27lyJ ()
#41 0x000055555555b255 in main ()

[GuillaumeGomez]

A few signals just break. @jeremyletang you have an idea about what's wrong here ?

[ptersilie]

Since the stuff I am currently working on depends on this, I wouldn't mind fixing this myself. Unfortunately I don't know where to start.

[GuillaumeGomez]

Me neither unfortunately, it's mostly @jeremyletang who worked on it. I think you should start looking at signal.rs (that's where macros are created). It's broken since the old closure system has been removed.

[ptersilie]

Well, the segfault seems to suggest it's this line (signal.rs:90):

let t : &'a Box<super::Signal> = ::std::mem::transmute(signal);

[gkoz]

This is a curious case. Somehow GTK seems to eat the user_data we provide when connecting the signal. I've added some printlns

diff --git a/glib/src/traits.rs b/glib/src/traits.rs
index e86e144..ccc813c 100644
--- a/glib/src/traits.rs
+++ b/glib/src/traits.rs
@@ -66,6 +66,9 @@ pub trait Connect<'a, T: Signal<'a>>: FFIGObject + PhantomFn<&'a T> {
             let mut tmp_signal_name = signal.get_signal_name().replace("_", "-")
                                         .to_tmp_for_borrow();
             let user_data_ptr   = transmute(Box::new(signal));
+
+            println!("connect {:?} {:?}, data={:?}", self.unwrap_gobject(),
+                        tmp_signal_name, user_data_ptr);

             ffi::glue_signal_connect(
                 self.unwrap_gobject(),
diff --git a/src/gtk/signals.rs b/src/gtk/signals.rs
index f33b38f..6e28df5 100644
--- a/src/gtk/signals.rs
+++ b/src/gtk/signals.rs
@@ -87,6 +87,7 @@ macro_rules! signal(

             pub extern fn trampoline<'a>(widget : *mut ffi::C_GtkWidget, $($arg_name : $arg_type),* , signal: *mut Box<super::Signal>) -> $ret_type {
                 unsafe {
+                    println!("signal from {:?} data={:?}", widget, signal);
                     let t : &'a Box<super::Signal> = ::std::mem::transmute(signal);

                     match t.get_user_data() {

and here's what I get:

connect 0x7f1e8c2698d0 "draw", data=0x7f1e8042e000
connect 0x7f1e8c2d0220 "delete-event", data=0x7f1e8042e010
connect 0x7f1e8c2699c0 "draw", data=0x7f1e8042e020
connect 0x7f1e8c2d0470 "delete-event", data=0x7f1e8042e030
signal from 0x7f1e8c2698d0, data=0x0
Illegal instruction (core dumped)

[gkoz]

Well turns out the signal definition in signals.rs is messed up and probably half-done. The actual signal passes the struct by a pointer not by value...

[GuillaumeGomez]

Hum... That's surprising that it did work before the closure change then...

[gkoz]

Okay, I spoke too rashly... The struct in question is a wrapper around the pointer, it's a clever way to get a type conversion for free. So it can work but not always apparently.

[GuillaumeGomez]

What I thought when I tried to fix that a while ago was about the "loss" of the pointer. That's where I got lost.

[gkoz]

This is because of how Rust implements destructors right now. They don't come for free.

struct X;
#[repr(C)]
struct Y(*mut X);
#[repr(C)]
struct Z(*mut X);

impl Drop for Z { fn drop(&mut self) { } }

fn main() {
    println!("sizeof(Y) = {}, sizeof(Z) = {}",
             ::std::mem::size_of::<Y>(),
             ::std::mem::size_of::<Z>());
}

[GuillaumeGomez]

Yes, that's what we want. So why do we have a loss of pointer ?

[gkoz]

cairo::Context has a destructor so you can't just stick it in the trampoline signature instead of a pointer, it will mess up the following arguments, user_data in this case.

[GuillaumeGomez]

Okay, I get it now. Hum... I hate when it get annoying to do "simple" stuff like that.

[gkoz]

I was expecting we'd have to do this the hard way anyway -- do all of the conversions at the FFI boundary explicitly.

[GuillaumeGomez]

I'm not sure this is necessary. I did really dirty stuff like that on other bindings without having to rewrite all the C-code in Rust. I definitely need more time to take care of that.

[gkoz]

Rewrite what? I mean make signal declarations a bit more tedious but in return have the *mut cairo_t be explicitly put into the Context.

[GuillaumeGomez]

Oh okay. I was going way too far away haha.

[gkoz]

@ptersilie You can unblock your progress for now by commenting out this Drop implementation: https://github.com/jeremyletang/rgtk/blob/master/src/cairo/context.rs#L57-63 From reading the GTK reference I don't think it would be appropriate to call cairo_destroy on the callback argument anyway. Other uses of cairo::Context will leak memory of course...

[ptersilie]

Thanks @gkoz and @GuillaumeGomez. :+1: Commenting the Drop implementation worked. I can live with a bit of memory leak for now if that means I can continue working. :) What do you mean with "other uses of cairo::Context though?

[gkoz]

I have no idea if this struct is (or could be) ever used other than in the draw callback. From what I've seen in the reference, in the case of draw we must not call cairo_destroy. Other cases may differ.

[ptersilie]

Ah that makes sense then. Thanks for the explanation. :)

So that means removing that Drop implementation is the proper fix then (in case cairo::Context is never used anywhere else)? Should we commit this fix then? I'm currently testing it and will let you know if I run into issues.

[GuillaumeGomez]

A PR with the Drop implementation commented with the link to this discussion would be nice while we try to find out what's wrong.