Mir

Merge lp:~vanvugt/mir/fix-1510218 into lp:mir

Proposed by Daniel van Vugt on 2015-10-27
Status: Merged
Approved by: Daniel van Vugt on 2015-10-29
Approved revision: 3066
Merged at revision: 3060
Proposed branch: lp:~vanvugt/mir/fix-1510218
Merge into: lp:mir
Diff against target: 221 lines (+122/-31)
5 files modified
src/platforms/android/client/android_client_platform.cpp (+3/-14)
src/platforms/common/client/mir/CMakeLists.txt (+1/-0)
src/platforms/common/client/mir/weak_egl.cpp (+65/-0)
src/platforms/common/client/mir/weak_egl.h (+47/-0)
src/platforms/mesa/client/client_platform.cpp (+6/-17)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-1510218
Reviewer Review Type Date Requested Status
Alberto Aguirre (community) 2015-10-28 Approve on 2015-10-28
Alan Griffiths 2015-10-27 Approve on 2015-10-28
PS Jenkins bot (community) continuous-integration Approve on 2015-10-28
Review via email: mp+275820@code.launchpad.net

Commit message

Avoid __attribute__((weak)) as that will fail to resolve symbols from
lazily loaded libraries (like Qt platform plugins apparently).
(LP: #1510218)

This implementation is written such that the tests introduced in r2771
still work. But if you want a new test specifically for RTLD_LAZY,
that's going to take a whole lot more work. I'm not sure it's worth it
as long as the initial tests from r2771 still work.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~vanvugt/mir/fix-1510218 updated on 2015-10-27
3065. By Daniel van Vugt on 2015-10-27

Header guard!

Alan Griffiths (alan-griffiths) wrote :

code does not need to be inlined in a header

review: Needs Fixing
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~vanvugt/mir/fix-1510218 updated on 2015-10-28
3066. By Daniel van Vugt on 2015-10-28

Don't inline WeakEGL.

It was inlined intentionally because I didn't think there was an appropriate
library to put it in. But then I discovered src/platforms/common/client/

Alan Griffiths (alan-griffiths) wrote :

Nit:

+namespace mir { namespace client {

in a .cpp isn't our "house style"

review: Approve
Alberto Aguirre (albaguirre) wrote :

Given that the API requires an EGLConfig parameter - which needs to be obtained from another call to libEGL, I guess it's not unreasonable to expect the caller of mir_connection_get_egl_pixel_format has linked to libEGL.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/platforms/android/client/android_client_platform.cpp'
2--- src/platforms/android/client/android_client_platform.cpp 2015-07-21 04:30:03 +0000
3+++ src/platforms/android/client/android_client_platform.cpp 2015-10-28 07:23:08 +0000
4@@ -24,6 +24,7 @@
5 #include "android_client_buffer_factory.h"
6 #include "egl_native_surface_interpreter.h"
7
8+#include "mir/weak_egl.h"
9 #include <EGL/egl.h>
10
11 #include <boost/throw_exception.hpp>
12@@ -124,19 +125,6 @@
13 return buf->anwb();
14 }
15
16-/*
17- * Driver modules get dlopened with RTLD_NOW, meaning that if the below egl
18- * functions aren't found in memory the driver fails to load. This would
19- * normally prevent software clients (those not linked to libEGL) from
20- * successfully loading our client module, but if we mark the undefined
21- * egl function symbols as "weak" then their absence is no longer an error,
22- * even with RTLD_NOW.
23- */
24-extern "C" EGLAPI EGLBoolean EGLAPIENTRY
25- eglGetConfigAttrib(EGLDisplay dpy, EGLConfig config,
26- EGLint attribute, EGLint *value)
27- __attribute__((weak));
28-
29 MirPixelFormat mcla::AndroidClientPlatform::get_egl_pixel_format(
30 EGLDisplay disp, EGLConfig conf) const
31 {
32@@ -144,7 +132,8 @@
33
34 // EGL_KHR_platform_android says this will always work...
35 EGLint vis = 0;
36- if (eglGetConfigAttrib(disp, conf, EGL_NATIVE_VISUAL_ID, &vis))
37+ mcl::WeakEGL weak;
38+ if (weak.eglGetConfigAttrib(disp, conf, EGL_NATIVE_VISUAL_ID, &vis))
39 mir_format = mir::graphics::android::to_mir_format(vis);
40
41 return mir_format;
42
43=== modified file 'src/platforms/common/client/mir/CMakeLists.txt'
44--- src/platforms/common/client/mir/CMakeLists.txt 2015-01-19 14:16:46 +0000
45+++ src/platforms/common/client/mir/CMakeLists.txt 2015-10-28 07:23:08 +0000
46@@ -2,4 +2,5 @@
47
48 add_library(client_platform_common OBJECT
49 aging_buffer.cpp
50+ weak_egl.cpp
51 )
52
53=== added file 'src/platforms/common/client/mir/weak_egl.cpp'
54--- src/platforms/common/client/mir/weak_egl.cpp 1970-01-01 00:00:00 +0000
55+++ src/platforms/common/client/mir/weak_egl.cpp 2015-10-28 07:23:08 +0000
56@@ -0,0 +1,65 @@
57+/*
58+ * EGL without any linkage requirements!
59+ * ~~~
60+ * Copyright © 2015 Canonical Ltd.
61+ *
62+ * This program is free software: you can redistribute it and/or modify it
63+ * under the terms of the GNU Lesser General Public License version 3,
64+ * as published by the Free Software Foundation.
65+ *
66+ * This program is distributed in the hope that it will be useful,
67+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
68+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
69+ * GNU Lesser General Public License for more details.
70+ *
71+ * You should have received a copy of the GNU Lesser General Public License
72+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
73+ *
74+ * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com>
75+ */
76+
77+#include "weak_egl.h"
78+#include <dlfcn.h>
79+
80+namespace mir { namespace client {
81+
82+WeakEGL::WeakEGL()
83+ : egl1(nullptr)
84+ , pGetConfigAttrib(nullptr)
85+{
86+}
87+
88+WeakEGL::~WeakEGL()
89+{
90+ if (egl1)
91+ dlclose(egl1);
92+}
93+
94+EGLBoolean WeakEGL::eglGetConfigAttrib(EGLDisplay dpy, EGLConfig config,
95+ EGLint attribute, EGLint* value)
96+{
97+ if (find("eglGetConfigAttrib", (void**)&pGetConfigAttrib))
98+ return pGetConfigAttrib(dpy, config, attribute, value);
99+ else
100+ return EGL_FALSE;
101+}
102+
103+bool WeakEGL::find(char const* name, void** func)
104+{
105+ if (!*func)
106+ {
107+ // RTLD_DEFAULT is first choice to support wrappers like MockEGL
108+ if (!(*func = dlsym(RTLD_DEFAULT, name)))
109+ {
110+ // This will work more in real-world situations if the library
111+ // is hidden behind an RTLD_LOCAL (e.g. a Qt plugin)
112+ if (!egl1)
113+ egl1 = dlopen("libEGL.so.1", RTLD_NOLOAD|RTLD_LAZY);
114+ if (egl1)
115+ *func = dlsym(egl1, name);
116+ }
117+ }
118+ return *func != nullptr;
119+}
120+
121+}} // namespace mir::client
122
123=== added file 'src/platforms/common/client/mir/weak_egl.h'
124--- src/platforms/common/client/mir/weak_egl.h 1970-01-01 00:00:00 +0000
125+++ src/platforms/common/client/mir/weak_egl.h 2015-10-28 07:23:08 +0000
126@@ -0,0 +1,47 @@
127+/*
128+ * EGL without any linkage requirements!
129+ * ~~~
130+ * Copyright © 2015 Canonical Ltd.
131+ *
132+ * This program is free software: you can redistribute it and/or modify it
133+ * under the terms of the GNU Lesser General Public License version 3,
134+ * as published by the Free Software Foundation.
135+ *
136+ * This program is distributed in the hope that it will be useful,
137+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
138+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
139+ * GNU Lesser General Public License for more details.
140+ *
141+ * You should have received a copy of the GNU Lesser General Public License
142+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
143+ *
144+ * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com>
145+ */
146+
147+#ifndef MIR_CLIENT_WEAK_EGL_H_
148+#define MIR_CLIENT_WEAK_EGL_H_
149+
150+#include <EGL/egl.h>
151+#include <dlfcn.h>
152+
153+namespace mir { namespace client {
154+
155+class WeakEGL
156+{
157+public:
158+ WeakEGL();
159+ ~WeakEGL();
160+ EGLBoolean eglGetConfigAttrib(EGLDisplay dpy, EGLConfig config,
161+ EGLint attribute, EGLint* value);
162+
163+private:
164+ bool find(char const* name, void** func);
165+
166+ void* egl1;
167+ EGLBoolean (*pGetConfigAttrib)(EGLDisplay dpy, EGLConfig config,
168+ EGLint attribute, EGLint* value);
169+};
170+
171+}} // namespace mir::client
172+
173+#endif // MIR_CLIENT_WEAK_EGL_H_
174
175=== modified file 'src/platforms/mesa/client/client_platform.cpp'
176--- src/platforms/mesa/client/client_platform.cpp 2015-10-19 12:10:38 +0000
177+++ src/platforms/mesa/client/client_platform.cpp 2015-10-28 07:23:08 +0000
178@@ -23,6 +23,7 @@
179 #include "native_surface.h"
180 #include "mir/client_buffer_factory.h"
181 #include "mir/client_context.h"
182+#include "mir/weak_egl.h"
183 #include "mir_toolkit/mesa/platform_operation.h"
184
185 #include <cstring>
186@@ -163,19 +164,6 @@
187 }
188
189
190-/*
191- * Driver modules get dlopened with RTLD_NOW, meaning that if the below egl
192- * functions aren't found in memory the driver fails to load. This would
193- * normally prevent software clients (those not linked to libEGL) from
194- * successfully loading our client module, but if we mark the undefined
195- * egl function symbols as "weak" then their absence is no longer an error,
196- * even with RTLD_NOW.
197- */
198-extern "C" EGLAPI EGLBoolean EGLAPIENTRY
199- eglGetConfigAttrib(EGLDisplay dpy, EGLConfig config,
200- EGLint attribute, EGLint *value)
201- __attribute__((weak));
202-
203 MirPixelFormat mclm::ClientPlatform::get_egl_pixel_format(
204 EGLDisplay disp, EGLConfig conf) const
205 {
206@@ -192,10 +180,11 @@
207 * EGL_NATIVE_VISUAL_ID, so ignore that for now.
208 */
209 EGLint r = 0, g = 0, b = 0, a = 0;
210- eglGetConfigAttrib(disp, conf, EGL_RED_SIZE, &r);
211- eglGetConfigAttrib(disp, conf, EGL_GREEN_SIZE, &g);
212- eglGetConfigAttrib(disp, conf, EGL_BLUE_SIZE, &b);
213- eglGetConfigAttrib(disp, conf, EGL_ALPHA_SIZE, &a);
214+ mcl::WeakEGL weak;
215+ weak.eglGetConfigAttrib(disp, conf, EGL_RED_SIZE, &r);
216+ weak.eglGetConfigAttrib(disp, conf, EGL_GREEN_SIZE, &g);
217+ weak.eglGetConfigAttrib(disp, conf, EGL_BLUE_SIZE, &b);
218+ weak.eglGetConfigAttrib(disp, conf, EGL_ALPHA_SIZE, &a);
219
220 if (r == 8 && g == 8 && b == 8)
221 {

Subscribers

People subscribed via source and target branches