Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

Dealing with G_DISABLE_CHECKS #270

Closed
gkoz opened this issue Mar 9, 2016 · 33 comments
Closed

Dealing with G_DISABLE_CHECKS #270

gkoz opened this issue Mar 9, 2016 · 33 comments
Milestone

Comments

@gkoz
Copy link
Member

gkoz commented Mar 9, 2016

GTK+ and other GLib-based libraries rely heavily on g_return_if_fail/g-return-val-if-fail checks for safety.

#define             g_return_if_fail(expr)

Verifies that the expression expr , usually representing a precondition, evaluates to TRUE. If the function returns a value, use g_return_val_if_fail() instead.

These shunts guard against undefined behavior.

If expr evaluates to FALSE, the current function should be considered to have undefined behaviour (a programmer error). The only correct solution to such an error is to change the module that is calling the current function, so that it avoids this incorrect call.

To make this undefined behaviour visible, if expr evaluates to FALSE, the result is usually that a critical message is logged and the current function returns.

But they're optional.

If G_DISABLE_CHECKS is defined then the check is not performed.

If G_DISABLE_CHECKS is set, this crate becomes unable to uphold the safety guarantees. We can't control that option because for technical and licensing reasons gtk-sys links with libgtk-3 dynamically. Even detecting it in a straightforward way doesn't seem possible.

Although the majority of those checks (NULL and object type checks) are superseded by Rust's type system, a significant number (potentially hundreds) remain necessary. Duplicating them all, beside being another handful of tedious error-prone work, would mean targeting not a documented interface but an implementation.

Ideas?

@gkoz
Copy link
Member Author

gkoz commented Mar 9, 2016

It took me some time to track down this packaging guideline:

--enable-debug. Turns on various amounts of debugging support. Setting this to 'no' disables g_assert(), g_return_if_fail(), g_return_val_if_fail() and all cast checks between different object types. Setting it to 'minimum' disables only cast checks. Setting it to 'yes' enables runtime debugging. The default is 'minimum'. Note that 'no' is fast, but dangerous as it tends to destabilize even mostly bug-free software by changing the effect of many bugs from simple warnings into fatal crashes. Thus --enable-debug=no should not be used for stable releases of GLib.

So one might argue that an --enable-debug=no build is itself a bug and we're free to not support such builds. That would mean we can drop checks like this.

It still is concerning to not be able to detect an incorrectly built library. And there is something more deterministic we can do: trigger a segfault in such library at startup. The downside would be that a correctly built version would emit a scary warning, e.g.:

> LD_LIBRARY_PATH=/bad/gtk/lib ./crash-gtk 
Segmentation fault (core dumped)
> ./crash-gtk 

(process:31013): Gtk-CRITICAL **: gtk_main_quit: assertion 'main_loops != NULL' failed
> 

@efyang
Copy link
Contributor

efyang commented Mar 10, 2016

dlsym or the rust equivalent goes by the same search path as the average rust application, right? So I was thinking, one of the functions that is under an

#ifdef G_ENABLE_DEBUG

in gtk is

gtk_arg_debug_cb

Checking the compiled libraries confirms that it is in fact toggled during compilation.

> nm gtkbad.so --demangle | grep gtk_arg_debug_cb
> nm gtkgood.so --demangle | grep gtk_arg_debug_cb
00000000002401c0 t gtk_arg_debug_cb

So at runtime, I was thinking that we could use libloading to load gtk (since the search path should be the same) temporarily, and then check for that function.

@efyang
Copy link
Contributor

efyang commented Mar 10, 2016

I can try this out tomorrow and get back to you if it works.

@gkoz
Copy link
Member Author

gkoz commented Mar 10, 2016

Looks like it doesn't always get exported.

> rpm -q gtk3
gtk3-3.18.7-2.fc23.x86_64
> nm --dynamic --demangle /lib64/libgtk-3.so | grep arg_debug
>

@efyang
Copy link
Contributor

efyang commented Mar 10, 2016

Hmm, I tried diffing the nm --dynamic --demangles (as opposed to just nm --demangle) this time for both of them, got some results that do in fact work here, such as g_return_if_fail_warning, which is probably the most relevant function to this, and it does work for me:

