Mir

Merge lp:~andreas-pokorny/mir/fix-1506137 into lp:mir

Proposed by Andreas Pokorny
Status: Merged
Approved by: Andreas Pokorny
Approved revision: no longer in the source branch.
Merged at revision: 3038
Proposed branch: lp:~andreas-pokorny/mir/fix-1506137
Merge into: lp:mir
Diff against target: 100 lines (+14/-24)
3 files modified
src/platforms/mesa/client/client_platform.cpp (+0/-16)
src/platforms/mesa/client/client_platform_factory.cpp (+11/-0)
src/platforms/mesa/server/kms/platform_symbols.cpp (+3/-8)
To merge this branch: bzr merge lp:~andreas-pokorny/mir/fix-1506137
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Daniel van Vugt Abstain
Kevin DuBois (community) Approve
Alan Griffiths Approve
Review via email: mp+274708@code.launchpad.net

Commit message

Delay mesa hack: reloading of module only needed for nested mesa platform

The mesa hack has horrible side effects - on krillin it makes mtks mali-driver frequently segfault during module unload.

Description of the change

This moves the reloading of the module to a later stage.

We only need the mesa hack when we want to make mesa run on top of a mir server - so this is currently only in the creation code of the guest platform.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

[----------] Global test environment tear-down
[==========] 1850 tests from 237 test cases ran. (238755 ms total)
[ PASSED ] 1849 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] ThreadedDispatcherSignalTest.keeps_dispatching_after_signal_interruption

1 FAILED TEST

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

no segfault on krillin.. rerunning..

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

mako this time .. rerunning

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

Failure to add ppa.. Rerunning

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

now a mako setup failure..

bleh I want to see at least two krillin runs without segfault.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

^ ok now at least three krillin runs without seg fault, and one of them with a different ci problem.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Hmm, this looks familiar. A month or two back I couldn't propose some (harmless) changes because of spurious segfaults in GL on at least one droid. I wonder if this was it...

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Sounds like the right idea.

review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Nit:

-extern "C" int __attribute__((constructor))
+extern "C" int

No need for the "extern "C" int" - that was only needed for "__attribute__((constructor))" can just be void.

This makes me wonder if we can do something similar in mesa/client/client_platform.cpp?

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

alright

review: Approve
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

> Nit:
>
> -extern "C" int __attribute__((constructor))
> +extern "C" int
>
> No need for the "extern "C" int" - that was only needed for
> "__attribute__((constructor))" can just be void.
>
> This makes me wonder if we can do something similar in
> mesa/client/client_platform.cpp?

I havent noticed.. but yes did that. The first time we need that is when we want to get setup egl and the display.. and this is when mesa tries to access the mir symbols..

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Nit:

-extern "C" int __attribute__((constructor))
+extern "C" int

No need for the "extern "C" int" - that was only needed for "__attribute__((constructor))" can just be void.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

> Nit:
>
> -extern "C" int __attribute__((constructor))
> +extern "C" int
>
> No need for the "extern "C" int" - that was only needed for
> "__attribute__((constructor))" can just be void.

aye.. missed that part..

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I think this is no longer required since gcc-4.9... ?

// Cast dladdr itself to work around g++-4.8 warnings (LP: #1366134)
typedef int (safe_dladdr_t)(void(*func)(), Dl_info *info);

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) :
review: Abstain
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

