Mir

Merge lp:~alan-griffiths/mir/fix-1615587 into lp:mir

  1. fix-1615587
  2. Merge into development-branch
Proposed by Alan Griffiths
Status: Merged
Merged at revision: 3668
Proposed branch: lp:~alan-griffiths/mir/fix-1615587
Merge into: lp:mir
Diff against target: 155 lines (+59/-1)
7 files modified
CMakeLists.txt (+1/-1)
include/platform/mir/graphics/platform.h (+10/-0)
include/platform/mir/input/platform.h (+11/-0)
src/include/client/mir/client_platform_factory.h (+9/-0)
tests/mir_test_framework/stub_input.cpp (+9/-0)
tests/mir_test_framework/stubbed_graphics_platform.cpp (+9/-0)
tests/unit-tests/library_example.h (+10/-0)
To merge this branch: bzr merge lp:~alan-griffiths/mir/fix-1615587
Reviewer Review Type Date Requested Status
Kevin DuBois (community) Approve
Mir CI Bot continuous-integration Needs Fixing
Daniel van Vugt Abstain
Chris Halse Rogers Approve
Review via email: mp+303539@code.launchpad.net

Commit message

We use "C" linkage to avoid name-mangling some functions that are not intended not for C compatibility. We don't want downstream projects seeing a warning from clang based tools for us doing this intentionally. (LP: 1615587)

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3661
https://mir-jenkins.ubuntu.com/job/mir-ci/1502/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1869/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1928
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1919
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1919
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1919
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1893/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1893
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1893/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1893
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1893/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1893/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1893/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1893
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1893/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1893
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1893/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1502/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Two (unrelated) CI failures: lp:1523621 and lp:1586382

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3661
https://mir-jenkins.ubuntu.com/job/mir-ci/1503/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1872/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1931
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1922
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1922
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1922
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1896
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1896/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1896/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1896
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1896/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1896/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1896/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1896
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1896/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1896
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1896/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1503/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Chris Halse Rogers (raof) wrote :

LGTM

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

Hmm, it is a bit weird actually. I can't remember now why we bother avoiding C++ mangling on code that is strictly not C-compatible anyway. Although I would prefer our driver model was plain C, since it's not, why are we giving it that appearance?

Symbol mangling follows strict rules. So we could just make them proper C++ functions and go searching for the proper mangled C++ symbol names. You only have to figure out what each mangled name is once, and code it once.

It feels like this makes a hack into a worse hack. But not that much worse, so abstain.

review: Abstain
Revision history for this message
Chris Halse Rogers (raof) wrote :

While name mangling follows strict rules, they're rules with
architecture specific cases, so if we tried to match on mangled name we
might need to change what symbol we look up depending on the
architecture we're running on.

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

We could just try and see if we're leaning on any architecture-specific cases. If so, then that's a no. But we might not have any such issues.

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

*If* we really don't like this (IMO perfectly reasonable) usage we could change the API/ABI design to be C compatible by adding a level of indirection. Vis:

struct GraphicsPlatformEntry
{
virtual mir::UniqueModulePtr<mir::graphics::Platform> create_host_platform(
    std::shared_ptr<mir::options::Option> const& options,
    std::shared_ptr<mir::EmergencyCleanupRegistry> const& emergency_cleanup_registry,
    std::shared_ptr<mir::graphics::DisplayReport> const& report) = 0;
};

// This is the published entry point. NB Ownership is not transferred.
extern "C" GraphicsPlatformEntry* graphics_platform_entry;

// client code does:
  if (void* p = dlvsym(so, "graphics_platform_entry", version))
  {
      return static_cast<GraphicsPlatformEntry*>(p)
        ->create_stub_platform(the_options(), the_emergency_cleanup(), the_display_report());
  }

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

I don't think that's helpful. Those functions still require C++.

Long term I'd like to see a cleaner driver architecture. Short term, let's land this and fix the bug...

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

One failure is lp:1563210 but the other...

