[Spice-devel] [PATCH 12/22] Add exception handling classes

[Frediano Ziglio]

> > On 2 Mar 2018, at 18:13, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > 
> > On Fri, Mar 02, 2018 at 02:00:23PM +0100, Christophe de Dinechin wrote:
> >> 
> >> 
> >>> On 2 Mar 2018, at 09:03, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>> 
> >>>>> 
> >>>>> On 1 Mar 2018, at 13:13, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>>>> 
> >>>>>> 
> >>>>>> From: Christophe de Dinechin <dinechin at redhat.com>
> >>>>>> 
> >>>>>> Throwing 'runtime_error' directly should be reserved for the support
> >>>>>> library.  Add an 'Error' class as a base class for all errors thrown
> >>>>>> by the streaming agent, as well as subclasses used to discriminate
> >>>>>> between categories of error.
> >>>>>> 
> >>>>>> The Error class offers:
> >>>>>> - a 'format_message' member function that formats a message without
> >>>>>> allocating memory, so that there is no risk of throwing another
> >>>>>> exception at throw time.
> >>>>>> 
> >>>>> 
> >>>>> why not formatting the message constructing the object so before the
> >>>>> throw?
> >>>> 
> >>>> That requires dynamic allocation, which is entirely unnecessary and
> >>>> potentially dangerous (replacing one exception kind with another).
> >>>> 
> >>> 
> >>> As explained by my string.c_str example your interface is prone to
> >>> errors.
> >> 
> >> The interface is not “prone to errors” at all. The scenario you
> >> describe makes no sense, since the whole interface is designed to
> >> avoid dynamic strings. This is now explicitlyi documented (see
> >> https://github.com/c3d/spice-streaming-agent/blob/c%2B%2B-refactoring/include/spice-streaming-agent/errors.hpp,
> >> not yet shared, I’m not entirely done with the reviews). The
> >> constructor comment now says:
> >> 
> >> 	Constructor takes the base message returned by what()
> >> 	The message must remain valid over the whole lifetime of the exception.
> >> 	It is recommended to only use static C strings, since formatting of any
> >> 	dynamic argument is done by 'format_message'
> > 
> > This behaviour seems inconsistent with std::runtime_error(const char *)
> > which as far as I can tell does not have this lifetime expectation.
> 
> You are correct, but please note that std::runtime_error takes a string as
> input argument, making it very clear that you are dealing with a ‘string’
> underneath, not a const char*.
> 
> > 
> >> 
> >> Anyway, your example is misleading. Anybody that passes the result of
> >> string::c_str() to a const char * without proper consideration for the
> >> lifetime is at fault. The const char * interface itself cannot be
> >> faulted for that.
> > 
> > For what it's worth, I usually expect const char * interfaces to make
> > their own copy of the const char * arg if they need it after they
> > return, and I've rarely seen exceptions to this expectation.
> 
> std::vector and all standard library containers? You do a push_back of a
> const char * in an std::vector<string>, you get a copy. If you do that in an
> std::vector<const char *>, you don’t.
> 
> In C++, raw pointers, including const char *, always have the explicit
> meaning that you manage the lifetime. So if you have “rarely seen
> exceptions”, you did not look hard enough IMHO ;-)
> 
> Let me give an example from *our own code*. ConcreteConfigureOption takes two
> const char * arguments. Does it make copies? No. Same thing with the related
> AddOption method. Anything particularly surprising there? I don’t think so.
> 
> Of course, we could switch to “strings everywhere” (which Lukas suggested at
> a point). But that’s wrong too, it causes unnecessary allocations and code
> bloat (see below what I’m talking about).
> 
> > 
> > If I understood properly, things are designed this way to avoid running out
> > of memory while building the exception. I'm definitely not in favour of
> > having an error-prone API to handle a very rare case which the rest of the
> > code is most likely not going to be able to cope with anyway.
> 
> Yet another iteration of the “bad_alloc” theory. You know that this is
> starting to get on my nerves? As stated *REPEATEDLY* before, bad_alloc is
> only one of the reasons for doing things that way.
> 
> To make this clear, let me elaborate a little on *one* other reason (I listed
> a few others), code bloat and runtime cost. Let’s say we want to store a
> write error with an errno. The two approaches under consideration are:
> 
> 	A. throw WriteError(message, operation, errno);
> 	B. throw std::runtime_error(message + “ in “ + operation + “: “ +
> 	strerror(errno))
> 
> I’ll briefly mention, as a gentle reminder of some of the “other reasons”,
> that (A) is
> 1. shorter to type
> 2. easier toi read
> 3. less error prone
> 4. more likely to guarantee consistent messages
> 
> and I will instead focus on the code being generated in both cases.
> 
> In case A, the code is roughly equivalent to:
> 
> 	tmp = WriteError_constructor(message, operation, errno)
> 	runtime_cxa_throw(tmp)
> 
> So roughy two calls, passing a total of four register args, no exception
> landing pad.
> 
> In case B, the code is roughly equivalent to (where LPn is a “landing pad”, a
> piece of code that cleans up if an exception is thrown):
> 
> 	tmp1 = string_constructor(message);		// LP1
> 	tmp2 = string_constructor(“ in “);			// LP2
> 	tmp3 = string_operator_plus(tmp1, tmp2);	// LP3
> 	tmp4 = string_constructor(operation);		// LP4
> 	tmp5 = string_operator_plus(tmp3, tmp4);	// LP5
> 	tmp6 = strerror(errno);
> 	tmp7 = string_constructor(tmp6);			// LP6
> 	tmp8 = string_operator_plus(tmp5, tmp7);	// LP7
> 	tmp9 = runtime_error_constructor(tmp8);	// LP8
> 	string_delete(tmp8);
> 	string_delete(tmp7);
> 	string_delete(tmp5);
> 	string_delete(tmp4);
> 	string_delete(tmp3);
> 	string_delete(tmp2);
> 	string_delete(tmp1);
> 	runtime_cxa_throw(tmp9)
> 
> LP8:	string_delete(tmp8);	// Chain to next landing pad
> LP7: string_delete(tmp7);
> LP6: string_delete(tmp6);
> LP5: string_delete(tmp5);
> LP4: string_delete(tmp4);
> LP3: string_delete(tmp3);
> LP2: string_delete(tmp2);
> LP1: string_delete(tmp1);
> 	runtime_rethrow();	// (__UnwindResume) to propagate bad_alloc
> 
> I omitted a few details for clarity, like the exception handling tables, etc.
> But the gist of it is that now *at each throw point* you have something like
> 25+ calls being generated (some are inlined), passing a total of at least 25
> register args…
> 
> Well, 10+ times worse? That’s bad enough. But it gets better.
> 
> Let’s consider runtime cost now. What do these calls do?
> 
> In case A, three word-size copies in the constructor of WriteError, and I
> think that’s about it.
> 
> In case B, each of the strings allocates memory and copies the string in it.
> Let’s be optimistic and say that it’s one call per allocation, and that you
> get it from a free list (let’s say something like 5 loads and three stores,
> that’s rather optimistic). So now we have another 7 calls, and well over 50
> load/store instructions just for the memory allocation itself. And then the
> base message is copied at least four times. Cache, cache, cache, buy me more
> cache!
> 
> In short, using B looks innocuous, but we replaced, roughly
> 	A. two calls, four register args, three memory copies
> with, at runtime,
> 	B. At least 25 calls, at least 25 register args, and four memory copies of
> 	the original message.
> 
> So here too, at least an order of magnitude worse, probably closer to two.
> 
> Guess what. After writing all this, I realized I had forgotten one
> concatenation for “: “, so that all my numbers are understimated… Oh well…
> 
> 
> Meta note: How long do you think it took me to write this reply, in a
> probably futile attempt to explain that there is more to it than bad_alloc?
> I’m sorry, but this is really grinding. Now, that is not an entire waste of
> time if you take the time to really understand what I explained, and if I
> get the benefit that you never mention bad_alloc ever again ;-)
> 
> Otherwise, if you only see that as some kind of “justification”, my time is
> wasted, so is yours, and everybody walks out of it unhappy.
> 
> 
> > 
> > 
> 
> > Christophe
> 
> 

An improvement to the declaration to avoid some issues could be

template <std::size_t N>
Error(const char (&msg)[N]) : ...
{
  ...
}

but even so I find that being able to pass a dynamic string adding
some information is really helpful.
In many cases you want to add information to the exception and
is quite common to catch one exception, add another informations
and throw a more detailed exception.

Frediano