Mir

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

Proposed by Alan Griffiths
Status: Merged
Approved by: Daniel van Vugt
Approved revision: 3497
Merged at revision: 3501
Proposed branch: lp:~alan-griffiths/mir/fix-1577106
Merge into: lp:mir
Diff against target: 175 lines (+20/-13)
10 files modified
src/common/fatal/fatal.cpp (+1/-1)
src/include/platform/mir/options/configuration.h (+1/-1)
src/include/server/mir/default_server_configuration.h (+2/-2)
src/platform/options/default_configuration.cpp (+3/-3)
src/platform/symbols.map (+1/-1)
src/server/default_server_configuration.cpp (+2/-2)
tests/acceptance-tests/test_server_shutdown.cpp (+2/-3)
tests/unit-tests/graphics/mesa/kms/test_display.cpp (+2/-0)
tests/unit-tests/graphics/mesa/kms/test_real_kms_output.cpp (+4/-0)
tests/unit-tests/test_fatal.cpp (+2/-0)
To merge this branch: bzr merge lp:~alan-griffiths/mir/fix-1577106
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve
Cemil Azizoglu (community) Approve
Daniel van Vugt Approve
Alan Griffiths Needs Information
Review via email: mp+293876@code.launchpad.net

Commit message

platform: change the default behaviour of fatal_error() to abort()

Description of the change

platform: change the default behaviour of fatal_error() to abort()

Note: I realize this is a platform ABI break and that changes are in the wrong stanza - if we land this I'll fix that asap.

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

[Voting on my own MP to avoid this landing without adequate discussion]

None of our downstreams have seen fit to configure fatal_error() to abort() (which is quite easy). Maybe we should respect that decision?

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

PASSED: Continuous integration, rev:3497
https://mir-jenkins.ubuntu.com/job/mir-ci/947/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1010
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1056
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1047
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1047
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1020
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1020/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1020
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1020/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1020
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1020/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/1020
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1020/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1020
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1020/artifact/output/*zip*/output.zip

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

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

I like it, but unrecoverable exceptions need to dump core too - per bug 1577106. Those are from 'throw' and not from 'fatal_error'. So if this lands, I don't think that's enough to say bug 1577106 is fixed. The bug might need to stay open.

Or did I miss something? Does this change also ensure unrecoverable exceptions leave core files too?

review: Needs Information
Revision history for this message
Daniel van Vugt (vanvugt) :
review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> I like it, but unrecoverable exceptions need to dump core too - per bug
> 1577106. Those are from 'throw' and not from 'fatal_error'. So if this lands,
> I don't think that's enough to say bug 1577106 is fixed. The bug might need to
> stay open.
>
> Or did I miss something? Does this change also ensure unrecoverable exceptions
> leave core files too?

You didn't miss something. We do have some code throwing exceptions that are not recovered from. But that code (or the code that propagates the error) is wrong. We need to look at cases to decide where we can decide the error is fatal. (And we may need to extend fatal_error(...) to collect more information.)

So this is only part of the fix.

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

> Alexandros pointed out that USC is being run with "--on-fatal-error-abort"
>
> https://bazaar.launchpad.net/~unity-system-compositor-team/unity-system-
> compositor/trunk/view/head:/debian/unity-system-compositor.sleep

He then pointed out that this is obsolete. USC is started without "--on-fatal-error-abort".

Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

ok

review: Approve
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/254/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1024/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/274/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1070
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1061
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1061
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1034/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1034
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1034/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1034
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1034/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/1034
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1034/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1034
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1034/artifact/output/*zip*/output.zip

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

That's bug 1576690. Try again.

