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

[Jonathan Wakely <jwakely at redhat dot com>]

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.