Why CI why?

16:40:44 running sudo /usr/bin/mir_privileged_tests
16:40:44 [WS-CLEANUP] Deleting project workspace...Running main() from gtest_main.cc
16:40:45 [WS-CLEANUP] done
16:40:45 Finished: ABORTED

https://mir-jenkins.ubuntu.com/job/device-runtests-mir/device_type=krillin/1400/consoleFull

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

In any case, this MP can only break compilation and the failures are in running tests. So I'll TA and see what happens next.

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/533/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1891/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/569/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1951
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1942
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1942
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1942
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1915
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1915/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1915
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1915/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1915/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1915
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1915/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1915
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1915/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1915
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1915/artifact/output/*zip*/output.zip

review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

^^^
Seems to be the same failure for several branches today...

15:27:29 /usr/include/c++/6/bits/unique_ptr.h:787: error: undefined reference to '_Unwind_Resume'
15:27:29 /usr/include/x86_64-linux-gnu/c++/6/bits/gthr-default.h:778: error: undefined reference to '_Unwind_Resume'
15:27:29 ../../../src/server/glib_main_loop_sources.cpp:316: error: undefined reference to '_Unwind_Resume'
15:27:29 /usr/include/boost/exception/exception.hpp:326: error: undefined reference to '_Unwind_Resume'
15:27:29 collect2: error: ld returned 1 exit status

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

