Mir

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

fix-1506137
Merge into development-branch

Proposed by Andreas Pokorny on 2015-10-16

Status:

Merged

Approved by:

Andreas Pokorny on 2015-10-20

Approved revision:

3034

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

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2015-10-20
Daniel van Vugt		2015-10-16	Abstain on 2015-10-20
Kevin DuBois (community)			Approve on 2015-10-19
Alan Griffiths			Approve on 2015-10-19
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.

PS Jenkins bot (ps-jenkins) wrote on 2015-10-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5300/rebuild

review: Needs Fixing (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2015-10-17:

[----------] 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

PS Jenkins bot (ps-jenkins) wrote on 2015-10-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5312/rebuild

review: Approve (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2015-10-17:

no segfault on krillin.. rerunning..

PS Jenkins bot (ps-jenkins) wrote on 2015-10-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5313/rebuild

review: Approve (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2015-10-17:

mako this time .. rerunning

PS Jenkins bot (ps-jenkins) wrote on 2015-10-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5314/rebuild

review: Needs Fixing (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2015-10-17:

Failure to add ppa.. Rerunning

PS Jenkins bot (ps-jenkins) wrote on 2015-10-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5315/rebuild

review: Needs Fixing (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2015-10-17:

now a mako setup failure..

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

PS Jenkins bot (ps-jenkins) wrote on 2015-10-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5316/rebuild

review: Approve (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-10-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5317/rebuild

review: Approve (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-10-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5318/rebuild

review: Approve (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2015-10-18:

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

Daniel van Vugt (vanvugt) wrote on 2015-10-19:

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...

Daniel van Vugt (vanvugt) wrote on 2015-10-19:

Sounds like the right idea.

review: Approve

Alan Griffiths (alan-griffiths) wrote on 2015-10-19:

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

Kevin DuBois (kdub) wrote on 2015-10-19:

alright

review: Approve

Andreas Pokorny (andreas-pokorny) wrote on 2015-10-19:

> 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..

Alan Griffiths (alan-griffiths) wrote on 2015-10-19:

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.

PS Jenkins bot (ps-jenkins) wrote on 2015-10-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5325/rebuild

review: Needs Fixing (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2015-10-19:

> 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..

PS Jenkins bot (ps-jenkins) wrote on 2015-10-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5329/rebuild

review: Approve (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-10-19:

review: Needs Fixing (continuous-integration)

Daniel van Vugt (vanvugt) wrote on 2015-10-20:

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

Daniel van Vugt (vanvugt) on 2015-10-20:

review: Abstain

lp:~andreas-pokorny/mir/fix-1506137 updated on 2015-10-20

3034. By Andreas Pokorny on 2015-10-20: just cast the fp

Andreas Pokorny (andreas-pokorny) wrote on 2015-10-20:

Autolanding failure due to problems installing the PPA.

PS Jenkins bot (ps-jenkins) on 2015-10-20:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Andreas Pokorny

Emanuele Antonio Faraone

Gerry Boland

Mir development team

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	}