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

[Jonathon Jongsma]

On Wed, 2018-03-28 at 22:50 +0200, Christophe de Dinechin wrote:
> > On 28 Mar 2018, at 22:32, Christophe de Dinechin <cdupontd at redhat.c
> > om> wrote:
> > 
> > 
> > 
> > > On 28 Mar 2018, at 20:46, Jonathon Jongsma <jjongsma at redhat.com>
> > > wrote:
> > > 
> > > On Wed, 2018-03-28 at 17:42 +0200, Christophe de Dinechin wrote:
> > > > Somewhat OT, but hopefully informative in the context of this
> > > > thread.
> > > > 
> > > > 
> > > > > On 26 Mar 2018, at 18:24, Jonathon Jongsma <jjongsma at redhat.c
> > > > > om>
> > > > > wrote:
> > > > > 
> > > > > As teuf mentioned in a different email, std::runtime_error in
> > > > > C++11
> > > > > has a const char* constructor:
> > > > 
> > > > Since the presence of this constructor was used repeatedly as
> > > > an
> > > > argument, I’ve asked a few times if someone could tell me why
> > > > this
> > > > constructor was added in C++ 11. So far, nobody answered. I did
> > > > not
> > > > think it was so hard to google it. Oh well, since nobody else
> > > > took up
> > > > the challenge, time for me to answer the question…
> > > > 
> > > > Here is one source giving a relatively short explanation: https
> > > > ://sta
> > > > ckoverflow.com/questions/29052647/c11-introduced-exception-
> > > > constructors-taking-const-char-but-why. I quote: "This allows
> > > > (or at
> > > > least is apparently intended to facilitate--see more below) the
> > > > implementation to eliminate copies in cases where it can detect
> > > > (by
> > > > means that are not themselves standardized) that what's being
> > > > passed
> > > > is a string literal or something else with static storage
> > > > duration.”
> > > > See also the note at the end of that answer: "[ Oxford: The
> > > > proposed
> > > > resolution simply addresses the issue of constructing the
> > > > exception
> > > > objects with const char* and string literals without the need
> > > > to
> > > > explicit include or construct a std::string. ]”
> > > > 
> > > > So what happened is that the committee knew that having an
> > > > std::string constructed for all runtime errors was problematic,
> > > > as
> > > > far back as 2000. This is defect 254, see http://std.dkuug.dk/j
> > > > tc1/sc
> > > > 22/wg21/docs/lwg-closed.html. It was a bit annoying if
> > > > implementors
> > > > took a great deal of care and effort to ensure that you could
> > > > throw
> > > > an exception reliably in low-memory situations (see section
> > > > 4.2.2 of 
> > > > https://github.com/itanium-cxx-abi/cxx-abi/blob/master/HP-excep
> > > > tion-9
> > > > 90818.pdf if you are curious about the kind of shenanigans I’m
> > > > talking about) only to see all these effort thwarted by the
> > > > (legacy)
> > > > interface of std::runtime_error.
> > > > 
> > > > However, you can imagine that back in 2000, deciding to remove
> > > > the
> > > > std::string constructor was difficult. So the problem was not
> > > > easy to
> > > > fix, and it shouldn’t be too hard to believe that there was a
> > > > lot of
> > > > discussion (254 is listed as one of the hot topics for the
> > > > Redmond
> > > > meeting). Before the Oxford meeting, the decision was to accept
> > > > throwing bad_alloc as a fact of life. But as you can see at the
> > > > bottom of bug 254, the consensus on a better longer-term
> > > > resolution
> > > > for C++0x, which became C++11, was to add a const char * that
> > > > would
> > > > allow the implementation to special-case std::runtime_error and
> > > > avoid
> > > > the copy. In the Oxford meeting, that resolution was confirmed,
> > > > and
> > > > the change made it to C++11.
> > > > 
> > > > Out of curiosity, I checked both clang and g++, and they don’t
> > > > actually store a string in std::runtime_error, I suspect in
> > > > large
> > > > part because of another related issue, namely the risk of
> > > > throwing
> > > > from string copy constructors. though they don’t seem to be
> > > > doing the
> > > > optimization either, because it’s quite hard relative to the
> > > > benefits
> > > > :-( So no worries, on clang, if you are really out of memory,
> > > > you
> > > > will not throw bad_allloc or end up in terminate(). No sir, the
> > > > standard forbids it. No, instead, you will segfault writing
> > > > into a
> > > > null pointer (line 74 of https://llvm.org/viewvc/llvm-project/l
> > > > ibcxx/
> > > > trunk/src/include/refstring.h?view=markup if you are curious).
> > > > So
> > > > much better.
> > > > 
> > > > Bottom line: the very reason there is a const char *
> > > > constructor in
> > > > std::runtime_error is to provide a workaround for a known
> > > > design bug.
> > > > There is no value in blindly repeating that design bug.
> > > > 
> > > > 
> > > > Cheers,
> > > > Christophe
> > > > 
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > 
> > > 
> > > This doesn't seem like a fruitful discussion any longer.
> > 
> > It’s long been unproductive, only blocking my patch series for no
> > good reason.
> > 
> > > I disagree
> > > with your opinion that *for our use cases* it's more desirable to
> > > avoid
> > > a small memory allocation than to make the class more resilient
> > > to
> > > programmer error. But you clearly won't be convinced, and it's
> > > not the
> > > most important issue in the world. So I'll withdraw from this
> > > discussion. 
> > 
> > And I find it distasteful that you still insist using std::string
> > would make it “more resilient to programmer error” when I’ve proven
> > it was not the case and spent an inordinate amount of time
> > explaining why.
> 
> By which I mean among other things what I wrote earlier today:
> 
> > Therefore, it is important for the interface of Error to “strongly
> > suggest” constant strings where I want constant strings. By the
> > mean of a “const char *”. Where I expect a dynamic string, I use
> > std::string (or const std::string & or whatever floats you boat).
> 
> As long as you keep ignoring that point, as long as you keep ignoring
> that detecting an inadvertent use of std::string input is actually
> *by design* and *desirable*, you have not understood that your
> proposal *in our use case* makes things significantly *less*
> resilient to the most important class of programmer error, where a
> programmer would mistakenly attempt to pass something like “can’t
> write “ + file.
> 
> So you will keep blocking this patch indefinitely because you don’t
> understand it. And I find this unacceptable.
> 


OK, let's try to be more productive here. Please accept this email as a
good-faith attempt to reach concensus.

To recap briefly, my suggestion was simply to store the string passed
to the Error constructor internally so that we did not segfault if that
string was freed by the caller before the exception was caught. If I
understand your position, the argument AGAINST this is basically:

   1. It requires additional memory allocation (which introduces the
      potential of throwing a std::bad_alloc exception while attempting to
      throw a different exception)
   2. The code is slower and more bloated (due to additional memory
      allocation, or e.g. constructing a new std::string for internal
      storage)
   3. It "encourages" people to use dynamically allocated strings for
      error messages which is inherently bad.

Is that accurate? Am I missing any? I think #3 is your primary
argument, right? For now I'm going to assume that I represented your
views accurately and I'll briefly address each of these points below.

Now, the argument FOR copying the string is basically:
   A. we don't want a segfault if somebody passes a string that's not a
      static constant.

I have a really hard time caring about the bad_alloc issue. If we fail
to allocate enough memory for a simple error message, we are going to
have much bigger problems. So I don't consider #1 to be a significant
issue. I also don't really really care about performance in exception-
handling situations, so #2 is also a non-issue for me. That basically
leaves me to decide whether it's more important discourage using
dynamic strings or whether it's more important to avoid segfaults when
somebody does use dynamic strings. 

But as I thought about it more, I found that I'm a bit confused here
about exactly why you consider dynamic strings to be evil. Is it just
because they can't be translated? Certainly, to be translatable, the
*template* for an error message must be a static constant. But if we
want to include any useful context to the user, the error message
itself must be somewhat dynamic. In other words, the error message
template "Failed to read file %s" is a static constant, but the error
message itself ("Failed to read file /home/cddd/foo.txt", or the
equivalent in russian, etc) is necessarily a dynamic string. (If this
is not the reason that you insist on static strings, please enlighten
me, because clearly I'm missing something.)

In any case, your patch also uses "dynamic" strings as error messages,
but it does so in a slightly different way. You store a pointer to the
static error message *components* within the Error class, but then you
format the message later (when the exception is caught) to create the
dynamic error message. According to your earlier responses on this
topic, you claim that this is done to avoid memory allocation during
the exception handling process (since format_message() uses a buffer
provided by the exception handler rather than allocating its own
buffer) and to improve performance by delaying formatting until we
actually need to do it. But these issues essentially map to #1 and #2
above, which I've already said I don't really care about. In addition,
it introduces a discrepency between the message that is returned by
what() and the message that is returned by format_message(), which I
find undesirable. So in my opinion, the lazy format_message() feature
adds complexity for very little gain. 

In my opinion, we should simply translate and format the full error
message at the point of throw and pass this formatted and translated
string to the Error constructor. 

   throw WriteError(format(_("Write failed writing header for file
   '%s'"), filename));

I think this has a number of advantages. 
 * It's simpler
 * Since the message is already translated, we don't need to worry
   about whether the string is static or dynamic (right?)
 * Following on from the last point, since we don't have to discourage
   dyanamic strings anymore, we can copy the string and store it
   internally to avoid potential segfaults
 * The what() function would return the fully formatted error message
   rather than just the static portion of the message.
 * It's easier to translate the strings [*]

The only drawbacks in my mind are the memory allocation and performance
issues I mentioned above. And I personally think that the advantages I
mentioned above far outweigh those disadvantages.

Jonathon




[*] In an earlier email, you gave an example of how you might handle
translations with your lazy formatting approach, but I'm not actually
sure that it would work. Quoting your email:

   This naive approach yields “l’écriture a été interrompue pendant le
   format pour le fichier ‘Hello.txt'”, which is understandable, but
   sounds a bit strange. It would be better to instead generate
   “L’écriture du format a été interrompue pour le fichier
   ‘Hello.txt’”; which involves word reordering and grammatical pairing
   (e.g. contractions, male/female, etc). In order to do that, you can
   take a more subtle approach where instead of translating pieces
   individually, you translate the complete message, but still
   carefully ignore the parts that you know are variable. So you could
   do instead:
   	snprintf(translated, sizeof(translated), “%s writing %s for
   file ‘%%s’”, what(), operation);
   	int written = snprintf(buffer, size, _(translated),
   filename.c_str());
   	return append_strerror(buffer, size, written); 

So you envision building the translatable strings from the static
components that you pass to the error constructor. But notice that the
variable "translated" is not a static constant string, so I don't think
that "_(translated)" would actually work. Maybe this example was not
intended to be taken literally? But I can't think of a good way that
you could extract a list of static constant strings from the code in a
way that would work with gettext.