Mir

Merge lp:~alan-griffiths/mir/fatal-error into lp:mir

fatal-error
Merge into development-branch

Proposed by Alan Griffiths on 2014-07-31

Status:

Merged

Approved by:

kevin gunn on 2014-08-05

Approved revision:

no longer in the source branch.

Merged at revision:

1822

Proposed branch:

lp:~alan-griffiths/mir/fatal-error

Merge into:

lp:mir

Diff against target:

527 lines (+292/-39)

10 files modified

include/platform/mir/fatal.h (+76/-0)
src/platform/CMakeLists.txt (+2/-0)
src/platform/fatal/CMakeLists.txt (+3/-0)
src/platform/fatal/fatal.cpp (+58/-0)
src/platform/graphics/mesa/CMakeLists.txt (+1/-2)
src/platform/graphics/mesa/display_buffer.cpp (+9/-8)
src/platform/graphics/mesa/real_kms_output.cpp (+18/-29)
tests/acceptance-tests/test_server_shutdown.cpp (+65/-0)
tests/unit-tests/CMakeLists.txt (+1/-0)
tests/unit-tests/test_fatal.cpp (+59/-0)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/fatal-error

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-08-05
Daniel van Vugt			Disapprove on 2014-08-05
Robert Carr (community)			Approve on 2014-08-04
Cemil Azizoglu (community)			Approve on 2014-08-04
Kevin DuBois (community)			Approve on 2014-08-04
Alexandros Frantzis (community)		2014-07-31	Approve on 2014-08-01
Review via email: mp+229086@code.launchpad.net

Commit message

platform: provide support for customizing Mir's behavior when a fatal_error occurs.

Description of the change

platform: provide support for customizing Mir's behavior when a fatal_error occurs.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-01:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2287/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-08-01:

Looks good.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-08-04:

(1) Author info for a few files is incorrect:
Author: Alan Griffiths <email address hidden>
Actually it was mostly copied from my branch, so please document as appropriate.

(2) The default action must be abort, not exceptions:
67 +void (*mir::fatal_error)(char const* reason, ...){&mir::fatal_error_except};

This is so that we can declare bug LP: #1316867 fixed and more importantly so that shell developers don't (by inaction) accidentally regress on it, preventing the creation of usable bug-reports. Catchable exceptions on unrecoverable errors must be an option at most, and never the default. Also keep in mind that we have upstart monitoring unity8 so it will restart cleanly either way. But only abort() gives you the extra bonus of usable bug-reports.

If abort() is not the default and not going to be always-on in usc and unity8 then there's no point in the proposal at all. It only gives you clean bug reports in debug builds and not in production (?!).

The good news is I have an alternative exceptions approach in mind that should make everyone happy. I'll prototype it when more important tasks are out the way.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-08-04:

P.S. The new approach I have in mind is to remove all the blanket catch() statements and ensure we only catch things in Mir that we have logic to recover from. Everything else that is unrecoverable should pass straight through as an uncaught exception (with cleaner stack and usable bug reports in theory). This idea will work so long as the shell itself is not insane enough to catch exceptions it doesn't know how to handle properly.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-08-04:

2) For all the reasons stated at great length on your previous proposal abort should *not* be the default action and must be /explicitly requested/ by users of the library.

1) As you wrote less than half the code in those two files; and you disagree with the rest of the team on on (2) I assumed you'd prefer not to be credited. But I'm happy to change them.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-04:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2299/rebuild

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-08-04:

Seems like a reasonable compromise to me, if the user wants us to behave badly for the purpose of maximizing debug info, they can set us up to abort, otherwise, we'd use the exception mechanism.

review: Approve

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-08-04:

We can set up u-s-c and u8 to abort but as discussed previously mir should throw an exception by default.

review: Approve

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-08-04:

LGTM. Aborting in libraries can be a pain for users (see: libdbus).

Not convinced all actions are really fatal errors, i.e. failed to schedule page flip, it's not recoverable, but why could it not be intermittent?

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-08-05:

(1) OK, yeah I don't approve of this new form so don't want to be listed as sole author. Although it's slightly inconsiderate considering the amount of code copied from my branch to list yourself first.

(2) Alright, you like exceptions and I like guaranteed post-mortem analysis. Let us agree to disagree.

So now I have an additional question: What does this branch solve or fix? Certainly not the bug I set out to fix in: https://code.launchpad.net/~vanvugt/mir/fatal-error/+merge/219471
Unless you explicitly turn "abort" on in downstream projects...

If abort isn't turned on by default then following simple logic, either:
(a) USC will want to turn it on always. So there's no reason to not make it the default in Mir for safety's sake, and go with my original proposal; or
(b) USC will not want to turn it on. This leads to no bugs being fixed. No improvement on the current situation. So why does this proposal exist at all?

I'm not trying to be negative. The reasoning doesn't seem add up. Is it (a) or (b), or am I foolish and you all know about an option (c) I'm not seeing?

review: Disapprove

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-08-05:

An in case anyone thinks (c) is to turn abort on after a crash and retry, that's just silly. You can't go back and ask users to reproduce a bug they generally can't reproduce on demand. I know this from many years of joining projects where developers believed that and it was simply proven to be ineffective in every case. You have to catch crashes with a clean bug-report the first time. Because there typically either won't be a second time for any given user, or it will be so long in the future that you've left the bug in the wild for too long and they've given up on your product completely.

Serious bugs need to generate clean crash reports the first time with no special configuration or preparation by the user.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-08-05:

racarr: Absolutely everything should be recoverable (like failure to schedule a page flip). We just lack such logic at the moment. The "fatal-error" approach is a temporary solution until smarter recovery logic exists in the failing module(s). And an important one because it gives us vital feedback in the form of clean bug reports showing us how such an apparently impossible situation occurred.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-08-05:

> (1) OK, yeah I don't approve of this new form so don't want to be listed as
> sole author. Although it's slightly inconsiderate considering the amount of
> code copied from my branch to list yourself first.

Fixed.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2318/rebuild

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:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === added file 'include/platform/mir/fatal.h'
 --- include/platform/mir/fatal.h	1970-01-01 00:00:00 +0000
 +++ include/platform/mir/fatal.h	2014-08-05 11:38:58 +0000
@@ -0,0 +1,76 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify
++ * it under the terms of the GNU Lesser General Public License version 3 as
++ * published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * ---
++ * Fatal error handling - Fatal errors are situations we don't expect to ever
++ * happen and don't have logic to gracefully recover from. The most useful
++ * thing you can do in that situation is abort to get a clean core file and
++ * stack trace to maximize the chances of it being readable.
++ *
++ * Author: Daniel van Vugt <daniel.van.vugt@canonical.com>
++ *         Alan Griffiths <alan@octopull.co.uk>
++ */
++
++#ifndef MIR_FATAL_H_
++#define MIR_FATAL_H_
++
++namespace mir
++{
++/**
++ * fatal_error() is strictly for "this should never happen" situations that
++ * you cannot recover from. By default it points at fatal_error_except().
++ * Note the reason parameter is a simple char* so its value is clearly visible
++ * in stack trace output.
++ *   \param [in] reason  A printf-style format string.
++ */
++extern void (*fatal_error)(char const* reason, ...);
++
++/**
++ * Throws an exception that will typically kill the Mir server and propagate from
++ * mir::run_mir.
++ *   \param [in] reason  A printf-style format string.
++ */
++void fatal_error_except(char const* reason, ...);
++
++/**
++ * An alternative to fatal_error_except() that kills the program and dump core
++ * as cleanly as possible.
++ *   \param [in] reason  A printf-style format string.
++ */
++void fatal_error_abort(char const* reason, ...);
++
++// Utility class to override & restore existing error handler
++class FatalErrorStrategy
++{
++public:
++    explicit FatalErrorStrategy(void (*fatal_error_handler)(char const* reason, ...)) :
++        old_fatal_error_handler(fatal_error)
++    {
++        fatal_error = fatal_error_handler;
++    }
++
++    ~FatalErrorStrategy()
++    {
++        fatal_error = old_fatal_error_handler;
++    }
++
++private:
++    void (*old_fatal_error_handler)(char const* reason, ...);
++    FatalErrorStrategy(FatalErrorStrategy const&) = delete;
++    FatalErrorStrategy& operator=(FatalErrorStrategy const&) = delete;
++};
++} // namespace mir
++
++#endif // MIR_FATAL_H_
 === modified file 'src/platform/CMakeLists.txt'
 --- src/platform/CMakeLists.txt	2014-07-24 13:44:09 +0000
 +++ src/platform/CMakeLists.txt	2014-08-05 11:38:58 +0000