> nm --dynamic --demangle /usr/lib/libgtk-3.so.0 | grep g_return_if_fail_warning
                 U g_return_if_fail_warning
> nm --dynamic --demangle gtkbad.so | grep g_return_if_fail_warning

Would you mind confirming?

@gkoz
Copy link
Member Author

gkoz commented Mar 10, 2016

Yes, that one nm does see but note the U. The symbol is not defined in libgtk-3.so it's provided by libglib-2.0.so so the dlsym way will tell us whether glib was built with G_ENABLE_DEBUG.

@efyang
Copy link
Contributor

efyang commented Mar 10, 2016

Oh ok.

@efyang
Copy link
Contributor

efyang commented Mar 10, 2016

One more idea: gtk only responds to GTK_DEBUG values when debug is enabled. Output is guaranteed to be given if we set GTK_DEBUG=help. So we could run a windowless application like this somehow:

extern crate gtk;

fn main() {
    gtk::init();
    gtk::idle_add(quit);
    gtk::main();
}

fn quit() -> gtk::Continue {
    gtk::main_quit();
    return gtk::Continue(false);
}

with GTK_DEBUG set to help for that specifically and pipe the input and check it.
Some tests of it:

> # Bad GTK
> GTK_DEBUG=help LD_LIBRARY_PATH=/home/honorabrutroll/gitproj/gtkg/gtkbad/gtk/.libs/:/home/honorabrutroll/gitproj/gtkg/gtkbad/gdk/.libs/:$LD_LIBRARY_PATH ./basic
Gtk-Message: Failed to load module "canberra-gtk-module"
> # Good GTK
> GTK_DEBUG=help LD_LIBRARY_PATH=/home/honorabrutroll/gitproj/gtkg/gtkgood/gtk/.libs/:/home/honorabrutroll/gitproj/gtkg/gtkgood/gdk/.libs/:$LD_LIBRARY_PATH ./basic
Supported debug values: misc plugsocket text tree updates keybindings multihead modules geometry icontheme printing builder size-request no-css-cache baselines pixel-cache no-pixel-cache interactive touchscreen actions resize all help
Gtk-Message: Failed to load module "canberra-gtk-module"

Don't really know how we would go about implementing this into the library though.

@gkoz
Copy link
Member Author

gkoz commented Mar 11, 2016

Interesting idea. We could try capturing those logs by overriding the log handler before calling gtk_init and removing the override afterwards. The tricky part would be keeping the debug functionality intact.

@efyang
Copy link
Contributor

efyang commented Mar 11, 2016

Let me see if parse_args has the same effect. That way would prevent ever initializing the window system at all.

@efyang
Copy link
Contributor

efyang commented Mar 11, 2016

It does in fact produce the same output. Code:

extern crate gtk_sys;

use std::ptr;
use gtk_sys::gtk_parse_args;

fn main() {
    unsafe {
        gtk_parse_args(ptr::null_mut(), ptr::null_mut());
    }
}

@efyang
Copy link
Contributor

efyang commented Mar 11, 2016

So basically, we would:

  • Get the current GTK_DEBUG and temporarily store it
  • Set GTK_DEBUG to help
  • Set the log handler to set messages to a pointer (probably going to be the most complicated part)
  • Run parse_args
  • Get the value of the pointer
  • Reset the handler back to its default
  • Set GTK_DEBUG to the stored value

For environment variables, I think the functions from std::env should be enough. If std::env functions don't work, we could always just make and run an os-specific command.

@gkoz
Copy link
Member Author

gkoz commented Mar 11, 2016

I think I've figured out an easier way. gtk_get_debug_flags is public. If it returns non-zero we can be pretty sure debug is enabled. All we need to do is provide such input that it is non-zero, e.g.

0. We're going to piggy-back on themiscflag.

  1. ParseGTK_DEBUG. If it containsmiscorallwithoutmiscthenfake = trueelsefake = false.
    2.if fakecreate fake command-line arguments "--gtk-debug misc" to feed to gtk_parse_args.
  2. Run gtk_parse_args.
  3. if gtk_get_debug_flags() == 0 die.
  4. if fake { gtk_set_debug_flags(gtk_get_debug_flags() & !GTK_DEBUG_MISC) }.
  5. Run gtk_init_check to continue the initialization.

