Just another followup on this:
An additional barrier that hasn't been mentioned has to do with exceptions
and destructors in our code. Previously we had a discussion on the list
about destructors throwing exceptions -- it seems that wesnoth has many
classes with destructors which are designed to throw exceptions, presumably
because some developers didn't fully understand the C++ rules about this.
In C++98, a destructor may throw an exception but
- children of the class may not be destroyed properly and their memory
leaked
- if the destructor was called during "stack unwinding" from an earlier
exception, the program is immediately terminated.
This is quite bad for us -- wesnoth uses many long range exceptions in the
course of "normal" program execution, for instance whenever a turn ends, or
a level ends, or a new level is loaded. If any of these events coincides
with a throwing destructor, the program crashes without an error message.
Not too common, but still a bug resulting in a crash.
In C++11 the rules are much stricter. If a destructor throws an exception
at all, the program is immediately terminated, whether or not there is
already an exception on the stack. So all of the instances of the unsafe
behavior will immediately crash the program.
After the first email thread, I went through many of the destructors and
wrapped them in "try { ... } catch (...) { } " blocks to prevent crashes.
(or "try {...} catch (...) { ... logging ... }" )
However in some cases this doesn't work, because the class is actually
designed to perform all of its actions in the destructor, and these actions
need to throw exceptions to work properly. For instance the "save_blocker"
class is designed to catch calls to save the game or launch a save game
dialog and defer them, in situations where the game cannot be safely saved
(some sequence of actions is taking places). The blocker calls these
instead when it is destroyed. However, the dialogs may throw exceptions
like an end level exception if the user decided to quit, or a filesystem
exception if it wasn't able to save, etc. These exceptions need to go up
the stack and be handled somewhere else, or the dialog won't work correctly.
In C++98 this bug might only have resulted in a memory leak typically, a
crash only rarely. However in C++11 this will always result in a crash. The
only way to fix it is to get rid of "save_blocker" and come up with a
better solution. I made a short-term commit here:
https://github.com/wesnoth/wesnoth/commit/675c8a95f76604f6cac6ec77b14f98da046ab3eb
There are several other cases in the code that used this design
anti-pattern. For instance there are many "unit reset" classes which have
active destructors like this:
https://github.com/wesnoth/wesnoth/blob/master/src/unit.cpp#L2097
https://github.com/wesnoth/wesnoth/blob/master/src/game_board.cpp#L372
https://github.com/wesnoth/wesnoth/blob/master/src/game_board.cpp#L307
https://github.com/wesnoth/wesnoth/blob/master/src/game_board.cpp#L287
They all have "try { ... } catch (...) {} " as a workaround now, but this
is bad. If any of these unit map operations can throw exceptions, or if a
programmer changes any of that code to start throwing exceptions in the
future, for example to signal an OOS error or something, these reset
objects will be forced to suppress them, so it won't work properly and the
programmer won't really have any idea that that is going to happen.
Similarly this looks somewhat broken to me:
https://github.com/wesnoth/wesnoth/blob/master/src/replay.cpp#L933
Putting "commit_and_sync" in a destructor is a bad idea because obviously
lots of things can go wrong, like a network error. If it's in a destructor
then we have to either duplicate all the error handling code in the
destructor (bad), or crash (bad), or suppress the exceptions (bad).
Currently we are suppressing the exceptions, but their proper handler might
be doing something like "you have lost network connection, do you want to
save the game?", and it may be that if we ignore the errors we just crash
immediately after anyways. It seems that it should be possible to write the
program so that this stuff does not need to be in a destructor. If it's
just that the play controller objects are too complicated right now to do
that comfortably, that's a sign that we need to refactor to simplify things.
Realistically I don't think we can move to C++11 until we've reviewed the
code and problems like these have been properly dealt with, or the program
will just be extremely unstable.
Additionally if there are any bugs like this on wesnothd or campaignd, it
can easily create a security problem when we move to C++11, i.e. server
crashing because some function called in a destructor threw a UTF-8
exception or similar, when it might have only given a memory leak before.
Best Regards,
Chris Beck
On Fri, Nov 28, 2014 at 3:39 PM, chris beck <beck...@gmail.com> wrote:
> Yeah I guess I phrased it badly -- it's not that they won't implement some
> of C++11. Here's the quote from Herb Sutter:
>
> "Visual C++ is targeting C++14, so we’re treating all the new features in
> C++11 and C++14 as a single bucket of work to do."
>
> On Fri, Nov 28, 2014 at 3:05 PM, Alexander van Gessel <ai0...@gmail.com>
> wrote:
>
>> It reads more like they're prioritizing some C++14 features over C++11
>> ones. C++11 will probably be a target, but it won't be complete before
>> (their implementation of) C++14 is.
>>
>> On Fri, Nov 28, 2014 at 6:54 PM, chris beck <beck...@gmail.com> wrote:
>>
>>> Okay, but it seems that MSVC will not fully implement C++11 and instead
>>> is targeting C++14. So our hands are tied. Either we have to say "only the
>>> C++11 they managed to implement" or "wait for C++14".
>>>
>>>
>>> http://blogs.msdn.com/b/somasegar/archive/2013/06/28/cpp-conformance-roadmap.aspx
>>>
>>> Best Regards,
>>> Chris Beck
>>>
>>> On Wed, Nov 26, 2014 at 5:37 PM, <anonymissi...@arcor.de> wrote:
>>>
>>>> > I would guess that either we should say "only MSVC supported stuff
>>>> please"
>>>> > or drop support.
>>>>
>>>> Not only do windows builds profit from the Linux ones, but also the
>>>> other way round. There are sometimes bugs, wrong memory handling in
>>>> particular, that valgrind & Co just do not find, but the MSVC debugger
>>>> does. Dropping support - bad idea.
>>>> "Only MSVC supported" is bad idea too, considering the build issues on
>>>> windows even without C++ 11, in combination with the low number and/or
>>>> activity of windows devs.
>>>>
>>>> cheers,
>>>> anonymissimus
>>>>
>>>> _______________________________________________
>>>> Wesnoth-dev mailing list
>>>> Wesnoth-dev@gna.org
>>>> https://mail.gna.org/listinfo/wesnoth-dev
>>>>
>>>
>>>
>>> _______________________________________________
>>> Wesnoth-dev mailing list
>>> Wesnoth-dev@gna.org
>>> https://mail.gna.org/listinfo/wesnoth-dev
>>>
>>>
>>
>> _______________________________________________
>> Wesnoth-dev mailing list
>> Wesnoth-dev@gna.org
>> https://mail.gna.org/listinfo/wesnoth-dev
>>
>>
>
_______________________________________________
Wesnoth-dev mailing list
Wesnoth-dev@gna.org
https://mail.gna.org/listinfo/wesnoth-dev