@@ -5,6 +5,7 @@
    shared_library_loader.cpp
    $<TARGET_OBJECTS:mirplatformgraphicscommon>
    $<TARGET_OBJECTS:miroptions>
++  $<TARGET_OBJECTS:mirfatal>
+ )
  target_link_libraries(mirplatform
@@ -17,3 +18,4 @@
  add_subdirectory(graphics/)
  add_subdirectory(options)
++add_subdirectory(fatal)
 === added directory 'src/platform/fatal'
 === added file 'src/platform/fatal/CMakeLists.txt'
 --- src/platform/fatal/CMakeLists.txt	1970-01-01 00:00:00 +0000
 +++ src/platform/fatal/CMakeLists.txt	2014-08-05 11:38:58 +0000
@@ -0,0 +1,3 @@
++add_library(mirfatal OBJECT
++  fatal.cpp
++)
 === added file 'src/platform/fatal/fatal.cpp'
 --- src/platform/fatal/fatal.cpp	1970-01-01 00:00:00 +0000
 +++ src/platform/fatal/fatal.cpp	2014-08-05 11:38:58 +0000
@@ -0,0 +1,58 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU Lesser General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Author: Daniel van Vugt <daniel.van.vugt@canonical.com>
++ *         Alan Griffiths <alan@octopull.co.uk>
++ */
++
++#include "mir/fatal.h"
++
++#include <boost/throw_exception.hpp>
++#include <boost/exception/info.hpp>
++
++#include <stdexcept>
++#include <cstdlib>
++#include <cstdio>
++#include <cstdarg>
++
++void mir::fatal_error_abort(char const* reason, ...)
++{
++    va_list args;
++
++    // Keep this as simple as possible, avoiding any object construction and
++    // minimizing the potential for heap operations between the error location
++    // and the abort().
++    va_start(args, reason);
++    fprintf(stderr, "Mir fatal error: ");
++    vfprintf(stderr, reason, args);
++    fprintf(stderr, "\n");
++    va_end(args);
++
++    std::abort();
++}
++
++void mir::fatal_error_except(char const* reason, ...)
++{
++    char buffer[1024];
++    va_list args;
++
++    va_start(args, reason);
++    vsnprintf(buffer, sizeof buffer, reason, args);
++    va_end(args);
++
++    BOOST_THROW_EXCEPTION(std::runtime_error(buffer));
++}
++
++void (*mir::fatal_error)(char const* reason, ...){&mir::fatal_error_except};
 === modified file 'src/platform/graphics/mesa/CMakeLists.txt'
 --- src/platform/graphics/mesa/CMakeLists.txt	2014-07-29 15:42:47 +0000
 +++ src/platform/graphics/mesa/CMakeLists.txt	2014-08-05 11:38:58 +0000
@@ -46,8 +46,7 @@
    LINK_FLAGS "-Wl,--exclude-libs=ALL -Wl,--version-script,${symbol_map}"
+ )
--target_link_libraries(
--  mirplatformgraphicsmesa
++target_link_libraries(mirplatformgraphicsmesa
    mirplatform
    ${Boost_PROGRAM_OPTIONS_LIBRARY}
 === modified file 'src/platform/graphics/mesa/display_buffer.cpp'
 --- src/platform/graphics/mesa/display_buffer.cpp	2014-07-09 08:22:57 +0000
 +++ src/platform/graphics/mesa/display_buffer.cpp	2014-08-05 11:38:58 +0000
@@ -22,6 +22,7 @@
  #include "mir/graphics/display_report.h"
  #include "bypass.h"
  #include "gbm_buffer.h"
++#include "mir/fatal.h"
  #include <boost/throw_exception.hpp>
  #include <GLES2/gl2.h>
@@ -143,18 +144,18 @@
      glClear(GL_COLOR_BUFFER_BIT);
      if (!egl.swap_buffers())
--        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to perform initial surface buffer swap"));
++        fatal_error("Failed to perform initial surface buffer swap");
      listener->report_successful_egl_buffer_swap_on_construction();
      scheduled_bufobj = get_front_buffer_object();
      if (!scheduled_bufobj)
--        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to get frontbuffer"));
++        fatal_error("Failed to get frontbuffer");
      for (auto& output : outputs)
+     {
          if (!output->set_crtc(scheduled_bufobj->get_drm_fb_id()))
--            BOOST_THROW_EXCEPTION(std::runtime_error("Failed to set DRM crtc"));
++            fatal_error("Failed to set DRM crtc");
+     }
      egl.release_current();
@@ -256,7 +257,7 @@
       * corresponding to the front buffer.
       */
      if (!bypass_buf && !egl.swap_buffers())
--        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to perform initial surface buffer swap"));
++        fatal_error("Failed to perform initial surface buffer swap");
      mgm::BufferObject *bufobj;
      if (bypass_buf)
@@ -271,7 +272,7 @@
+     }
      if (!bufobj)
--        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to get front buffer object"));
++        fatal_error("Failed to get front buffer object");
      /*
       * Schedule the current front buffer object for display, and wait
@@ -284,14 +285,14 @@
+     {
          if (!bypass_buf)
              bufobj->release();
--        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to schedule page flip"));
++        fatal_error("Failed to schedule page flip");
+     }
      else if (needs_set_crtc)
+     {
          for (auto& output : outputs)
+         {
              if (!output->set_crtc(bufobj->get_drm_fb_id()))
--                BOOST_THROW_EXCEPTION(std::runtime_error("Failed to set DRM crtc"));
++                fatal_error("Failed to set DRM crtc");
+         }
          needs_set_crtc = false;
+     }
@@ -397,7 +398,7 @@
+ {
      if (!egl.make_current())
+     {
--        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to make EGL surface current"));
++        fatal_error("Failed to make EGL surface current");
+     }
+ }
 === modified file 'src/platform/graphics/mesa/real_kms_output.cpp'
 --- src/platform/graphics/mesa/real_kms_output.cpp	2014-07-09 10:48:47 +0000
 +++ src/platform/graphics/mesa/real_kms_output.cpp	2014-08-05 11:38:58 +0000
@@ -18,11 +18,7 @@
  #include "real_kms_output.h"
  #include "page_flipper.h"
--
--#include <boost/throw_exception.hpp>
--#include <boost/exception/info.hpp>
--
--#include <stdexcept>
++#include "mir/fatal.h"
  #include <vector>
  #include <string.h> // strcmp
@@ -166,7 +162,7 @@
      connector = resources.connector(connector_id);
      if (!connector)
--        BOOST_THROW_EXCEPTION(std::runtime_error("No DRM connector found\n"));
++        fatal_error("No DRM connector found");
      // TODO: What if we can't locate the DPMS property?
      for (int i = 0; i < connector->count_props; i++)
@@ -202,9 +198,10 @@
  bool mgm::RealKMSOutput::set_crtc(uint32_t fb_id)
+ {
      if (!ensure_crtc())
--        BOOST_THROW_EXCEPTION(std::runtime_error("Output " +
--            connector_name(connector.get()) +
--            " has no associated CRTC to set a framebuffer on"));
++    {
++        fatal_error("Output %s has no associated CRTC to set a framebuffer on",
++                    connector_name(connector.get()).c_str());
++    }
      auto ret = drmModeSetCrtc(drm_fd, current_crtc->crtc_id,
                                fb_id, fb_offset.dx.as_int(), fb_offset.dy.as_int(),
@@ -235,12 +232,8 @@
 , 0, 0, nullptr, 0, nullptr);
      if (result)
+     {
--        std::string const msg =
--            "Couldn't clear output " + connector_name(connector.get());
--
--        BOOST_THROW_EXCEPTION(
--            ::boost::enable_error_info(std::runtime_error(msg))
--                << (boost::error_info<KMSOutput, decltype(result)>(result)));
++        fatal_error("Couldn't clear output %s (drmModeSetCrtc = %d)",
++                   connector_name(connector.get()).c_str(), result);
+     }
      current_crtc = nullptr;
@@ -252,10 +245,10 @@
      if (power_mode != mir_power_mode_on)
          return true;
      if (!current_crtc)
--        BOOST_THROW_EXCEPTION(std::runtime_error("Output " +
--            connector_name(connector.get()) +
--            " has no associated CRTC to schedule page flips on"));
--
++    {
++        fatal_error("Output %s has no associated CRTC to schedule page flips on",
++                   connector_name(connector.get()).c_str());
++    }
      return page_flipper->schedule_flip(current_crtc->crtc_id, fb_id);
+ }
@@ -265,10 +258,10 @@
      if (power_mode != mir_power_mode_on)
          return;
      if (!current_crtc)
--        BOOST_THROW_EXCEPTION(std::runtime_error("Output " +
--            connector_name(connector.get()) +
--            " has no associated CRTC to wait on"));
--
++    {
++        fatal_error("Output %s has no associated CRTC to wait on",
++                   connector_name(connector.get()).c_str());
++    }
      page_flipper->wait_for_flip(current_crtc->crtc_id);
+ }
@@ -283,9 +276,7 @@
                  gbm_bo_get_width(buffer),
                  gbm_bo_get_height(buffer)))
+         {
--            BOOST_THROW_EXCEPTION(
--                ::boost::enable_error_info(std::runtime_error("drmModeSetCursor() failed"))
--                    << (boost::error_info<KMSOutput, decltype(result)>(result)));
++            fatal_error("drmModeSetCursor failed (returned %d)", result);
+         }
          has_cursor_ = true;
@@ -300,9 +291,7 @@
                                              destination.x.as_uint32_t(),
                                              destination.y.as_uint32_t()))
+         {
--            BOOST_THROW_EXCEPTION(
--                ::boost::enable_error_info(std::runtime_error("drmModeMoveCursor() failed"))
--                    << (boost::error_info<KMSOutput, decltype(result)>(result)));
++            fatal_error("drmModeMoveCursor failed (returned %d)", result);
+         }
+     }
+ }
 === modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
 --- tests/acceptance-tests/test_server_shutdown.cpp	2014-07-21 03:35:31 +0000
 +++ tests/acceptance-tests/test_server_shutdown.cpp	2014-08-05 11:38:58 +0000
@@ -23,6 +23,7 @@
  #include "mir/compositor/display_buffer_compositor_factory.h"
  #include "mir/input/composite_event_filter.h"
  #include "mir/run_mir.h"
++#include "mir/fatal.h"
  #include "mir_test_framework/display_server_test_fixture.h"
  #include "mir_test/fake_event_hub_input_configuration.h"
@@ -419,6 +420,70 @@
      });
+ }
++TEST_F(ServerShutdown, server_removes_endpoint_on_mir_fatal_error_abort)
++{   // Even fatal errors sometimes need to be caught for critical cleanup...
++    struct ServerConfig : TestingServerConfiguration
++    {
++        void on_start() override
++        {
++            sync.wait_for_signal_ready_for();
++            mir::fatal_error("Bang");
++        }
++
++        mir::FatalErrorStrategy on_error{mir::fatal_error_abort};
++        mtf::CrossProcessSync sync;
++    };
++
++    ServerConfig server_config;
++    launch_server_process(server_config);
++
++    run_in_test_process([&]
++    {
++        ASSERT_TRUE(file_exists(server_config.the_socket_file()));
++
++        server_config.sync.signal_ready();
++
++        auto result = wait_for_shutdown_server_process();
++        EXPECT_EQ(mtf::TerminationReason::child_terminated_by_signal, result.reason);
++        // Under valgrind the server process is reported as being terminated
++        // by SIGKILL because of multithreading madness.
++        // TODO: Investigate if we can do better than this workaround
++        EXPECT_TRUE(result.signal == SIGABRT || result.signal == SIGKILL);
++
++        EXPECT_FALSE(file_exists(server_config.the_socket_file()));
++    });
++}
++
++
++TEST_F(ServerShutdown, server_removes_endpoint_on_mir_fatal_error_except)
++{   // Even fatal errors sometimes need to be caught for critical cleanup...
++    struct ServerConfig : TestingServerConfiguration
++    {
++        void on_start() override
++        {
++            sync.wait_for_signal_ready_for();
++            mir::fatal_error("Bang");
++        }
++
++        mtf::CrossProcessSync sync;
++    };
++
++    ServerConfig server_config;
++    launch_server_process(server_config);
++
++    run_in_test_process([&]
++    {
++        ASSERT_TRUE(file_exists(server_config.the_socket_file()));
++
++        server_config.sync.signal_ready();
++
++        auto result = wait_for_shutdown_server_process();
++        EXPECT_EQ(mtf::TerminationReason::child_terminated_normally, result.reason);
++
++        EXPECT_FALSE(file_exists(server_config.the_socket_file()));
++    });
++}
++
  struct OnSignal : ServerShutdown, ::testing::WithParamInterface<int> {};
  TEST_P(OnSignal, removes_endpoint_on_signal)
 === modified file 'tests/unit-tests/CMakeLists.txt'
 --- tests/unit-tests/CMakeLists.txt	2014-07-31 12:38:17 +0000
 +++ tests/unit-tests/CMakeLists.txt	2014-08-05 11:38:58 +0000
@@ -19,6 +19,7 @@
    test_thread_name.cpp
    test_default_emergency_cleanup.cpp
    test_basic_observers.cpp
++  test_fatal.cpp
    test_fd.cpp
+ )
 === added file 'tests/unit-tests/test_fatal.cpp'
 --- tests/unit-tests/test_fatal.cpp	1970-01-01 00:00:00 +0000
 +++ tests/unit-tests/test_fatal.cpp	2014-08-05 11:38:58 +0000
@@ -0,0 +1,59 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Author: Daniel van Vugt <daniel.van.vugt@canonical.com>
++ *         Alan Griffiths <alan@octopull.co.uk>
++ */
++
++#include "mir/fatal.h"
++#include <gtest/gtest.h>
++#include <gmock/gmock.h>
++#include <csignal>
++
++using namespace testing;
++
++TEST(FatalTest, abort_formats_message_to_stderr)
++{
++    mir::FatalErrorStrategy on_error{mir::fatal_error_abort};
++
++    EXPECT_DEATH({mir::fatal_error("%s had %d %s %s", "Mary", 1, "little", "lamb");},
++                 "Mary had 1 little lamb");
++}
++
++TEST(FatalTest, abort_raises_sigabrt)
++{
++    mir::FatalErrorStrategy on_error{mir::fatal_error_abort};
++
++    EXPECT_EXIT({mir::fatal_error("Hello world");},
++                KilledBySignal(SIGABRT),
++                "Hello world");
++}
++
++TEST(FatalTest, throw_formats_message_to_what)
++{
++    EXPECT_THROW(
++        mir::fatal_error("%s had %d %s %s", "Mary", 1, "little", "lamb"),
++        std::runtime_error);
++
++    try
++    {
++        mir::fatal_error("%s had %d %s %s", "Mary", 1, "little", "lamb");
++    }
++    catch (std::exception const& x)
++    {
++        EXPECT_THAT(x.what(), StrEq("Mary had 1 little lamb"));
++    }
++}
++