Autolanding failure due to problems installing the PPA.

Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/platforms/mesa/client/client_platform.cpp'
2--- src/platforms/mesa/client/client_platform.cpp 2015-07-17 08:08:44 +0000
3+++ src/platforms/mesa/client/client_platform.cpp 2015-10-20 05:41:35 +0000
4@@ -26,7 +26,6 @@
5 #include "mir_toolkit/mesa/platform_operation.h"
6
7 #include <cstring>
8-#include <dlfcn.h>
9
10 namespace mcl=mir::client;
11 namespace mclm=mir::client::mesa;
12@@ -55,21 +54,6 @@
13 {
14 return ((a - 1) / b) + 1;
15 }
16-
17-// Hack around the way mesa loads mir: This hack makes the
18-// necessary symbols global.
19-extern "C" int __attribute__((constructor))
20-ensure_loaded_with_rtld_global_mesa_client()
21-{
22- Dl_info info;
23-
24- // Cast dladdr itself to work around g++-4.8 warnings (LP: #1366134)
25- typedef int (safe_dladdr_t)(int(*func)(), Dl_info *info);
26- safe_dladdr_t *safe_dladdr = (safe_dladdr_t*)&dladdr;
27- safe_dladdr(&ensure_loaded_with_rtld_global_mesa_client, &info);
28- dlopen(info.dli_fname, RTLD_NOW | RTLD_NOLOAD | RTLD_GLOBAL);
29- return 0;
30-}
31 }
32
33 mclm::ClientPlatform::ClientPlatform(
34
35=== modified file 'src/platforms/mesa/client/client_platform_factory.cpp'
36--- src/platforms/mesa/client/client_platform_factory.cpp 2015-07-16 08:13:28 +0000
37+++ src/platforms/mesa/client/client_platform_factory.cpp 2015-10-20 05:41:35 +0000
38@@ -25,6 +25,7 @@
39
40 #include <sys/mman.h>
41 #include <unistd.h>
42+#include <dlfcn.h>
43 #include <stdexcept>
44 #include <boost/throw_exception.hpp>
45
46@@ -33,6 +34,15 @@
47
48 namespace
49 {
50+// Hack around the way mesa loads mir: This hack makes the
51+// necessary symbols global.
52+void ensure_loaded_with_rtld_global_mesa_client()
53+{
54+ Dl_info info;
55+
56+ dladdr(reinterpret_cast<void*>(&ensure_loaded_with_rtld_global_mesa_client), &info);
57+ dlopen(info.dli_fname, RTLD_NOW | RTLD_NOLOAD | RTLD_GLOBAL);
58+}
59
60 struct RealBufferFileOps : public mclm::BufferFileOps
61 {
62@@ -63,6 +73,7 @@
63
64 std::shared_ptr<mcl::ClientPlatform> create_client_platform(mcl::ClientContext* context)
65 {
66+ ensure_loaded_with_rtld_global_mesa_client();
67 MirPlatformPackage package;
68 context->populate_server_package(package);
69 if (package.data_items != 0 || package.fd_items != 1)
70
71=== modified file 'src/platforms/mesa/server/kms/platform_symbols.cpp'
72--- src/platforms/mesa/server/kms/platform_symbols.cpp 2015-10-02 14:35:09 +0000
73+++ src/platforms/mesa/server/kms/platform_symbols.cpp 2015-10-20 05:41:35 +0000
74@@ -99,17 +99,11 @@
75
76 // Hack around the way mesa loads mir: This hack makes the
77 // necessary symbols global.
78-extern "C" int __attribute__((constructor))
79-ensure_loaded_with_rtld_global()
80+void ensure_loaded_with_rtld_global()
81 {
82 Dl_info info;
83-
84- // Cast dladdr itself to work around g++-4.8 warnings (LP: #1366134)
85- typedef int (safe_dladdr_t)(int(*func)(), Dl_info *info);
86- safe_dladdr_t *safe_dladdr = (safe_dladdr_t*)&dladdr;
87- safe_dladdr(&ensure_loaded_with_rtld_global, &info);
88+ dladdr(reinterpret_cast<void*>(&ensure_loaded_with_rtld_global), &info);
89 dlopen(info.dli_fname, RTLD_NOW | RTLD_NOLOAD | RTLD_GLOBAL);
90- return 0;
91 }
92
93 }
94@@ -195,5 +189,6 @@
95 std::shared_ptr<mg::DisplayReport> const&,
96 std::shared_ptr<mg::NestedContext> const& nested_context)
97 {
98+ ensure_loaded_with_rtld_global();
99 return std::make_shared<mgm::GuestPlatform>(nested_context);
100 }

Subscribers

People subscribed via source and target branches