@efyang
Copy link
Contributor

efyang commented Mar 11, 2016

I think we still need to store the GTK_DEBUG variable and set it to en empty string or misc beforehand, as running parse_args with it set to the user given would lead to duplicate outputs (one from parse_args, one from init_check). But otherwise using gtk_get_debug_flags seems like a good idea.

@gkoz
Copy link
Member Author

gkoz commented Mar 11, 2016

Ah but that's the catch with gtk_parse_args: having completed its part of initialization it becomes a no-op.

@efyang
Copy link
Contributor

efyang commented Mar 12, 2016

I made a working prototype here. I used g_parse_debug_string like in the actual gtk pre-initialization, and then saved that value and used it to set the debug flags back to the original before continuing initialization. This way no debug information is lost. In addition, we could probably define some custom flag in gtk_debug_vars which would eliminate any conflict with preexisting gtk flags.

@efyang
Copy link
Contributor

efyang commented Mar 12, 2016

Thus far my tests have been successful:

> LD_LIBRARY_PATH=/home/honorabrutroll/gitproj/gtkg/gtkgood/gtk/.libs/:/home/honorabrutroll/gitproj/gtkg/gtkgood/gdk/.libs/:$LD_LIBRARY_PATH cargo run 
     Running `/home/honorabrutroll/gitproj/gtk-test-debug/target/debug/gtkdebug`
GTK_DEBUG: 
Gtk-Message: Failed to load module "canberra-gtk-module"
debug_flags_1: 1
Working library.
debug_flags_2: 0
> LD_LIBRARY_PATH=/home/honorabrutroll/gitproj/gtkg/gtkbad/gtk/.libs/:/home/honorabrutroll/gitproj/gtkg/gtkbad/gdk/.libs/:$LD_LIBRARY_PATH cargo run  
     Running `/home/honorabrutroll/gitproj/gtk-test-debug/target/debug/gtkdebug`
GTK_DEBUG: 
Gtk-Message: Failed to load module "canberra-gtk-module"
debug_flags_1: 0
thread '<main>' panicked at 'This library was configured with --enable-debug=no', main.rs:67
note: Run with `RUST_BACKTRACE=1` for a backtrace.
Process didn't exit successfully: `/home/honorabrutroll/gitproj/gtk-test-debug/target/debug/gtkdebug` (exit code: 101)
> cargo run
     Running `/home/honorabrutroll/gitproj/gtk-test-debug/target/debug/gtkdebug`
GTK_DEBUG: 
debug_flags_1: 1
Working library.
debug_flags_2: 0

@EPashkin
Copy link
Member

It working on windows with msys2's mingw64/mingw-w64-x86_64-gtk3 3.18.6-1 package.

     Running `target\release\gtkdebug.exe`
GTK_DEBUG:
debug_flags_1: 1
Working library.
debug_flags_2: 0

@gkoz
Copy link
Member Author

gkoz commented Mar 12, 2016

I'm still not sure what benefits maintaining the list of debug options and performing slightly risky set_var have over faking a --gtk-debug misc command-line argument.

fn main() {
    let gtk_debug_var = env::var_os("GTK_DEBUG")
        .map(|s| s.to_string_lossy().to_lowercase())
        .unwrap_or("".to_owned());
    println!("GTK_DEBUG: {}", gtk_debug_var);

    let words = gtk_debug_var.split(&[':', ';', ',', ' ', '\t'][..])
        .collect::<Vec<_>>();
    let has_misc = words.contains(&"all") ^ words.contains(&"misc");

    unsafe {
        let mut argc = 2;
        let mut args = vec![
            b"\0" as *const u8 as *mut c_char,
            b"--gtk-debug=misc\0" as *const u8 as *mut c_char,
        ];
        let mut argv = args.as_mut_ptr();
        gtk_parse_args(&mut argc, &mut argv);
        println!("debug_flags_1: {:?}", GtkDebugFlag::from_bits(gtk_get_debug_flags()));
        if gtk_get_debug_flags() == 0 {
            panic!("This library was configured with --enable-debug=no");
        } else {
            println!("Working library.");
        }
    }

    unsafe {
        if !has_misc {
            gtk_set_debug_flags(gtk_get_debug_flags() & !GTK_DEBUG_MISC.bits());
        }
        println!("debug_flags_2: {:?}", GtkDebugFlag::from_bits(gtk_get_debug_flags()));
        // continue initialization
        gtk_init_check(ptr::null_mut(), ptr::null_mut());
    }
}