Revision history for this message
Mir CI Bot (mir-ci-bot) :
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/common/fatal/fatal.cpp'
2--- src/common/fatal/fatal.cpp 2015-06-18 14:33:10 +0000
3+++ src/common/fatal/fatal.cpp 2016-05-05 11:04:12 +0000
4@@ -55,4 +55,4 @@
5 BOOST_THROW_EXCEPTION(std::runtime_error(buffer));
6 }
7
8-void (*mir::fatal_error)(char const* reason, ...){&mir::fatal_error_except};
9+void (*mir::fatal_error)(char const* reason, ...){&mir::fatal_error_abort};
10
11=== modified file 'src/include/platform/mir/options/configuration.h'
12--- src/include/platform/mir/options/configuration.h 2016-01-29 08:18:22 +0000
13+++ src/include/platform/mir/options/configuration.h 2016-05-05 11:04:12 +0000
14@@ -45,7 +45,7 @@
15 extern char const* const host_socket_opt;
16 extern char const* const frontend_threads_opt;
17 extern char const* const touchspots_opt;
18-extern char const* const fatal_abort_opt;
19+extern char const* const fatal_except_opt;
20 extern char const* const debug_opt;
21 extern char const* const nbuffers_opt;
22 extern char const* const composite_delay_opt;
23
24=== modified file 'src/include/server/mir/default_server_configuration.h'
25--- src/include/server/mir/default_server_configuration.h 2016-03-29 07:30:50 +0000
26+++ src/include/server/mir/default_server_configuration.h 2016-05-05 11:04:12 +0000
27@@ -178,8 +178,8 @@
28 std::shared_ptr<cookie::Authority> the_cookie_authority() override;
29 /**
30 * Function to call when a "fatal" error occurs. This implementation allows
31- * the default strategy to be overridden by --on-fatal-error-abort to force a
32- * core. (This behavior is useful for diagnostic purposes during development.)
33+ * the default strategy to be overridden by --on-fatal-error-except to avoid a
34+ * core.
35 * To change the default strategy used FatalErrorStrategy. See acceptance test
36 * ServerShutdown.fatal_error_default_can_be_changed_to_abort
37 * for an example.
38
39=== modified file 'src/platform/options/default_configuration.cpp'
40--- src/platform/options/default_configuration.cpp 2016-05-03 04:36:33 +0000
41+++ src/platform/options/default_configuration.cpp 2016-05-05 11:04:12 +0000
42@@ -47,7 +47,7 @@
43 char const* const mo::name_opt = "name";
44 char const* const mo::offscreen_opt = "offscreen";
45 char const* const mo::touchspots_opt = "enable-touchspots";
46-char const* const mo::fatal_abort_opt = "on-fatal-error-abort";
47+char const* const mo::fatal_except_opt = "on-fatal-error-except";
48 char const* const mo::debug_opt = "debug";
49 char const* const mo::nbuffers_opt = "nbuffers";
50 char const* const mo::composite_delay_opt = "composite-delay";
51@@ -187,8 +187,8 @@
52 "Display visualization of touchspots (e.g. for screencasting).")
53 (enable_key_repeat_opt, po::value<bool>()->default_value(true),
54 "Enable server generated key repeat")
55- (fatal_abort_opt, "On \"fatal error\" conditions [e.g. drivers behaving "
56- "in unexpected ways] abort (to get a core dump)")
57+ (fatal_except_opt, "On \"fatal error\" conditions [e.g. drivers behaving "
58+ "in unexpected ways] throw an exception (instead of a core dump)")
59 (debug_opt, "Enable extra development debugging. "
60 "This is only interesting for people doing Mir server or client development.");
61
62
63=== modified file 'src/platform/symbols.map'
64--- src/platform/symbols.map 2016-03-29 07:30:50 +0000
65+++ src/platform/symbols.map 2016-05-05 11:04:12 +0000
66@@ -118,7 +118,7 @@
67 mir::options::nbuffers_opt*;
68 mir::options::enable_input_opt*;
69 mir::options::enable_key_repeat_opt*;
70- mir::options::fatal_abort_opt*;
71+ mir::options::fatal_except_opt*;
72 mir::options::frontend_threads_opt*;
73 mir::options::glog*;
74 mir::options::glog_log_dir*;
75
76=== modified file 'src/server/default_server_configuration.cpp'
77--- src/server/default_server_configuration.cpp 2016-03-29 07:30:50 +0000
78+++ src/server/default_server_configuration.cpp 2016-05-05 11:04:12 +0000
79@@ -196,8 +196,8 @@
80 auto mir::DefaultServerConfiguration::the_fatal_error_strategy()
81 -> void (*)(char const* reason, ...)
82 {
83- if (the_options()->is_set(options::fatal_abort_opt))
84- return &fatal_error_abort;
85+ if (the_options()->is_set(options::fatal_except_opt))
86+ return &fatal_error_except;
87 else
88 return fatal_error;
89 }
90
91=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
92--- tests/acceptance-tests/test_server_shutdown.cpp 2016-05-03 04:36:33 +0000
93+++ tests/acceptance-tests/test_server_shutdown.cpp 2016-05-05 11:04:12 +0000
94@@ -163,10 +163,8 @@
95 };
96 }
97
98-TEST_F(ServerShutdownDeathTest, on_fatal_error_abort_option_causes_abort_on_fatal_error)
99+TEST_F(ServerShutdownDeathTest, abort_on_fatal_error)
100 {
101- add_to_environment( "MIR_SERVER_ON_FATAL_ERROR_ABORT", "");
102-
103 mt::CrossProcessSync sync;
104
105 run_in_server_and_disable_core_dump([&]
106@@ -191,6 +189,7 @@
107 TEST_F(ServerShutdownDeathTest, mir_fatal_error_during_init_removes_endpoint)
108 { // Even fatal errors sometimes need to be caught for critical cleanup...
109
110+ add_to_environment( "MIR_SERVER_ON_FATAL_ERROR_EXCEPT", "");
111 add_to_environment("MIR_SERVER_FILE", mir_test_socket);
112 server.add_init_callback([&] { mir::fatal_error("Bang"); });
113 server.apply_settings();
114
115=== modified file 'tests/unit-tests/graphics/mesa/kms/test_display.cpp'
116--- tests/unit-tests/graphics/mesa/kms/test_display.cpp 2016-01-29 08:18:22 +0000
117+++ tests/unit-tests/graphics/mesa/kms/test_display.cpp 2016-05-05 11:04:12 +0000
118@@ -26,6 +26,7 @@
119 #include "mir/renderer/gl/render_target.h"
120 #include "mir/time/steady_clock.h"
121 #include "mir/glib_main_loop.h"
122+#include "mir/fatal.h"
123
124 #include "mir/test/doubles/mock_egl.h"
125 #include "mir/test/doubles/mock_gl.h"
126@@ -446,6 +447,7 @@
127
128 TEST_F(MesaDisplayTest, post_update_flip_failure)
129 {
130+ mir::FatalErrorStrategy on_error{mir::fatal_error_except};
131 using namespace testing;
132
133 auto const crtc_id = get_connected_crtc_id();
134
135=== modified file 'tests/unit-tests/graphics/mesa/kms/test_real_kms_output.cpp'
136--- tests/unit-tests/graphics/mesa/kms/test_real_kms_output.cpp 2016-04-08 08:46:30 +0000
137+++ tests/unit-tests/graphics/mesa/kms/test_real_kms_output.cpp 2016-05-05 11:04:12 +0000
138@@ -18,6 +18,7 @@
139
140 #include "src/platforms/mesa/server/kms/real_kms_output.h"
141 #include "src/platforms/mesa/server/kms/page_flipper.h"
142+#include "mir/fatal.h"
143
144 #include "mir/test/fake_shared.h"
145
146@@ -200,6 +201,7 @@
147
148 TEST_F(RealKMSOutputTest, set_crtc_failure_is_handled_gracefully)
149 {
150+ mir::FatalErrorStrategy on_error{mir::fatal_error_except};
151 using namespace testing;
152
153 uint32_t const fb_id{67};
154@@ -278,6 +280,8 @@
155
156 TEST_F(RealKMSOutputTest, clear_crtc_throws_if_drm_call_fails)
157 {
158+ mir::FatalErrorStrategy on_error{mir::fatal_error_except};
159+
160 using namespace testing;
161
162 setup_outputs_connected_crtc();
163
164=== modified file 'tests/unit-tests/test_fatal.cpp'
165--- tests/unit-tests/test_fatal.cpp 2016-03-29 07:30:50 +0000
166+++ tests/unit-tests/test_fatal.cpp 2016-05-05 11:04:12 +0000
167@@ -45,6 +45,8 @@
168
169 TEST(FatalErrorTest, throw_formats_message_to_what)
170 {
171+ mir::FatalErrorStrategy on_error{mir::fatal_error_except};
172+
173 EXPECT_THROW(
174 mir::fatal_error("%s had %d %s %s", "Mary", 1, "little", "lamb"),
175 std::runtime_error);

Subscribers

People subscribed via source and target branches