Merge lp:~afrantzis/mir/mir-client-ensure-global-symbol-resolution into lp:mir

[Alexandros Frantzis]

client: Provide a way to ensure libmirclient's symbols are available for symbol resolution in other shared objects

This is useful if libmirclient, or another library linking to libmirclient, is loaded
at runtime with dlopen() using RTLD_LOCAL (e.g., through a plugin mechanism)
and libmirclient's symbols need to be resolved at runtime in other shared objects
(like Mesa's libEGL).

client: Provide a way to ensure libmirclient's symbols are available for symbol resolution in other shared objects

This is useful if libmirclient, or another library linking to libmirclient, is loaded
at runtime with dlopen() using RTLD_LOCAL (e.g., through a plugin mechanism)
and libmirclient's symbols need to be resolved at runtime in other shared objects
(like Mesa's libEGL).

The specific problem that we are trying to solve is that with Qt client applications on the desktop, EGL fails to recognize native MirConnections as valid EGLNativeDisplayType objects, falling back to X11 and crashing. The reason is that the symbols needed for the MirConnection validation and which are exported by libmirclient, are not available to libEGL for dynamic runtime resolution. The problem occurs because Qt loads the platform shared object at runtime with local symbol visibility (RTLD_LOCAL), which is reasonable in general, but not suitable for our use case.

(See reference diagram: http://people.ubuntu.com/~afrantzis/mir-egl-symbol-resolution.png for the following)
Concerning our options for using this:

Ideally we would want to deal with the issue in libEGL, but unfortunately we cannot, since it would defeat the purpose of the runtime resolution, which is to transparently support applications linking against either libmirserver or libmirclient or both (or neither) and doing the right thing.

Next choice would be the Qt platform plugin (libqubuntumirclient), but the plugin doesn't have any knowledge of libmirclient, it's all abstracted through platform-api. We could make it work by linking with libmirclient at either runtime or loadtime and call the mir_client_ensure_global_symbol_resolution() from here.

Moving down a step we reach the platform-api from where we do have direct access to libmirclient. It's easy to call mir_client_ensure_global_symbol_resolution() from here. The trade-off is that we are then going to force this behavior on all users of the platform-api's mirclient backend (which might be OK).

We could even make libmirclient itself force all of its symbols to become available for global resolution, but this seems too intrusive for now (but it is an option if we find that this problem is widespread).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1247
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/415/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/253/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/250
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/144
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/144/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/144/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/415/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1247
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/417/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/255/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/252
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/146
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/146/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/146/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/417/rebuild

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

The CI failures are caused by CI scripts not disabling the new test category (special-tests). It only makes sense to change CI if the MP is approved, so please review to break the vicious circle.

[Alan Griffiths (alan-griffiths)]

Wouldn't it be as easy (and have mess in fewer places) for platform-api to call dlopen() directly?

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> Wouldn't it be as easy (and have mess in fewer places) for platform-api to call dlopen() directly?

That's another option, but:

1. platform-api (or wherever this ends up in) will need to contain the logic to load the correct libmirserver.so.X library, which is not as straightforward as doing it in mirclient (since we know our soname)
2. it exposes the mechanism by which this is performed which platform-api doesn't really care about
3. it will not be as easy to reuse the code in other similar cases or move the point of application

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1248
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/419/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/258/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/255
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/148
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/148/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/148
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/148/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/419/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1249
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/422/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/262
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/259
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/151
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/151/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/151
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/151/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/422/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> > Wouldn't it be as easy (and have mess in fewer places) for platform-api to
> call dlopen() directly?
>
> That's another option, but:
>
> 1. platform-api (or wherever this ends up in) will need to contain the logic
> to load the correct libmirserver.so.X library, which is not as straightforward
> as doing it in mirclient (since we know our soname)
> 2. it exposes the mechanism by which this is performed which platform-api
> doesn't really care about
> 3. it will not be as easy to reuse the code in other similar cases or move the
> point of application

OK then

review: Approve

[Daniel van Vugt (vanvugt)]

I'm concerned, because using RTLD_LAZY|RTLD_GLOBAL is usually a bad sign that you've done something wrong. You should never need either. But you seem to have good reasons I don't yet fully understand.

review: Abstain

[Alan Griffiths (alan-griffiths)]

Actually, having slept at the weekend I'm concerned that "MIR_ENABLE_SPECIAL_TESTS" is a horribly uninformative name.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> Actually, having slept at the weekend I'm concerned that
> "MIR_ENABLE_SPECIAL_TESTS" is a horribly uninformative name.

The name was vague on purpose so that this category could include all kinds of tests that have special needs, mostly in terms of special build flags or linking requirements. For example I was thinking that the c89 test we have could be also placed here.

Alternatively, we could just put this in acceptance tests (it is an acceptance test after all), but we need to have a different executable due to the special linking, which is why I have opted to separate it.

[Alan Griffiths (alan-griffiths)]

> > Actually, having slept at the weekend I'm concerned that
> > "MIR_ENABLE_SPECIAL_TESTS" is a horribly uninformative name.
>
> The name was vague on purpose so that this category could include all kinds of
> tests that have special needs, mostly in terms of special build flags or
> linking requirements. For example I was thinking that the c89 test we have
> could be also placed here.
>
> Alternatively, we could just put this in acceptance tests (it is an acceptance
> test after all), but we need to have a different executable due to the special
> linking, which is why I have opted to separate it.

Hmm, or our so-called "unit-tests" that need an LD_PRELOAD to work.

I see the motivation, but I'm not sure these form a coherent "SPECIAL" category. Amy trying to come up with a name I like...

[Alan Griffiths (alan-griffiths)]

> I see the motivation, but I'm not sure these form a coherent "SPECIAL"
> category. Amy trying to come up with a name I like...

On IRC we came up with MIR_ENABLE_SPECIAL_LINKAGE_TESTS

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1250
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/435/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/284
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/281
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/164
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/164/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/164
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/164/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/435/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Ugly, but appears to address the problem. (And haven't thought of a more elegant solution within code we "own".)

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/185/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/313/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/310/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/88/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/88/console

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

1. Needs Fixing: Text conflict in debian/rules

2. Needs information:
"The specific problem that we are trying to solve is that with Qt client applications on the desktop, EGL fails to recognize native MirConnections as valid EGLNativeDisplayType objects, falling back to X11 and crashing. The reason is that the symbols needed for the MirConnection validation and which are exported by libmirclient, are not available to libEGL for dynamic runtime resolution."
How is failing to find a symbol not a fatal error? If we need validation functions and presently can't find them then what function is being called?

Experience with many global/local symbol resolution problems in plugin systems tells me there's possibly a nicer solution but I still don't fully understand the problem being solved.

review: Needs Fixing

[Chris Halse Rogers (raof)]

Hm. Looks like this could be more awesomely solved in the Mir EGL platform with judicious use of RTLD_NOLOAD;
first try resolving client & server symbols, then if neither are resolved checking whether either mirclient or mirserver are loaded with RTLD_NOLOAD, and reopening them with RTLD_GLOBAL if so.

It's only available on glibc, but whatever.

Longer term it might be nicer to dynamically load the EGL platforms and use actual link-time linking.

[Chris Halse Rogers (raof)]

I also don't object to the existing approach, except that we'll want to drop it when we solve this correctly.

[Kevin DuBois (kdub)]

Is this still active?

Unmerged revisions

1250. By Alexandros Frantzis on 2013-11-25

tests: Rename special-tests to special-linkage-tests

1249. By Alexandros Frantzis on 2013-11-22

special-tests: Add explicit dependency to mirclient library

1248. By Alexandros Frantzis on 2013-11-21

build: Disable special tests when building the package for armhf

1247. By Alexandros Frantzis on 2013-11-21

client: Provide function to ensure libmirclient's symbols are available for symbol resolution in other shared objects

This is useful if libmirclient, or another library linking to libmirclient, is loaded
at runtime with dlopen() using RTLD_LOCAL (e.g., through a plugin mechanism)
and libmirclient's symbols need to be resolved at runtime in other shared objects
(like Mesa's libEGL).

=== modified file 'debian/rules'
--- debian/rules	2013-11-21 03:16:21 +0000
+++ debian/rules	2013-11-25 14:25:30 +0000
@@ -24,7 +24,8 @@
 	  -DMIR_PLATFORM=android \
 	  -DMIR_ENABLE_UNIT_TESTS=NO \
 	  -DMIR_ENABLE_ACCEPTANCE_TESTS=NO \
-	  -DMIR_ENABLE_INTEGRATION_TESTS=NO
+	  -DMIR_ENABLE_INTEGRATION_TESTS=NO \
+	  -DMIR_ENABLE_SPECIAL_LINKAGE_TESTS=NO
 else
 ifeq ($(DEB_HOST_ARCH),powerpc)
 	dh_auto_configure -- \

=== added file 'include/client/mir_toolkit/mir_client_ensure_global_symbol_resolution.h'
--- include/client/mir_toolkit/mir_client_ensure_global_symbol_resolution.h	1970-01-01 00:00:00 +0000
+++ include/client/mir_toolkit/mir_client_ensure_global_symbol_resolution.h	2013-11-25 14:25:30 +0000
@@ -0,0 +1,40 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef MIR_CLIENT_ENSURE_GLOBAL_SYMBOL_RESOLUTION_H
+#define MIR_CLIENT_ENSURE_GLOBAL_SYMBOL_RESOLUTION_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * Ensure the symbols defined by libmirclient are available for symbol
+ * resolution in other shared objects.
+ *
+ * This is useful if libmirclient, or another library linking to libmirclient,
+ * is loaded at runtime with dlopen() using RTLD_LOCAL (e.g., through a plugin
+ * mechanism) and libmirclient's symbols need to be resolved at runtime in
+ * other shared objects (like Mesa's libEGL).
+ *
+ */
+void mir_client_ensure_global_symbol_resolution();
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* MIR_CLIENT_ENSURE_GLOBAL_SYMBOL_RESOLUTION_H */

=== modified file 'src/client/CMakeLists.txt'
--- src/client/CMakeLists.txt	2013-11-21 03:16:21 +0000
+++ src/client/CMakeLists.txt	2013-11-25 14:25:30 +0000
@@ -33,6 +33,7 @@
   default_connection_configuration.cpp
   surface_map.cpp
   lifecycle_control.cpp
+  mir_client_ensure_global_symbol_resolution.cpp
 )
 
 if( MIR_PLATFORM STREQUAL "android")
@@ -76,6 +77,7 @@
 
   PROPERTIES
   SOVERSION ${MIRCLIENT_ABI}
+  COMPILE_DEFINITIONS MIRCLIENT_LIBRARY_NAME=\"$<TARGET_SONAME_FILE_NAME:mirclient>\"
 )
 
 # Ensure mirclient_compat_links exist before any dependents of mirclient

=== added file 'src/client/mir_client_ensure_global_symbol_resolution.cpp'
--- src/client/mir_client_ensure_global_symbol_resolution.cpp	1970-01-01 00:00:00 +0000
+++ src/client/mir_client_ensure_global_symbol_resolution.cpp	2013-11-25 14:25:30 +0000
@@ -0,0 +1,28 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "mir_toolkit/mir_client_ensure_global_symbol_resolution.h"
+
+#include "dlfcn.h"
+
+void mir_client_ensure_global_symbol_resolution()
+{
+    void* handle = dlopen(MIRCLIENT_LIBRARY_NAME, RTLD_LAZY|RTLD_GLOBAL);
+    if (handle)
+        dlclose(handle);
+}

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2013-10-15 10:10:05 +0000
+++ tests/CMakeLists.txt	2013-11-25 14:25:30 +0000
@@ -22,6 +22,7 @@
 option(MIR_ENABLE_ACCEPTANCE_TESTS "Build & run acceptance tests" ON)
 option(MIR_ENABLE_INTEGRATION_TESTS "Build & run integration tests" ON)
 option(MIR_ENABLE_UNIT_TESTS "Build & run unit tests" ON)
+option(MIR_ENABLE_SPECIAL_LINKAGE_TESTS "Build & run special linkage tests" ON)
 
 if (MIR_ENABLE_ACCEPTANCE_TESTS)
   add_subdirectory(acceptance-tests/)
@@ -35,6 +36,10 @@
   add_subdirectory(integration-tests/)
 endif (MIR_ENABLE_INTEGRATION_TESTS)
 
+if (MIR_ENABLE_SPECIAL_LINKAGE_TESTS)
+  add_subdirectory(special-linkage-tests/)
+endif (MIR_ENABLE_SPECIAL_LINKAGE_TESTS)
+
 add_subdirectory(mir_test/)
 add_subdirectory(mir_test_framework/)
 add_subdirectory(mir_test_doubles/)

=== added directory 'tests/special-linkage-tests'
=== added file 'tests/special-linkage-tests/CMakeLists.txt'
--- tests/special-linkage-tests/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ tests/special-linkage-tests/CMakeLists.txt	2013-11-25 14:25:30 +0000
@@ -0,0 +1,24 @@
+add_executable(
+  symbol-resolution-tests
+
+  test_client_symbol_resolution.cpp
+)
+
+
+target_link_libraries(
+  symbol-resolution-tests
+
+  ${GTEST_BOTH_LIBRARIES}
+  -ldl
+)
+
+set_target_properties(
+  symbol-resolution-tests
+
+  PROPERTIES
+  COMPILE_DEFINITIONS MIRCLIENT_LIBRARY_FILE=\"$<TARGET_SONAME_FILE:mirclient>\"
+)
+
+add_dependencies(symbol-resolution-tests mirclient)
+
+mir_discover_tests(symbol-resolution-tests)

=== added file 'tests/special-linkage-tests/test_client_symbol_resolution.cpp'
--- tests/special-linkage-tests/test_client_symbol_resolution.cpp	1970-01-01 00:00:00 +0000
+++ tests/special-linkage-tests/test_client_symbol_resolution.cpp	2013-11-25 14:25:30 +0000
@@ -0,0 +1,50 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#include <gtest/gtest.h>
+#include <dlfcn.h>
+
+typedef void (ensure_global_symbol_resolution_func_t)();
+
+TEST(ClientSymbolResolutionTest, can_find_client_symbols_after_ensuring_global_symbol_resolution)
+{
+    char const* const client_symbol = "mir_connect_sync";
+    void* client_lib = dlopen(MIRCLIENT_LIBRARY_FILE, RTLD_LAZY);
+    void* program = dlopen(NULL, RTLD_LAZY);
+
+    ASSERT_TRUE(client_lib != nullptr);
+    ASSERT_TRUE(program != nullptr);
+
+    /* Client symbols aren't available yet through the program's handle */
+    EXPECT_TRUE(dlsym(program, client_symbol) == nullptr);
+
+    void* sym = dlsym(client_lib, "mir_client_ensure_global_symbol_resolution");
+    /*
+     * This ugly trick is needed to silence warnings about converting from void* to
+     * function pointer.
+     */
+    auto ensure_global_symbol_resolution =
+        *reinterpret_cast<ensure_global_symbol_resolution_func_t**>(&sym);
+    ASSERT_TRUE(ensure_global_symbol_resolution != nullptr);
+
+    ensure_global_symbol_resolution();
+    EXPECT_TRUE(dlsym(program, client_symbol) != nullptr);
+
+    dlclose(program);
+    dlclose(client_lib);
+}