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

[Gleb Natapov <gleb at scylladb dot com>]

On Wed, Aug 03, 2016 at 12:47:30PM +0100, Jonathan Wakely wrote:
> > > 
> > > > +  } // 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() { }
> 
Ah, I missed that fact that the code is in user visible include.

> 
> > 
> > > 
> > > > +          (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.
> 
I'll make it terminate like you've suggested then.

> We don't seem to do that even without exception_ptr involved:
> 
Yes, that's the reason current make_exception_ptr behaves as it does,
but to fix your test case below the code that generates code for 'throw'
will have to be fixed.

> #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;
>    }
> }
[skip]
> > > > -  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?
I missed this comment. typeid() returns const std::type_info so I
either need to add const here or cast the const away from  typeid()
return value.

--
			Gleb.