Mir

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

  1. fix-1577106
  2. Merge into development-branch
Proposed by Alan Griffiths
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
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 :

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

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
=== modified file 'src/common/fatal/fatal.cpp'
--- src/common/fatal/fatal.cpp 2015-06-18 14:33:10 +0000
+++ src/common/fatal/fatal.cpp 2016-05-05 11:04:12 +0000
@@ -55,4 +55,4 @@
55 BOOST_THROW_EXCEPTION(std::runtime_error(buffer));55 BOOST_THROW_EXCEPTION(std::runtime_error(buffer));
56}56}
5757
58void (*mir::fatal_error)(char const* reason, ...){&mir::fatal_error_except};58void (*mir::fatal_error)(char const* reason, ...){&mir::fatal_error_abort};
5959
=== modified file 'src/include/platform/mir/options/configuration.h'
--- src/include/platform/mir/options/configuration.h 2016-01-29 08:18:22 +0000
+++ src/include/platform/mir/options/configuration.h 2016-05-05 11:04:12 +0000
@@ -45,7 +45,7 @@
45extern char const* const host_socket_opt;45extern char const* const host_socket_opt;
46extern char const* const frontend_threads_opt;46extern char const* const frontend_threads_opt;
47extern char const* const touchspots_opt;47extern char const* const touchspots_opt;
48extern char const* const fatal_abort_opt;48extern char const* const fatal_except_opt;
49extern char const* const debug_opt;49extern char const* const debug_opt;
50extern char const* const nbuffers_opt;50extern char const* const nbuffers_opt;
51extern char const* const composite_delay_opt;51extern char const* const composite_delay_opt;
5252
=== modified file 'src/include/server/mir/default_server_configuration.h'
--- src/include/server/mir/default_server_configuration.h 2016-03-29 07:30:50 +0000
+++ src/include/server/mir/default_server_configuration.h 2016-05-05 11:04:12 +0000
@@ -178,8 +178,8 @@
178 std::shared_ptr<cookie::Authority> the_cookie_authority() override;178 std::shared_ptr<cookie::Authority> the_cookie_authority() override;
179 /**179 /**
180 * Function to call when a "fatal" error occurs. This implementation allows180 * Function to call when a "fatal" error occurs. This implementation allows
181 * the default strategy to be overridden by --on-fatal-error-abort to force a181 * the default strategy to be overridden by --on-fatal-error-except to avoid a
182 * core. (This behavior is useful for diagnostic purposes during development.)182 * core.
183 * To change the default strategy used FatalErrorStrategy. See acceptance test183 * To change the default strategy used FatalErrorStrategy. See acceptance test
184 * ServerShutdown.fatal_error_default_can_be_changed_to_abort184 * ServerShutdown.fatal_error_default_can_be_changed_to_abort
185 * for an example.185 * for an example.
186186
=== modified file 'src/platform/options/default_configuration.cpp'
--- src/platform/options/default_configuration.cpp 2016-05-03 04:36:33 +0000
+++ src/platform/options/default_configuration.cpp 2016-05-05 11:04:12 +0000
@@ -47,7 +47,7 @@
47char const* const mo::name_opt = "name";47char const* const mo::name_opt = "name";
48char const* const mo::offscreen_opt = "offscreen";48char const* const mo::offscreen_opt = "offscreen";
49char const* const mo::touchspots_opt = "enable-touchspots";49char const* const mo::touchspots_opt = "enable-touchspots";
50char const* const mo::fatal_abort_opt = "on-fatal-error-abort";50char const* const mo::fatal_except_opt = "on-fatal-error-except";
51char const* const mo::debug_opt = "debug";51char const* const mo::debug_opt = "debug";
52char const* const mo::nbuffers_opt = "nbuffers";52char const* const mo::nbuffers_opt = "nbuffers";
53char const* const mo::composite_delay_opt = "composite-delay";53char const* const mo::composite_delay_opt = "composite-delay";
@@ -187,8 +187,8 @@
187 "Display visualization of touchspots (e.g. for screencasting).")187 "Display visualization of touchspots (e.g. for screencasting).")
188 (enable_key_repeat_opt, po::value<bool>()->default_value(true),188 (enable_key_repeat_opt, po::value<bool>()->default_value(true),
189 "Enable server generated key repeat")189 "Enable server generated key repeat")
190 (fatal_abort_opt, "On \"fatal error\" conditions [e.g. drivers behaving "190 (fatal_except_opt, "On \"fatal error\" conditions [e.g. drivers behaving "
191 "in unexpected ways] abort (to get a core dump)")191 "in unexpected ways] throw an exception (instead of a core dump)")
192 (debug_opt, "Enable extra development debugging. "192 (debug_opt, "Enable extra development debugging. "
193 "This is only interesting for people doing Mir server or client development.");193 "This is only interesting for people doing Mir server or client development.");
194194
195195
=== modified file 'src/platform/symbols.map'
--- src/platform/symbols.map 2016-03-29 07:30:50 +0000
+++ src/platform/symbols.map 2016-05-05 11:04:12 +0000
@@ -118,7 +118,7 @@
118 mir::options::nbuffers_opt*;118 mir::options::nbuffers_opt*;
119 mir::options::enable_input_opt*;119 mir::options::enable_input_opt*;
120 mir::options::enable_key_repeat_opt*;120 mir::options::enable_key_repeat_opt*;
121 mir::options::fatal_abort_opt*;121 mir::options::fatal_except_opt*;
122 mir::options::frontend_threads_opt*;122 mir::options::frontend_threads_opt*;
123 mir::options::glog*;123 mir::options::glog*;
124 mir::options::glog_log_dir*;124 mir::options::glog_log_dir*;
125125
=== modified file 'src/server/default_server_configuration.cpp'
--- src/server/default_server_configuration.cpp 2016-03-29 07:30:50 +0000
+++ src/server/default_server_configuration.cpp 2016-05-05 11:04:12 +0000
@@ -196,8 +196,8 @@
196auto mir::DefaultServerConfiguration::the_fatal_error_strategy()196auto mir::DefaultServerConfiguration::the_fatal_error_strategy()
197-> void (*)(char const* reason, ...)197-> void (*)(char const* reason, ...)
198{198{
199 if (the_options()->is_set(options::fatal_abort_opt))199 if (the_options()->is_set(options::fatal_except_opt))
200 return &fatal_error_abort;200 return &fatal_error_except;
201 else201 else
202 return fatal_error;202 return fatal_error;
203}203}
204204
=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
--- tests/acceptance-tests/test_server_shutdown.cpp 2016-05-03 04:36:33 +0000
+++ tests/acceptance-tests/test_server_shutdown.cpp 2016-05-05 11:04:12 +0000
@@ -163,10 +163,8 @@
163 };163 };
164}164}
165165
166TEST_F(ServerShutdownDeathTest, on_fatal_error_abort_option_causes_abort_on_fatal_error)166TEST_F(ServerShutdownDeathTest, abort_on_fatal_error)
167{167{
168 add_to_environment( "MIR_SERVER_ON_FATAL_ERROR_ABORT", "");
169
170 mt::CrossProcessSync sync;168 mt::CrossProcessSync sync;
171169
172 run_in_server_and_disable_core_dump([&]170 run_in_server_and_disable_core_dump([&]
@@ -191,6 +189,7 @@
191TEST_F(ServerShutdownDeathTest, mir_fatal_error_during_init_removes_endpoint)189TEST_F(ServerShutdownDeathTest, mir_fatal_error_during_init_removes_endpoint)
192{ // Even fatal errors sometimes need to be caught for critical cleanup...190{ // Even fatal errors sometimes need to be caught for critical cleanup...
193191
192 add_to_environment( "MIR_SERVER_ON_FATAL_ERROR_EXCEPT", "");
194 add_to_environment("MIR_SERVER_FILE", mir_test_socket);193 add_to_environment("MIR_SERVER_FILE", mir_test_socket);
195 server.add_init_callback([&] { mir::fatal_error("Bang"); });194 server.add_init_callback([&] { mir::fatal_error("Bang"); });
196 server.apply_settings();195 server.apply_settings();
197196
=== modified file 'tests/unit-tests/graphics/mesa/kms/test_display.cpp'
--- tests/unit-tests/graphics/mesa/kms/test_display.cpp 2016-01-29 08:18:22 +0000
+++ tests/unit-tests/graphics/mesa/kms/test_display.cpp 2016-05-05 11:04:12 +0000
@@ -26,6 +26,7 @@
26#include "mir/renderer/gl/render_target.h"26#include "mir/renderer/gl/render_target.h"
27#include "mir/time/steady_clock.h"27#include "mir/time/steady_clock.h"
28#include "mir/glib_main_loop.h"28#include "mir/glib_main_loop.h"
29#include "mir/fatal.h"
2930
30#include "mir/test/doubles/mock_egl.h"31#include "mir/test/doubles/mock_egl.h"
31#include "mir/test/doubles/mock_gl.h"32#include "mir/test/doubles/mock_gl.h"
@@ -446,6 +447,7 @@
446447
447TEST_F(MesaDisplayTest, post_update_flip_failure)448TEST_F(MesaDisplayTest, post_update_flip_failure)
448{449{
450 mir::FatalErrorStrategy on_error{mir::fatal_error_except};
449 using namespace testing;451 using namespace testing;
450452
451 auto const crtc_id = get_connected_crtc_id();453 auto const crtc_id = get_connected_crtc_id();
452454
=== modified file 'tests/unit-tests/graphics/mesa/kms/test_real_kms_output.cpp'
--- tests/unit-tests/graphics/mesa/kms/test_real_kms_output.cpp 2016-04-08 08:46:30 +0000
+++ tests/unit-tests/graphics/mesa/kms/test_real_kms_output.cpp 2016-05-05 11:04:12 +0000
@@ -18,6 +18,7 @@
1818
19#include "src/platforms/mesa/server/kms/real_kms_output.h"19#include "src/platforms/mesa/server/kms/real_kms_output.h"
20#include "src/platforms/mesa/server/kms/page_flipper.h"20#include "src/platforms/mesa/server/kms/page_flipper.h"
21#include "mir/fatal.h"
2122
22#include "mir/test/fake_shared.h"23#include "mir/test/fake_shared.h"
2324
@@ -200,6 +201,7 @@
200201
201TEST_F(RealKMSOutputTest, set_crtc_failure_is_handled_gracefully)202TEST_F(RealKMSOutputTest, set_crtc_failure_is_handled_gracefully)
202{203{
204 mir::FatalErrorStrategy on_error{mir::fatal_error_except};
203 using namespace testing;205 using namespace testing;
204206
205 uint32_t const fb_id{67};207 uint32_t const fb_id{67};
@@ -278,6 +280,8 @@
278280
279TEST_F(RealKMSOutputTest, clear_crtc_throws_if_drm_call_fails)281TEST_F(RealKMSOutputTest, clear_crtc_throws_if_drm_call_fails)
280{282{
283 mir::FatalErrorStrategy on_error{mir::fatal_error_except};
284
281 using namespace testing;285 using namespace testing;
282286
283 setup_outputs_connected_crtc();287 setup_outputs_connected_crtc();
284288
=== modified file 'tests/unit-tests/test_fatal.cpp'
--- tests/unit-tests/test_fatal.cpp 2016-03-29 07:30:50 +0000
+++ tests/unit-tests/test_fatal.cpp 2016-05-05 11:04:12 +0000
@@ -45,6 +45,8 @@
4545
46TEST(FatalErrorTest, throw_formats_message_to_what)46TEST(FatalErrorTest, throw_formats_message_to_what)
47{47{
48 mir::FatalErrorStrategy on_error{mir::fatal_error_except};
49
48 EXPECT_THROW(50 EXPECT_THROW(
49 mir::fatal_error("%s had %d %s %s", "Mary", 1, "little", "lamb"),51 mir::fatal_error("%s had %d %s %s", "Mary", 1, "little", "lamb"),
50 std::runtime_error);52 std::runtime_error);

Subscribers

People subscribed via source and target branches