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

[Jonathan Wakely <jwakely at redhat dot com>]

On 28/07/16 10:20 +0300, Gleb Natapov wrote:
> [resent with hopefully correct libstdc++ mailing list address this time]
>
> Here is my attempt to fix
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68297. The resulting patch
> is a little bit long because I had to split <exception> and cxxabi.h

"A little bit", yes ;-)

A changelog would help review it, because it's not clear what has
moved to where. You've split files, but not said what parts end up
where (which obviously can be seen by the patch, but it would be
easier with a summary in ChangeLog form).

> include files. The former had to be split due to circular dependency
> that formed after including <typeinfo> in exception_ptr.h and the later
> is because of inability to include cxxabi.h in exception_ptr.h because
> it makes libstdc++/30586 to reappear again.


> diff --git a/libstdc++-v3/libsupc++/eh_throw.cc b/libstdc++-v3/libsupc++/eh_throw.cc
> index 9aac218..b368f8a 100644
> --- a/libstdc++-v3/libsupc++/eh_throw.cc
> +++ b/libstdc++-v3/libsupc++/eh_throw.cc
> @@ -55,6 +55,22 @@ __gxx_exception_cleanup (_Unwind_Reason_Code code, _Unwind_Exception *exc)
> #endif
> }
>
> +extern "C" __cxa_refcounted_exception*
> +__cxxabiv1::__cxa_init_primary_exception(void *obj, const std::type_info *tinfo,
> +                                         void (_GLIBCXX_CDTOR_CALLABI *dest) (void *))
> +{
> +  __cxa_refcounted_exception *header
> +    = __get_refcounted_exception_header_from_obj (obj);
> +  header->referenceCount = 0;
> +  header->exc.exceptionType = tinfo;
> +  header->exc.exceptionDestructor = dest;
> +  header->exc.unexpectedHandler = std::get_unexpected ();
> +  header->exc.terminateHandler = std::get_terminate ();
> +  __GXX_INIT_PRIMARY_EXCEPTION_CLASS(header->exc.unwindHeader.exception_class);
> +  header->exc.unwindHeader.exception_cleanup = __gxx_exception_cleanup;
> +
> +  return header;
> +}

I'd like to see any additions like this function discussed on the C++
ABI list, so we at least have an idea whether other vendors would
consider implementing it too.



