Merge lp:~vanvugt/compiz-core/fix-938478 into lp:compiz-core

[Daniel van Vugt]

Detect unresolvable symbols at load time (and so fail gracefully), instead
of waiting till run-time and allowing them to cause a crash. (LP: #938478)

[Mikkel Kamstrup Erlandsen (kamstrup)]

I am wondering if this will impact compiz startup time?

[Daniel van Vugt (vanvugt)]

I think you'd be looking at microseconds difference. Certainly it's no slower in testing.

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[Sam Spilsbury (smspillaz)]

This will massively increase startup time with the unity plugin.

Just let it crash, symbol mismatches should be a fatal error.

review: Disapprove

[Sam Spilsbury (smspillaz)]

I'll admit I'm speaking too fast. Let me do some measurements to check here. I remember even doing dlopen on unity takes about a second or two and unity + gtk + nux + sigc means a few thousand symbols to resolve.

[Sam Spilsbury (smspillaz)]

review: Needs Information

[Sam Spilsbury (smspillaz)]

LAZY COLD:

Using host libthread_db library "/lib/i386-linux-gnu/libthread_db.so.1".
opening /home/smspillaz/Applications/Compiz/lib/compiz/libccp.so ...
 time taken : -260 ms
[New Thread 0xb7532b40 (LWP 2353)]
Backend : gconf
Integration : true
Profile : unity
Adding plugins
Initializing core options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libcomposite.so ...
 time taken : -125 ms
Initializing composite options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libopengl.so ...
 time taken : -179 ms
Initializing opengl options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libdecor.so ...
 time taken : -139 ms
Initializing decor options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libmove.so ...
 time taken : -104 ms
Initializing move options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libplace.so ...
 time taken : -80 ms
Initializing place options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libimgpng.so ...
 time taken : -25 ms
opening /home/smspillaz/Applications/Compiz/lib/compiz/libmousepoll.so ...
 time taken : -60 ms
Initializing mousepoll options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libregex.so ...
 time taken : -20 ms
opening /home/smspillaz/Applications/Compiz/lib/compiz/libsession.so ...
 time taken : -60 ms
Initializing session options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libunitymtgrabhandles.so ...
 time taken : -1366 ms
Initializing unitymtgrabhandles options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libwall.so ...
 time taken : -85 ms
Initializing wall options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libvpswitch.so ...
 time taken : -77 ms
Initializing vpswitch options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libanimation.so ...
 time taken : -292 ms
Initializing animation options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libcompiztoolbox.so ...
 time taken : -64 ms
opening /home/smspillaz/Applications/Compiz/lib/compiz/libgnomecompat.so ...
 time taken : -68 ms
Initializing gnomecompat options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libgrid.so ...
 time taken : -99 ms
Initializing grid options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libresize.so ...
 time taken : -108 ms
Initializing resize options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libsnap.so ...
 time taken : -92 ms
Initializing snap options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libexpo.so ...
 time taken : -78 ms
Initializing expo options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libfade.so ...
 time taken : -90 ms
Initializing fade options...done
opening /home/smspillaz/.compiz-1/plugins/libworkarounds.so ...
 time taken : -177 ms
Initializing workarounds options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libscale.so ...
 time taken : -108 ms
Initializing scale options...done
opening /home/smspillaz/Applications/Compiz/lib/compiz/libezoom.so ...
 time taken : -188 ms
Initializing ezoom options...done
ope...

[Mikkel Kamstrup Erlandsen (kamstrup)]

libunitymtgrabhandles.so ...
 time taken : -1303 ms

libunityshell.so ...
 time taken : -5415 ms

da-yum boy!

Are you sure this is a stripped build? Also - just how many symbols
are these libs exporting? If I understand correctly they shouldn't
need to export any other symbols than those for the compiz plugin
right?

[Mikkel Kamstrup Erlandsen (kamstrup)]

Answering my own question:

$ nm -DC --defined-only /usr/lib/compiz/libmove.so | wc -l
195

$ nm -DC --defined-only /usr/lib/compiz/libunitymtgrabhandles.so | wc -l
500

$ nm -DC --defined-only /usr/lib/compiz/libanimation.so | wc -l
1054

$ nm -DC --defined-only /usr/lib/compiz/libunityshell.so | wc -l
5384

And for reference, what I expect from something with the "plugin" moniker:

$ nm -DC --defined-only /usr/lib/i386-linux-gnu/gio/modules/libgiognutls.so | wc -l
3

[Sam Spilsbury (smspillaz)]

No, you're right, they aren't stripped.

I can re-run the benchmarks on stripped libraries if you think its appropriate.

[Daniel van Vugt (vanvugt)]

OK, so the impact on start-up, best we can tell is 0.12 seconds (worst case with Unity). I think that's worthwhile to gain reliable error handling and avoid mysterious (albeit unlikely) crashes. But it's not important enough to waste too much time debating.

Unity taking 5 seconds to start up however is definitely worth investigating outside of this discussion.

[Sam Spilsbury (smspillaz)]

On Thu, 23 Feb 2012, Daniel van Vugt wrote:

> OK, so the impact on start-up, best we can tell is 0.12 seconds (worst case with Unity). I think that's worthwhile to gain reliable error handling and avoid mysterious (albeit unlikely) crashes. But it's not important enough to waste too much time debating.
>
> Unity taking 5 seconds to start up however is definitely worth investigating outside of this discussion.

What do you think about using RTLD_NOW in --debug ?

I think that alan_g was working on getting the plugin loader under test,
we could probably unit test loading all the plugins as part of our
testsuite, that would catch any undefined symbols in an automated way.

> --
> https://code.launchpad.net/~vanvugt/compiz-core/fix-938478/+merge/94098
> You are reviewing the proposed merge of lp:~vanvugt/compiz-core/fix-938478 into lp:compiz-core.
>

[Daniel van Vugt (vanvugt)]

I think the feature needs to be always enabled, so it catches the mistakes in all cases including in-the-wild.

If you're only enabling it in debug mode you may as well use "env LD_BIND_NOW=1" instead.

[Alan Griffiths (alan-griffiths)]

> What do you think about using RTLD_NOW in --debug ?

Differences between debug and normal use always cause pain.

I've seen systems where plugins can be flagged as "trusted" or "untrusted" and loaded with more or less paranoid options accordingly. So a separate option might make sense. Vis:

compiz plug1 plug2 --load-check plug3 --no-load-check plug4

But that's probably too much work for the problem.

> I think that alan_g was working on getting the plugin loader under test,

The plugin loader code is under test - but to do that the file system is mocked out.

> we could probably unit test loading all the plugins as part of our
> testsuite, that would catch any undefined symbols in an automated way.

It isn't really a /unit/ test - and the point of a plugin architecture is that "all the plugins" isn't a set known at build time.

Also, to emulate the symbols available at runtime requires a pretty complete replication of the runtime binary. (Easier done as a command line switch?)

[Sam Spilsbury (smspillaz)]

> > What do you think about using RTLD_NOW in --debug ?
>
> Differences between debug and normal use always cause pain.
>
> I've seen systems where plugins can be flagged as "trusted" or "untrusted" and
> loaded with more or less paranoid options accordingly. So a separate option
> might make sense. Vis:
>
> compiz plug1 plug2 --load-check plug3 --no-load-check plug4
>
> But that's probably too much work for the problem.

I think the reality here is that at least in the Ubuntu usecase, the user is going to be running with a fixed set of plugins which are known to be not-bad at release.

>
> > I think that alan_g was working on getting the plugin loader under test,
>
> The plugin loader code is under test - but to do that the file system is
> mocked out.
>
> > we could probably unit test loading all the plugins as part of our
> > testsuite, that would catch any undefined symbols in an automated way.
>
> It isn't really a /unit/ test - and the point of a plugin architecture is that
> "all the plugins" isn't a set known at build time.
>
> Also, to emulate the symbols available at runtime requires a pretty complete
> replication of the runtime binary. (Easier done as a command line switch?)

The plugins are linking to libcompiz_core now, so I don't really foresee any problems with making a simple test which takes the plugin loading part, links libcompiz_core and uses dlopen with RLTD_NOW to check for unresolved symbols. We could even make it part of the build process.

In any case.

review: Abstain

[Daniel van Vugt (vanvugt)]

I'm leaning toward merging this so that we have proper checking every time compiz starts. The worst case start-up delay this has been observed to introduce is 0.12 seconds, which is insignificant compared to the 5 seconds it takes unityshell to load.

[Alan Griffiths (alan-griffiths)]

> I'm leaning toward merging this so that we have proper checking every time
> compiz starts. The worst case start-up delay this has been observed to
> introduce is 0.12 seconds, which is insignificant compared to the 5 seconds it
> takes unityshell to load.

+1 - I prefer to have a clear diagnostic to startup speed followed by a later crash.

=== modified file 'src/plugin.cpp'
--- src/plugin.cpp	2012-02-16 05:31:28 +0000
+++ src/plugin.cpp	2012-02-22 08:21:23 +0000
@@ -159,7 +159,7 @@
 	return false;
     }
 
-    int open_flags = RTLD_LAZY;
+    int open_flags = RTLD_NOW;
 #ifdef DEBUG
     // Do not unload the library during dlclose.
     open_flags |= RTLD_NODELETE;