On Wed, Aug 03, 2016 at 10:48:27AM +0100, Jonathan Wakely wrote:
> 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).
>
Will do for next submission.
> > 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> ?
>
>
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.
> > - /// 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>.
>
OK. I'll try to do that.
> > -
> > - // @} 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.
>
OK.
> This function should be declared 'inline' too.
>
We take a pointer to it. How can it be 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?
>
> > + (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;
}
}
> 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.