I see it was already top-approved, but fix is alright by me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2016-08-17 03:00:44 +0000
3+++ CMakeLists.txt 2016-08-22 11:07:29 +0000
4@@ -74,7 +74,7 @@
5 endif()
6
7 if ("${CMAKE_CXX_COMPILER}" MATCHES "clang")
8- set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-return-type-c-linkage -Wno-mismatched-tags")
9+ set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-mismatched-tags")
10 endif()
11
12 # Link time optimization allows leaner, cleaner libraries
13
14=== modified file 'include/platform/mir/graphics/platform.h'
15--- include/platform/mir/graphics/platform.h 2016-07-20 04:54:07 +0000
16+++ include/platform/mir/graphics/platform.h 2016-08-22 11:07:29 +0000
17@@ -132,6 +132,12 @@
18
19 extern "C"
20 {
21+#if defined(__clang__)
22+#pragma clang diagnostic push
23+// These functions are given "C" linkage to avoid name-mangling, not for C compatibility.
24+// (We don't want a warning for doing this intentionally.)
25+#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
26+#endif
27
28 /**
29 * Function prototype used to return a new host graphics platform. The host graphics platform
30@@ -183,6 +189,10 @@
31 mir::graphics::PlatformPriority probe_graphics_platform(mir::options::ProgramOption const& options);
32
33 mir::ModuleProperties const* describe_graphics_module();
34+
35+#if defined(__clang__)
36+#pragma clang diagnostic pop
37+#endif
38 }
39
40 #endif // MIR_GRAPHICS_PLATFORM_H_
41
42=== modified file 'include/platform/mir/input/platform.h'
43--- include/platform/mir/input/platform.h 2016-01-29 08:18:22 +0000
44+++ include/platform/mir/input/platform.h 2016-08-22 11:07:29 +0000
45@@ -109,6 +109,13 @@
46
47 extern "C"
48 {
49+#if defined(__clang__)
50+#pragma clang diagnostic push
51+// These functions are given "C" linkage to avoid name-mangling, not for C compatibility.
52+// (We don't want a warning for doing this intentionally.)
53+#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
54+#endif
55+
56 /**
57 * Function used to initialize an input platform.
58 *
59@@ -159,5 +166,9 @@
60 * \ingroup platform_enablement
61 */
62 mir::ModuleProperties const* describe_input_module();
63+
64+#if defined(__clang__)
65+#pragma clang diagnostic pop
66+#endif
67 }
68 #endif // MIR_INPUT_PLATFORM_H_
69
70=== modified file 'src/include/client/mir/client_platform_factory.h'
71--- src/include/client/mir/client_platform_factory.h 2016-01-29 08:18:22 +0000
72+++ src/include/client/mir/client_platform_factory.h 2016-08-22 11:07:29 +0000
73@@ -47,7 +47,16 @@
74 }
75 }
76
77+#if defined(__clang__)
78+#pragma clang diagnostic push
79+// These functions are given "C" linkage to avoid name-mangling, not for C compatibility.
80+// (We don't want a warning for doing this intentionally.)
81+#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
82+#endif
83 extern "C" mir::UniqueModulePtr<mir::client::ClientPlatform> create_client_platform(mir::client::ClientContext* context);
84 extern "C" bool is_appropriate_module(mir::client::ClientContext* context);
85+#if defined(__clang__)
86+#pragma clang diagnostic pop
87+#endif
88
89 #endif /* MIR_CLIENT_CLIENT_PLATFORM_FACTORY_H_ */
90
91=== modified file 'tests/mir_test_framework/stub_input.cpp'
92--- tests/mir_test_framework/stub_input.cpp 2016-01-29 08:18:22 +0000
93+++ tests/mir_test_framework/stub_input.cpp 2016-08-22 11:07:29 +0000
94@@ -68,7 +68,16 @@
95 return &description;
96 }
97
98+#if defined(__clang__)
99+#pragma clang diagnostic push
100+// These functions are given "C" linkage to avoid name-mangling, not for C compatibility.
101+// (We don't want a warning for doing this intentionally.)
102+#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
103+#endif
104 extern "C" mir::UniqueModulePtr<mtf::FakeInputDevice> add_fake_input_device(mi::InputDeviceInfo const& info)
105 {
106 return mir::make_module_ptr<mtf::FakeInputDeviceImpl>(info);
107 }
108+#if defined(__clang__)
109+#pragma clang diagnostic pop
110+#endif
111
112=== modified file 'tests/mir_test_framework/stubbed_graphics_platform.cpp'
113--- tests/mir_test_framework/stubbed_graphics_platform.cpp 2016-08-12 18:41:07 +0000
114+++ tests/mir_test_framework/stubbed_graphics_platform.cpp 2016-08-22 11:07:29 +0000
115@@ -287,10 +287,19 @@
116 std::unique_ptr<std::vector<geom::Rectangle>> chosen_display_rects;
117 }
118
119+#if defined(__clang__)
120+#pragma clang diagnostic push
121+// These functions are given "C" linkage to avoid name-mangling, not for C compatibility.
122+// (We don't want a warning for doing this intentionally.)
123+#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
124+#endif
125 extern "C" std::shared_ptr<mg::Platform> create_stub_platform(std::vector<geom::Rectangle> const& display_rects)
126 {
127 return std::make_shared<mtf::StubGraphicPlatform>(display_rects);
128 }
129+#if defined(__clang__)
130+#pragma clang diagnostic pop
131+#endif
132
133 mir::UniqueModulePtr<mg::Platform> create_host_platform(
134 std::shared_ptr<mo::Option> const& /*options*/,
135
136=== modified file 'tests/unit-tests/library_example.h'
137--- tests/unit-tests/library_example.h 2015-02-20 08:30:20 +0000
138+++ tests/unit-tests/library_example.h 2016-08-22 11:07:29 +0000
139@@ -33,6 +33,16 @@
140 };
141
142 using ModuleFunction = mir::UniqueModulePtr<SomeInterface> (*)();
143+
144+#if defined(__clang__)
145+#pragma clang diagnostic push
146+// These functions are given "C" linkage to avoid name-mangling, not for C compatibility.
147+// (We don't want a warning for doing this intentionally.)
148+#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
149+#endif
150 extern "C" mir::UniqueModulePtr<SomeInterface> module_create_instance();
151+#if defined(__clang__)
152+#pragma clang diagnostic pop
153+#endif
154
155 #endif

Subscribers

People subscribed via source and target branches