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

[Gleb Natapov <gleb at scylladb dot com>]

On Thu, Aug 04, 2016 at 07:01:45PM +0100, Jonathan Wakely wrote:
> On 04/08/16 20:01 +0300, Gleb Natapov wrote:
> > Instead of throwing an exception allocate its memory and initialize it
> > explicitly. Makes std::make_exception_ptr more efficient since no stack
> > unwinding is needed.
> > 
> > In this version I hopefully addressed all Jonathan comments.
> > 
> > * libsupc++/exception (std::exception): Move...
> > * libsupc++/exception.h: ...here; New.
> > * libsupc++/cxxabi.h (__cxa_allocate_exception): Move...
> > * libsupc++/cxxabi_init_exception.h: ...here and add
> > __cxa_init_primary_exception; New.
> > * config/abi/pre/gnu-versioned-namespace.ver: add
> > __cxa_init_primary_exception and std::exception_ptr(void*)
> > * config/abi/pre/gnu.ver (CXXABI_1.3.11) : add
> > __cxa_init_primary_exception and std::exception_ptr(void*)
> > (CXXABI_1.3.11): New.
> > * include/Makefile.am: add exception.h and cxxabi_init_exception.h
> > * include/Makefile.in: Likewise.
> > * libsupc++/Makefile.am: add exception.h and cxxabi_init_exception.h
> > * libsupc++/Makefile.in: Likewise.
> > * libsupc++/eh_throw.cc(__cxa_throw): add __cxa_init_primary_exception
> > and use it
> > * libsupc++/exception_ptr.h(std::make_exception_ptr): use
> > __cxa_allocate_exception and __cxa_init_primary_exception to create
> > exception pointer
> > * libsupc++/typeinfo: include bits/exception.h instead of exception
> 
> This version is *much* easier to review :-)
> 
> > +namespace __cxxabiv1
> > +{
> > +  struct __cxa_refcounted_exception;
> > +
> > +  extern "C"
> > +    {
> > +      // Allocate memory for the primary exception plus the thrown object.
> > +      void*
> > +        __cxa_allocate_exception(size_t) _GLIBCXX_NOTHROW;
> 
> Please align __cxa_allocate_exception with the return type on the
> previous line.
> 
> > +
> > +      // Initialize exception
> 
> Please add "(this is a GNU extension)" to the comment.
> 
> > +      __cxa_refcounted_exception*
> > +      __cxa_init_primary_exception(void *object, std::type_info *tinfo,
> > +                void (_GLIBCXX_CDTOR_CALLABI *dest) (void *)) _GLIBCXX_NOTHROW;
> > +
> > +    }
> > +} // namespace __cxxabiv1
> > +
> > +#endif
> > +
> > +#pragma GCC visibility pop
> > +
> > +#endif // _CXXABI_INIT_EXCEPTION_H
> > index 63631f6..8be903b 100644
> > --- a/libstdc++-v3/libsupc++/exception
> > +++ b/libstdc++-v3/libsupc++/exception
> > @@ -36,39 +36,12 @@
> > 
> > #include <bits/c++config.h>
> > #include <bits/atomic_lockfree_defines.h>
> > +#include <bits/exception.h>
> > 
> > extern "C++" {
> > 
> > namespace std
> > {
> > -  /**
> > -   * @defgroup exceptions Exceptions
> > -   * @ingroup diagnostics
> > -   *
> > -   * Classes and functions for reporting errors via exception classes.
> > -   * @{
> > -   */
> 
> The Doxygen group that starts with @{ ends later in this file, but you
> haven't moved the corresponding @} to the new file.
> 
> I can take care of fixing that up though (we want the contents of the
> new file and this one to all be in the group).
> 
Leave it as is in the next submission? I am not too familiar with
Doxygen.

> 
> > @@ -162,8 +169,12 @@ namespace std
> >     swap(exception_ptr& __lhs, exception_ptr& __rhs)
> >     { __lhs.swap(__rhs); }
> > 
> > -  } // namespace __exception_ptr
> > +    template<typename _Ex>
> > +      static inline void
> > +      __dest_thunk(void* x)
> > +      { reinterpret_cast<_Ex*>(x)->~_Ex(); }
> 
> This is still 'static' so we get a separate instantiation in every
> translation unit that uses make_exception_ptr. It should non 'inline'
> but not 'static'.
> 
> Looking at it again now, is there any reason to use reinterpret_cast
> rather than static_cast? I think they're identical in this context,
> and I prefer not to only use reinterpret_cast as a last resort.
> 
static_cast should work just fine.

> Other than that, I think this is good to commit.
> 
> I can't think of any way to test the changes beyond the existing tests
> for exception_ptr, so we don't need new test files.
> 
> 

--
			Gleb.