[waffle] Update on "Don't explicitly link to libraries - use libdl instead"

[Emil Velikov]

On 18 February 2015 at 00:02, Chad Versace <chad.versace at intel.com> wrote:
> On 02/17/2015 03:31 PM, Emil Velikov wrote:
>> On 17/02/15 19:34, Chad Versace wrote:
>>> On 02/12/2015 05:56 AM, Emil Velikov wrote:
>>>> Hello all,
>>>>
>>>> In case you've been wondering what happened with that here it goes:
>>>>
>>>> Currently the following libraries could be nuked
>>>>
>>>>   NEEDED               libwayland-client.so.0
>>>>   NEEDED               libwayland-egl.so.1
>>>>   NEEDED               libX11-xcb.so.1
>>>>   NEEDED               libX11.so.6
>>>>   NEEDED               libxcb.so.1
>>>>   NEEDED               libudev.so.1
>>>>
>>>> From the above we're using the following problematic APIs:
>>>>
>>>> X11/xcb
>>>> * XGetXCBConnection
>>>>
>>>> libwayland-client
>>>> * wl_registry_add_listener
>>>> * wl_shell_surface_add_listener
>>>> * Others ?
>>>>
>>>> On the X11/xcb side we can fix this by transitioning to a xcb only
>>>> implementation, but that means (a) breaking the API/ABI and (b)
>>>> updating all the users (with XLookupString being the messiest).
>>>
>>> We shouldn't break Waffle's X11 API. If you want to expose an XCB-only
>>> backend, then it would make sense to add a new platform type,
>>> WAFFLE_PLATFORM_XCB_EGL, which shares most of its code with
>>> WAFFLE_PLATFORM_X11_EGL.
>>>
>> Hmm I like the idea of WAFFLE_PLATFORM_XCB_EGL. It will ease the
>> transition quite a bit, I will throw it in the Waffle TODO list.
>>
>>> Is there any reason why Waffle can't use dlsym() to get XGetXCBConnection?
>>>
>> Afaict the general idea of using dlopen/dlsym for libX11/libxcb is
>> frowned upon, upon asking in #xorg-devel these are roughly the answers:
>>
>>  - Create libwaffle-x11.so backend which links against the lib* and
>> dlopen that one in your main library.
>>  - Link against SDL, which already dlopen/dlsyms lib*
>>  - You should not do this it's bad, and by bad I mean it'll take a lot
>> of effort.
>>
>> ... all in all no technical reasons behind it.
>>
>> Maybe the people which were trying to help had too many bad experiences
>> with Xlib so they were trying to save me the headaches :P
>
> Thanks for doing all this research. Xlib is old, and the memory of the
> painful havoc it wreaks on those foolish enough to trust it is slow to
> fade.
>
>> Either way I will give it a try and see how badly things behave.
>
> I really don't understand how dlsym'ing XGetXCBConnection is a bad idea...
> Let's just give it a try, and prepare for the future regrets!
>
The more I looked at this, the more I'm inclined to move the current
x11+xcb code to x11 alone, and think/worry about xcb when we need to
use it. Mostly as there are a handful of functions that have not xcb
counterpart, plus interacting with libGL (glX) implies on a couple of
occasions that one must use x11.

>>>> Why problematic - the functions are defined as "static inline" within
>>>> the respective headers, which means that they are expanded during
>>>> compilation. My biggest concern is that (imho) there is no guarantee
>>>> by either library that the current implementation will stay the same,
>>>> thus we cannot manually inline them in waffle.
>>>>
>>>> Should be bite the bullet and do it anyway ? Does anyone know a lovely
>>>> trick to resolve this ?
>>>
>>> There is an easy way to dlsym() the Wayland functions. The inline protocol functions
>>> in wayland-client-protocol.h are all implemented by real library calls to wl_proxy_* functions.
>>> The wl_proxy functions *do* have stable ABI. So, to stop linking to libwayland-client,
>>> for each wl_proxy function used by wayland-client-protocol.h (there's about a dozen),
>>> Waffle needs to provide its own definition of the wl_proxy function that forwards its
>>> arguments to a dlysm() wl_proxy function. Like this:
>>>
>>> void
>>> wl_proxy_add_dispatcher(struct wl_proxy *proxy, wl_dispatcher_func_t dispatcher_func,
>>>                         const void *dispatcher_data, void *data)
>>> {
>>>     // This is a function pointer dlsym'd during waffle_init().
>>>     wfl_wl_proxy_add_dispatcher(prox, dispatcher_func, dispatcher_data, data);
>>> }
>>>
>> Upon closer look adding wrappers might not be our biggest concern, but
>> the data symbols we reference in waffle:
>>
>> $ objdump -CtT libwaffle-1.so | grep "wl_.*_interface"
>>
>> wl_surface_interface
>> wl_shell_surface_interface
>> wl_compositor_interface
>> wl_registry_interface
>> wl_shell_interface
>
> Why are these more of a concern than the inline wrappers? dlsym works on all
> public symbols, including variable symbols. Am I missing something?
>
>> There is some input from Pekka (pq) in the dri logs [1] around 06:52.
>> But until we get that far, I'll be looking and testing the libX11/libxcb
>> side(lack) of things :-)
>
>> [1] http://people.freedesktop.org/~cbrill/dri-log/?channel=wayland&date=2015-02-13
>
> I read the logs. Let me explain some more...
>
> Re-implementing all the wrapper functions will be a big pain, for all the reasons
> described in irc. But I wasn't suggesting that. I was suggesting that we dlsym
> all the real, non-inline functions called by the inline functions in wayland-client-protocol.h,
> and #include wayland-client-protocol.h as-is. As in the example code below.
>
> When linking libwaffle at buildtime, all the symbols called by the inline
> waffle-client-protocol.h functions will be defined by libwaffle itself.
>
> // ------------------------
> // file: wayland-wrapper.h
>
> #include <wayland-client.h>
>
> // This function pointer is dlsym'd during waffle_init().
> extern void
> (*wrapped_wl_proxy_add_dispatcher)(struct wl_proxy *proxy, wl_dispatcher_func_t dispatcher_func,
>                                    const void *dispatcher_data, void *data);
>
> void
> wl_proxy_add_dispatcher(struct wl_proxy *proxy, wl_dispatcher_func_t dispatcher_func,
>                         const void *dispatcher_data, void *data);
>
> // ------------------------
> // file: wayland-wrapper.c
>
> #include "wayland-wrapper.h"
>
> void
> (*wrapped_wl_proxy_add_dispatcher)(struct wl_proxy *proxy, wl_dispatcher_func_t dispatcher_func,
>                                    const void *dispatcher_data, void *data);
>
> void
> wl_proxy_add_dispatcher(struct wl_proxy *proxy, wl_dispatcher_func_t dispatcher_func,
>                         const void *dispatcher_data, void *data)
> {
>     wrapped_wl_proxy_add_dispatcher(prox, dispatcher_func, dispatcher_data, data);
> }
>
That's what I was thinking earlier, yet using this approach to
directly reference data symbols kind of eluded me.
Anywho I've prepped something that compiles, and asked on #wayland for
a charming person to give it a test - valgrind-check-func should be
enough ?

I'll send out the series in a second to gather some input (read
flame), but be warned it's not pretty sight :-P

Cheers,
Emil