[PATCH RFC] do not throw in std::make_exception_ptr

[Jonathan Wakely]

On 03/08/16 14:26 +0300, Gleb Natapov wrote:
>On Wed, Aug 03, 2016 at 10:48:27AM +0100, Jonathan Wakely wrote:
>> Does bad_exception need to move to <bits/exception.h> ?
>>
>>
>I think only std::exception is really needed by <typeinfo>. When I
>did header split I when with "move as much as possible" approach, not
>the other way around. You seems to suggest the opposite approach. I'll
>try it.

Yes, that way <typeinfo> and <cxxabi.h> are as small as possible, and
only declare what they need.

Code that wants std::bad_exception or std::set_unexpected() should
include <exception>, as before.

>> >     swap(exception_ptr& __lhs, exception_ptr& __rhs)
>> >     { __lhs.swap(__rhs); }
>> >
>> > -  } // namespace __exception_ptr
>> > +    template<typename _Ex>
>> > +      static void dest_thunk(void* x) {
>> > +          reinterpret_cast<_Ex*>(x)->_Ex::~_Ex();
>> > +      }
>>
>> This isn't a name reserved for implementors, it needs to be uglified,
>> e.g. __dest_thunk.
>>
>OK.
>
>> This function should be declared 'inline' too.
>>
>We take a pointer to it. How can it be inline?

Well it certainly *can* be, declaring it inline doesn't mean you can't
take its address.

Currently it's 'static' which means every translation unit that calls
make_exception_ptr will instantiate a different copy of the function
template. That produces more object code than needed, so it should not
be static. It's a tiny function defined in a header, so should be
'inline'.

Practically speaking, making it 'inline' doesn't make much difference,
because an instantiated function template will produce a weak symbol
anyway, which as the same effect as declaring it inline. But there's
no reason _not_ to declare it inline.

>>
>> > +  } // namespace __exception_ptr
>> >
>> >   /// Obtain an exception_ptr pointing to a copy of the supplied object.
>> >   template<typename _Ex>
>> > @@ -173,7 +184,15 @@ namespace std
>> > #if __cpp_exceptions
>> >       try
>> > 	{
>> > -	  throw __ex;
>> > +#if __cpp_rtti && !_GLIBCXX_HAVE_CDTOR_CALLABI
>> > +          void *e = __cxxabiv1::__cxa_allocate_exception(sizeof(_Ex));
>>
>> Again, 'e' isn't a reserved name.
>It is local variable, why should it be reserved?

Because otherwise this valid C++ program won't compile:

#define e 2.71828
#include <exception>
int main() { }


>
>>
>> > +          (void)__cxxabiv1::__cxa_init_primary_exception(e, &typeid(__ex),
>> > +                                           __exception_ptr::dest_thunk<_Ex>);
>> > +          new (e) _Ex(__ex);
>>
>> If the copy constructor of _Ex throws an exception should we call
>> std::terminate here?
>>
>> That's what would have happened previously, I believe.
>>
>I do not think so. throw compiles to something like:
>
>  __cxa_allocate_exception
>  call move_or_copy_constructor
>  __cxa_throw
>
>If move_or_copy_constructor throws the code does not terminate, but
>catch() gets different exception instead.
>
>> I don't think we want to catch that exception and store it
>> in the exception_ptr in place of the __ex object we were asked to
>> store.
>>
>I wrote a test program below to check current behaviour and this is what code
>does now.
>
>#include <iostream>
>#include <exception>
>
>struct E {
>    E()  {}
>    E(const E&) { throw 5; }
>};
>
>int main() {
>    auto x = std::make_exception_ptr(E());
>    try {
>        std::rethrow_exception(x);
>    } catch(E& ep) {
>        std::cout << "E" << std::endl;
>    } catch (int& i) {
>        std::cout << "int" << std::endl;
>    }
>}

Huh. If I'm reading the ABI spec correctly, we should terminate if the
copy constructor throws.

We don't seem to do that even without exception_ptr involved:

#include <iostream>
#include <exception>

struct E {
    E()  {}
    E(const E&) { throw 5; }
};

int main() {
    static E e;
    try {
        throw e;
    } catch(E& ep) {
        std::cout << "E" << std::endl;
    } catch (int& i) {
        std::cout << "int" << std::endl;
    }
}

>> So on that basis, do we need the try/catch around your new code?
>>
>> Can we just do:
>>
>>  template<typename _Ex>
>>    exception_ptr    make_exception_ptr(_Ex __ex) _GLIBCXX_USE_NOEXCEPT
>>    {
>> #if __cpp_exceptions
>> # if __cpp_rtti && !_GLIBCXX_HAVE_CDTOR_CALLABI
>>          void *__ptr = __cxxabiv1::__cxa_allocate_exception(sizeof(_Ex));
>>          (void)__cxxabiv1::__cxa_init_primary_exception(__ptr, &typeid(__ex),
>>                                           __exception_ptr::__dest_thunk<_Ex>);
>>          new (__ptr) _Ex(__ex);
>> # else
>>      try
>> 	{
>> 	  throw __ex;
>> 	}
>>      catch(...)
>> 	{
>> 	  return current_exception();
>> 	}
>> # endif
>> #else
>>      return exception_ptr();
>> #endif
>>    }
>>
>> The noexcept spec will cause it to terminate if the copy constructor
>> of _Ex throws.
>>
>>
>> > +          return exception_ptr(e);
>> > +#else
>> > +          throw __ex;
>> > +#endif
>> > 	}
>> >       catch(...)
>> > 	{
>>
>>
>> > diff --git a/libstdc++-v3/libsupc++/unwind-cxx.h b/libstdc++-v3/libsupc++/unwind-cxx.h
>> > index 9121934..11da4a7 100644
>> > --- a/libstdc++-v3/libsupc++/unwind-cxx.h
>> > +++ b/libstdc++-v3/libsupc++/unwind-cxx.h
>> > @@ -31,7 +31,7 @@
>> > // Level 2: C++ ABI
>> >
>> > #include <typeinfo>
>> > -#include <exception>
>> > +#include <bits/exception.h>
>> > #include <cstddef>
>> > #include "unwind.h"
>> > #include <bits/atomic_word.h>
>> > @@ -62,7 +62,7 @@ namespace __cxxabiv1
>> > struct __cxa_exception
>> > {
>> >   // Manage the exception object itself.
>> > -  std::type_info *exceptionType;
>> > +  const std::type_info *exceptionType;
>> >   void (_GLIBCXX_CDTOR_CALLABI *exceptionDestructor)(void *);
>>
>> The __cxa_exception type is defined by
>> https://mentorembedded.github.io/cxx-abi/abi-eh.html#cxx-data and this
>> doesn't conform to that spec. Is this change necessary?
>>
>
>--
>			Gleb.