> extern "C" void
> __cxxabiv1::__cxa_throw (void *obj, std::type_info *tinfo,
> @@ -64,17 +80,10 @@ __cxxabiv1::__cxa_throw (void *obj, std::type_info *tinfo,
>
>   __cxa_eh_globals *globals = __cxa_get_globals ();
>   globals->uncaughtExceptions += 1;
> -
>   // Definitely a primary.
> -  __cxa_refcounted_exception *header
> -    = __get_refcounted_exception_header_from_obj (obj);
> +  __cxa_refcounted_exception *header =
> +    __cxa_init_primary_exception(obj, tinfo, dest);
>   header->referenceCount = 1;
> -  header->exc.exceptionType = tinfo;
> -  header->exc.exceptionDestructor = dest;
> -  header->exc.unexpectedHandler = std::get_unexpected ();
> -  header->exc.terminateHandler = std::get_terminate ();
> -  __GXX_INIT_PRIMARY_EXCEPTION_CLASS(header->exc.unwindHeader.exception_class);
> -  header->exc.unwindHeader.exception_cleanup = __gxx_exception_cleanup;
>
> #ifdef __USING_SJLJ_EXCEPTIONS__
>   _Unwind_SjLj_RaiseException (&header->exc.unwindHeader);
> diff --git a/libstdc++-v3/libsupc++/exception b/libstdc++-v3/libsupc++/exception
> index 63631f6..6f8b596 100644
> --- a/libstdc++-v3/libsupc++/exception
> +++ b/libstdc++-v3/libsupc++/exception
> @@ -34,135 +34,7 @@
>
> #pragma GCC visibility push(default)
>
> -#include <bits/c++config.h>
> -#include <bits/atomic_lockfree_defines.h>
> -
> -extern "C++" {
> -
> -namespace std
> -{
> -  /**
> -   * @defgroup exceptions Exceptions
> -   * @ingroup diagnostics
> -   *
> -   * Classes and functions for reporting errors via exception classes.
> -   * @{
> -   */
> -
> -  /**
> -   *  @brief Base class for all library exceptions.
> -   *
> -   *  This is the base class for all exceptions thrown by the standard
> -   *  library, and by certain language expressions.  You are free to derive
> -   *  your own %exception classes, or use a different hierarchy, or to
> -   *  throw non-class data (e.g., fundamental types).
> -   */
> -  class exception
> -  {
> -  public:
> -    exception() _GLIBCXX_USE_NOEXCEPT { }
> -    virtual ~exception() _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT;
> -
> -    /** Returns a C-style character string describing the general cause
> -     *  of the current error.  */
> -    virtual const char*
> -    what() const _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT;
> -  };
> -
> -  /** If an %exception is thrown which is not listed in a function's
> -   *  %exception specification, one of these may be thrown.  */
> -  class bad_exception : public exception
> -  {
> -  public:
> -    bad_exception() _GLIBCXX_USE_NOEXCEPT { }
> -
> -    // This declaration is not useless:
> -    // http://gcc.gnu.org/onlinedocs/gcc-3.0.2/gcc_6.html#SEC118
> -    virtual ~bad_exception() _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT;
> -
> -    // See comment in eh_exception.cc.
> -    virtual const char*
> -    what() const _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT;
> -  };


Does bad_exception need to move to <bits/exception.h> ?


> -  /// If you write a replacement %terminate handler, it must be of this type.
> -  typedef void (*terminate_handler) ();
> -
> -  /// If you write a replacement %unexpected handler, it must be of this type.
> -  typedef void (*unexpected_handler) ();

These typedefs are certainly needed in <bits/exception.h> because
unwind-cxx.h uses them, but ...


> -
> -  /// Takes a new handler function as an argument, returns the old function.
> -  terminate_handler set_terminate(terminate_handler) _GLIBCXX_USE_NOEXCEPT;
> -
> -#if __cplusplus >= 201103L
> -  /// Return the current terminate handler.
> -  terminate_handler get_terminate() noexcept;
> -#endif
> -
> -  /** The runtime will call this function if %exception handling must be
> -   *  abandoned for any reason.  It can also be called by the user.  */
> -  void terminate() _GLIBCXX_USE_NOEXCEPT __attribute__ ((__noreturn__));
> -
> -  /// Takes a new handler function as an argument, returns the old function.
> -  unexpected_handler set_unexpected(unexpected_handler) _GLIBCXX_USE_NOEXCEPT;
> -
> -#if __cplusplus >= 201103L
> -  /// Return the current unexpected handler.
> -  unexpected_handler get_unexpected() noexcept;
> -#endif
> -
> -  /** The runtime will call this function if an %exception is thrown which
> -   *  violates the function's %exception specification.  */
> -  void unexpected() __attribute__ ((__noreturn__));
> -
> -  /** [18.6.4]/1:  'Returns true after completing evaluation of a
> -   *  throw-expression until either completing initialization of the
> -   *  exception-declaration in the matching handler or entering @c unexpected()
> -   *  due to the throw; or after entering @c terminate() for any reason
> -   *  other than an explicit call to @c terminate().  [Note: This includes
> -   *  stack unwinding [15.2].  end note]'
> -   *
> -   *  2: 'When @c uncaught_exception() is true, throwing an
> -   *  %exception can result in a call of @c terminate()
> -   *  (15.5.1).'
> -   */
> -  bool uncaught_exception() _GLIBCXX_USE_NOEXCEPT __attribute__ ((__pure__));
> -
> -#if __cplusplus > 201402L || !defined(__STRICT_ANSI__) // c++1z or gnu++98
> -#define __cpp_lib_uncaught_exceptions 201411
> -  /// The number of uncaught exceptions.
> -  int uncaught_exceptions() _GLIBCXX_USE_NOEXCEPT __attribute__ ((__pure__));
> -#endif

None of these seem to be needed in <bits/exception.h>, is that right?

If we're splitting <exception> so that a smaller piece of it can be
used elsewhere without the entire file, then lets make that smaller
piece *only* contain what's needed elsewhere.

There's no need to move things out of <exception> if they aren't
needed by the files you are changing to include <bits/exception.h>.

> -
> -  // @} group exceptions
> -} // namespace std
> -
> -namespace __gnu_cxx
> -{
> -_GLIBCXX_BEGIN_NAMESPACE_VERSION
> -
> -  /**
> -   *  @brief A replacement for the standard terminate_handler which
> -   *  prints more information about the terminating exception (if any)
> -   *  on stderr.
> -   *
> -   *  @ingroup exceptions
> -   *
> -   *  Call
> -   *   @code
> -   *     std::set_terminate(__gnu_cxx::__verbose_terminate_handler)
> -   *   @endcode
> -   *  to use.  For more info, see
> -   *  http://gcc.gnu.org/onlinedocs/libstdc++/manual/bk01pt02ch06s02.html
> -   *
> -   *  In 3.4 and later, this is on by default.
> -   */
> -  void __verbose_terminate_handler();

Likewise, does this need to move to <bits/exception.h> ?

It's not needed by the ABI headers, only by <bits/basic_ios.h> and the
.cc files in libsupc++.

> diff --git a/libstdc++-v3/libsupc++/exception_ptr.h b/libstdc++-v3/libsupc++/exception_ptr.h
> index 783e539..6127b00 100644
> --- a/libstdc++-v3/libsupc++/exception_ptr.h
> +++ b/libstdc++-v3/libsupc++/exception_ptr.h
> @@ -35,6 +35,9 @@
>
> #include <bits/c++config.h>
> #include <bits/exception_defines.h>
> +#include <typeinfo>
> +#include <new>
> +#include <bits/cxxabiv1.h>
>
> #if ATOMIC_INT_LOCK_FREE < 2
> #  error This platform does not support exception propagation.
> @@ -62,6 +65,8 @@ namespace std
>    *  value.
>    */
>   exception_ptr current_exception() _GLIBCXX_USE_NOEXCEPT;
> +  template<typename _Ex>
> +  exception_ptr make_exception_ptr(_Ex) _GLIBCXX_USE_NOEXCEPT;
>
>   /// Throw the object pointed to by the exception_ptr.
>   void rethrow_exception(exception_ptr) __attribute__ ((__noreturn__));
> @@ -87,6 +92,8 @@ namespace std
>
>       friend exception_ptr std::current_exception() _GLIBCXX_USE_NOEXCEPT;
>       friend void std::rethrow_exception(exception_ptr);
> +      template<typename _Ex>
> +      friend exception_ptr std::make_exception_ptr(_Ex) _GLIBCXX_USE_NOEXCEPT;
>
>     public:
>       exception_ptr() _GLIBCXX_USE_NOEXCEPT;
> @@ -162,8 +169,12 @@ namespace std
>     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.

This function should be declared 'inline' too.


> +  } // 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.

> +          (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 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.

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?