@efyang
Copy link
Contributor

efyang commented Mar 12, 2016

Yeah that looks like a far more maintainable solution, and it does work with all of my tests. I just wasn't sure how using & would be able to remove it from the flags (I also tried it this way but probably messed up somewhere with the argv pointer and got segfaults).

@efyang
Copy link
Contributor

efyang commented Mar 12, 2016

Ah so gtk would be storing the flags in set sections of the integer itself. That makes a lot more sense why you would be using binary ops now (I haven't seen or used bitflags before now).

@efyang
Copy link
Contributor

efyang commented Mar 13, 2016

So I assume you'll be putting this in a PR @gkoz?

@gkoz
Copy link
Member Author

gkoz commented Mar 14, 2016

@efyang I don't want to push this into the upcoming 0.0.7 release so sometime after.

@mwm
Copy link

mwm commented Apr 20, 2016

This breaks things on FreeBSD, as they configure libgtk with --disable-debug.

I note that the referenced document is for glib, not libgtk, and they do build that with the default debug level. I have reached out to the maintainers about this issue.

I've verified that building libgtk without specifying a debug level works properly. I'll be running with that while I continue to chase issues in the behavior of gtk. It'll be interesting to see how the system behavior changes.

@gkoz
Copy link
Member Author

gkoz commented Apr 20, 2016

The recommendation for libgtk-3 is the same: https://developer.gnome.org/gtk3/stable/gtk-building.html#extra-configuration-options

@kennytm
Copy link
Contributor

kennytm commented Jun 19, 2016

Homebrew's gtk+3 on OS X --disable-debug by default. This can be changed locally with

  1. brew edit gtk+3 and delete the offending line
  2. brew reinstall --build-from-source gtk+3

(filed as Homebrew/homebrew-core#2142)

@talklittle
Copy link
Contributor

talklittle commented Nov 12, 2016

Apparently affects libgtk on Ubuntu too.

Filed https://bugs.launchpad.net/ubuntu/+source/gtk+3.0/+bug/1641358

Edit: Nevermind, not a bug in Ubuntu's libgtk configuration. It is already using the default, equivalent to --enable-debug=minimum. See #405 for more info on the bug I am seeing.

@cars10
Copy link

cars10 commented Feb 11, 2017

I have a problem with that, too. I try to run tests in cargo with functions using gtk, see the following gist:
https://gist.github.com/cars10/35f9252ad812266040dab5ff9b96df6a

This fails with various error messages, sometimes referencing to this bug. Weird thing is, if i comment out one of the tests it works fine - the problem seems to be running gtk::init() in more then one test. Any hints?

@EPashkin
Copy link
Member

@cars10 try disable running tests in many treads https://internals.rust-lang.org/t/disable-parallel-tests/1592

@cars10
Copy link

cars10 commented Feb 11, 2017

Then it's even more weird: running with RUST_TEST_THREADS=1 cargo test panicks with the following

panicked at 'Attempted to initialize GTK from two different threads.'

@EPashkin
Copy link
Member

After I add println!("{:?}", std::thread::current().name()); to second test, I get Some("tests::test2")
Seems test framework run each test in new thread even if RUST_TEST_THREADS=1 set.
Until way don't spawn new thread for test found, you need run all your tests from one #[test] function

@cars10
Copy link

cars10 commented Feb 17, 2017

That sucks. Thanks.,

@EPashkin
Copy link
Member

EPashkin commented Apr 1, 2017

@cars10 I was stupid, we can use https://doc.rust-lang.org/book/testing.html#the-tests-directory.
Bad thing that travis have non-debug build gtk (or I forgot set some flags)
See #483

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants