Potential leakage with exceptions

[romainfrancois]

While looking at tidyverse/dplyr#1062, I need up examining yet another time the whole exception business in Rcpp. I think there is a problem.

#include <Rcpp.h>
using namespace Rcpp ;

// [[Rcpp::export]]
void foo(){
        stop( "foo %s", "bar" ) ;
}

/*** R
        foo()

*/

When I sourceCpp this through valgrind, I get:

> Rcpp::sourceCpp( "/tmp/test.cpp")

> foo()
Error in eval(expr, envir, enclos) : foo bar
Calls: <Anonymous> ... source -> withVisible -> eval -> eval -> foo -> <Anonymous>
Execution halted
==68583==
==68583== HEAP SUMMARY:
==68583==     in use at exit: 33,147,961 bytes in 15,982 blocks
==68583==   total heap usage: 41,762 allocs, 25,780 frees, 67,956,199 bytes allocated
==68583==
==68583== 32 bytes in 1 blocks are possibly lost in loss record 21 of 1,770
==68583==    at 0x4C2B0E0: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==68583==    by 0xC9563B8: std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)
==68583==    by 0xC957AE0: char* std::string::_S_construct<char const*>(char const*, char const*, std::allocator<char> const&, std::forward_iterator_tag) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)
==68583==    by 0xC957EF7: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)
==68583==    by 0xD5BA785: Rcpp::exception::exception(char const*) (exceptions.h:31)
==68583==    by 0xD5BC5F7: void Rcpp::stop<char [4]>(char const*, char const (&) [4]) (exceptions.h:256)
==68583==    by 0xD5B9EA6: foo() (test.cpp:6)
==68583==    by 0xD5B9ED9: sourceCpp_34287_foo (test.cpp:21)
==68583==    by 0x4EC84F4: ??? (in /usr/lib/R/lib/libR.so)
==68583==    by 0x4F08055: Rf_eval (in /usr/lib/R/lib/libR.so)
==68583==    by 0x4F0B631: ??? (in /usr/lib/R/lib/libR.so)
==68583==    by 0x4F07F30: Rf_eval (in /usr/lib/R/lib/libR.so)
==68583==
==68583== 144 bytes in 1 blocks are possibly lost in loss record 37 of 1,770
==68583==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==68583==    by 0xC8F94E2: __cxa_allocate_exception (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)
==68583==    by 0xD5BC5D7: void Rcpp::stop<char [4]>(char const*, char const (&) [4]) (exceptions.h:256)
==68583==    by 0xD5B9EA6: foo() (test.cpp:6)
==68583==    by 0xD5B9ED9: sourceCpp_34287_foo (test.cpp:21)
==68583==    by 0x4EC84F4: ??? (in /usr/lib/R/lib/libR.so)
==68583==    by 0x4F08055: Rf_eval (in /usr/lib/R/lib/libR.so)
==68583==    by 0x4F0B631: ??? (in /usr/lib/R/lib/libR.so)
==68583==    by 0x4F07F30: Rf_eval (in /usr/lib/R/lib/libR.so)
==68583==    by 0x4F09144: Rf_applyClosure (in /usr/lib/R/lib/libR.so)
==68583==    by 0x4F07C0D: Rf_eval (in /usr/lib/R/lib/libR.so)
==68583==    by 0x4F0C4F3: ??? (in /usr/lib/R/lib/libR.so)
==68583==
==68583== LEAK SUMMARY:
==68583==    definitely lost: 0 bytes in 0 blocks
==68583==    indirectly lost: 0 bytes in 0 blocks
==68583==      possibly lost: 176 bytes in 2 blocks
==68583==    still reachable: 33,147,785 bytes in 15,980 blocks
==68583==         suppressed: 0 bytes in 0 blocks
==68583== Reachable blocks (those to which a pointer was found) are not shown.
==68583== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==68583==
==68583== For counts of detected and suppressed errors, rerun with: -v
==68583== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

I believe that the problem is that the destructor for the exception is never called because of jumping. i.e. stop generates an Rcpp::exception:

class exception : public std::exception {
    public:
        explicit exception(const std::string& message_) : message(message_){}
        exception(const std::string& message_, const char* file, int line ) : message(message_){
            rcpp_set_stack_trace( stack_trace(file,line) ) ;
        }
        virtual ~exception() throw(){}
        virtual const char* what() const throw() {
            return message.c_str() ;
        }
    private:
        std::string message ;
    } ;

that is caught though END_RCPP :

#ifndef VOID_END_RCPP
#define VOID_END_RCPP                                                          \
  }                                                                            \
  catch (Rcpp::internal::InterruptedException &__ex__) {                       \
    Rf_onintr();                                                               \
  }                                                                            \
  catch (std::exception &__ex__) {                                             \
    forward_exception_to_r(__ex__);                                            \
  }                                                                            \
  catch (...) {                                                                \
    ::Rf_error("c++ exception (unknown reason)");                              \
  }
#endif

#ifndef END_RCPP
#define END_RCPP VOID_END_RCPP return R_NilValue;
#endif

and then in forward_exception_to_r we end up jumping with a callback to R's stop :

inline void forward_exception_to_r( const std::exception& ex){
    SEXP stop_sym  = Rf_install( "stop" ) ;
    Rcpp::Shield<SEXP> condition( exception_to_r_condition(ex) );
    Rcpp::Shield<SEXP> expr( Rf_lang2( stop_sym , condition ) ) ;
    Rf_eval( expr, R_GlobalEnv ) ;
}

The problem here is that the exception is never destructed, and therefore the std::string message it holds is not destructed either, hence the complaints from valgrind.

See this simpler example:

#include <Rcpp.h>
using namespace Rcpp ;


struct dummy{
        dummy(int value_) : value(value_){}

        ~dummy(){
                Rprintf( "~dummy" ) ;
        }
        int value ;
} ;

struct dummy_exception : public std::exception {
        dummy_exception(int value) : exception(), x(value) {}

        ~dummy_exception() throw(){}

        dummy x ;
} ;


// [[Rcpp::export]]
void foo(){
        throw dummy_exception(3) ;
}

/*** R
        foo()

*/

The point here is that I want to see ~dummy, and I don't:

$ R --vanilla < test.R

R version 3.1.3 (2015-03-09) -- "Smooth Sidewalk"
Copyright (C) 2015 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> Rcpp::sourceCpp( "/tmp/test.cpp")

> foo()
Error in eval(expr, envir, enclos) : std::exception
Calls: <Anonymous> ... source -> withVisible -> eval -> eval -> foo -> <Anonymous>
Execution halted

whereas if I handle the try-catch manually without then calling R's stop the exception is properly destructed:

// [[Rcpp::export]]
void foo(){
        try{
                throw dummy_exception(3) ;
        } catch( const std::exception& ex ){

        }
}

with this I get:

$ R --vanilla < test.R

R version 3.1.3 (2015-03-09) -- "Smooth Sidewalk"
Copyright (C) 2015 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> Rcpp::sourceCpp( "/tmp/test.cpp")

> foo()
~dummy>
>

Now leaking a std::string might not be a big deal, but it's definitely real.

[romainfrancois]

An easy solution is to not call stop in forward_exception_to_r but instead construct some object to return to the R side containing the information from the exception, and then call stop on the R side if necessary.

Another solution, perhaps less disruptive is to use a global std::string instead of one within the exception. That might be enough as there is only ever one Rcpp::exception at a time I guess.

[romainfrancois]

I've tried the easy solution, i.e. the second one I mention, I get this now:

> foo()
Error in eval(expr, envir, enclos) : foo bar
Calls: <Anonymous> ... source -> withVisible -> eval -> eval -> foo -> <Anonymous>
Execution halted
==69415==
==69415== HEAP SUMMARY:
==69415==     in use at exit: 33,149,903 bytes in 15,978 blocks
==69415==   total heap usage: 41,777 allocs, 25,799 frees, 67,965,619 bytes allocated
==69415==
==69415== 136 bytes in 1 blocks are possibly lost in loss record 36 of 1,765
==69415==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==69415==    by 0xC8F94E2: __cxa_allocate_exception (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)
==69415==    by 0xD5BC51E: void Rcpp::stop<char [4]>(char const*, char const (&) [4]) (exceptions.h:259)
==69415==    by 0xD5B9EA6: foo() (test.cpp:7)
==69415==    by 0xD5B9ED9: sourceCpp_21010_foo (test.cpp:22)
==69415==    by 0x4EC84F4: ??? (in /usr/lib/R/lib/libR.so)
==69415==    by 0x4F08055: Rf_eval (in /usr/lib/R/lib/libR.so)
==69415==    by 0x4F0B631: ??? (in /usr/lib/R/lib/libR.so)
==69415==    by 0x4F07F30: Rf_eval (in /usr/lib/R/lib/libR.so)
==69415==    by 0x4F09144: Rf_applyClosure (in /usr/lib/R/lib/libR.so)
==69415==    by 0x4F07C0D: Rf_eval (in /usr/lib/R/lib/libR.so)
==69415==    by 0x4F0C4F3: ??? (in /usr/lib/R/lib/libR.so)
==69415==
==69415== LEAK SUMMARY:
==69415==    definitely lost: 0 bytes in 0 blocks
==69415==    indirectly lost: 0 bytes in 0 blocks
==69415==      possibly lost: 136 bytes in 1 blocks
==69415==    still reachable: 33,149,767 bytes in 15,977 blocks
==69415==         suppressed: 0 bytes in 0 blocks
==69415== Reachable blocks (those to which a pointer was found) are not shown.
==69415== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==69415==
==69415== For counts of detected and suppressed errors, rerun with: -v
==69415== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

that's an improvement as the std::string does no longer have to be destructed, so it does not leak, but still no picnic, as std::exception seems to have some data of its own that does not correctly get handled because the destructor is not called:

==69415==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==69415==    by 0xC8F94E2: __cxa_allocate_exception (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)

the exception allocates memory with __cxa_allocate_exception and I guess could free this memory with __cxa_free_exception, at least with libc++ see http://libcxxabi.llvm.org/spec.html.

Now poking around with __cxa_free_exception and equivalent ways to manually free memory for exceptions in other platforms could I guess do the trick. I'd rather not mess with this and use the first solution I mention above.

[romainfrancois]

Tried to manually delete the exception, but that does not work:

> Rcpp::sourceCpp( "/tmp/test.cpp")

> foo()
==69519== Invalid free() / delete / delete[] / realloc()
==69519==    at 0x4C2C2BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==69519==    by 0xD5BB341: forward_exception_to_r(std::exception const&) (exceptions.h:312)
==69519==    by 0xD5B9F9B: sourceCpp_58809_foo (test.cpp:24)
==69519==    by 0x4EC84F4: ??? (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F08055: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F0B631: ??? (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F07F30: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F09144: Rf_applyClosure (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F07C0D: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F0C4F3: ??? (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F001DC: ??? (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F07C4F: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==  Address 0x9a36380 is 128 bytes inside a block of size 136 alloc'd
==69519==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==69519==    by 0xC8F94E2: __cxa_allocate_exception (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)
==69519==    by 0xD5BC52E: void Rcpp::stop<char [4]>(char const*, char const (&) [4]) (exceptions.h:259)
==69519==    by 0xD5B9EA6: foo() (test.cpp:7)
==69519==    by 0xD5B9ED9: sourceCpp_58809_foo (test.cpp:22)
==69519==    by 0x4EC84F4: ??? (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F08055: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F0B631: ??? (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F07F30: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F09144: Rf_applyClosure (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F07C0D: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F0C4F3: ??? (in /usr/lib/R/lib/libR.so)
==69519==
Error in eval(expr, envir, enclos) : foo bar
Calls: <Anonymous> ... source -> withVisible -> eval -> eval -> foo -> <Anonymous>
Execution halted
==69519==
==69519== HEAP SUMMARY:
==69519==     in use at exit: 33,149,903 bytes in 15,978 blocks
==69519==   total heap usage: 41,777 allocs, 25,800 frees, 67,965,619 bytes allocated
==69519==
==69519== 136 bytes in 1 blocks are possibly lost in loss record 36 of 1,765
==69519==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==69519==    by 0xC8F94E2: __cxa_allocate_exception (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)
==69519==    by 0xD5BC52E: void Rcpp::stop<char [4]>(char const*, char const (&) [4]) (exceptions.h:259)
==69519==    by 0xD5B9EA6: foo() (test.cpp:7)
==69519==    by 0xD5B9ED9: sourceCpp_58809_foo (test.cpp:22)
==69519==    by 0x4EC84F4: ??? (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F08055: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F0B631: ??? (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F07F30: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F09144: Rf_applyClosure (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F07C0D: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519==    by 0x4F0C4F3: ??? (in /usr/lib/R/lib/libR.so)
==69519==
==69519== LEAK SUMMARY:
==69519==    definitely lost: 0 bytes in 0 blocks
==69519==    indirectly lost: 0 bytes in 0 blocks
==69519==      possibly lost: 136 bytes in 1 blocks
==69519==    still reachable: 33,149,767 bytes in 15,977 blocks
==69519==         suppressed: 0 bytes in 0 blocks
==69519== Reachable blocks (those to which a pointer was found) are not shown.
==69519== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==69519==
==69519== For counts of detected and suppressed errors, rerun with: -v
==69519== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

[jjallaire]

We had a PR to address this a while back:

#225

We didn't take it or pursue it further mostly because it was mucking around
with such low level stuff (risky and requiring a lot of careful
testing/debugging) and the gain seemed not huge (prevent leaks on stop
which should be exceptional in any case).

All of that said, I agree we are doing this "wrong" now and it should
cleaned up, just need to find the time to make the changes and confirm they
haven't borked something else up :-)

On Fri, Apr 10, 2015 at 4:31 AM, Romain François notifications@github.com
wrote:

Tried to manually delete the exception, but that does not work:

Rcpp::sourceCpp( "/tmp/test.cpp")

foo()
==69519== Invalid free() / delete / delete[] / realloc()
==69519== at 0x4C2C2BC: operator delete(void_) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==69519== by 0xD5BB341: forward_exception_to_r(std::exception const&) (exceptions.h:312)
==69519== by 0xD5B9F9B: sourceCpp_58809_foo (test.cpp:24)
==69519== by 0x4EC84F4: ??? (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F08055: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F0B631: ??? (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F07F30: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F09144: Rf_applyClosure (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F07C0D: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F0C4F3: ??? (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F001DC: ??? (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F07C4F: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== Address 0x9a36380 is 128 bytes inside a block of size 136 alloc'd
==69519== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==69519== by 0xC8F94E2: _cxa_allocate_exception (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)
==69519== by 0xD5BC52E: void Rcpp::stop<char [4]>(char const, char const (&) [4]) (exceptions.h:259)
==69519== by 0xD5B9EA6: foo() (test.cpp:7)
==69519== by 0xD5B9ED9: sourceCpp_58809_foo (test.cpp:22)
==69519== by 0x4EC84F4: ??? (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F08055: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F0B631: ??? (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F07F30: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F09144: Rf_applyClosure (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F07C0D: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F0C4F3: ??? (in /usr/lib/R/lib/libR.so)
==69519==
Error in eval(expr, envir, enclos) : foo bar
Calls: ... source -> withVisible -> eval -> eval -> foo ->
Execution halted
==69519==
==69519== HEAP SUMMARY:
==69519== in use at exit: 33,149,903 bytes in 15,978 blocks
==69519== total heap usage: 41,777 allocs, 25,800 frees, 67,965,619 bytes allocated
==69519==
==69519== 136 bytes in 1 blocks are possibly lost in loss record 36 of 1,765
==69519== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==69519== by 0xC8F94E2: __cxa_allocate_exception (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.19)
==69519== by 0xD5BC52E: void Rcpp::stop<char [4]>(char const*, char const (&) [4]) (exceptions.h:259)
==69519== by 0xD5B9EA6: foo() (test.cpp:7)
==69519== by 0xD5B9ED9: sourceCpp_58809_foo (test.cpp:22)
==69519== by 0x4EC84F4: ??? (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F08055: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F0B631: ??? (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F07F30: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F09144: Rf_applyClosure (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F07C0D: Rf_eval (in /usr/lib/R/lib/libR.so)
==69519== by 0x4F0C4F3: ??? (in /usr/lib/R/lib/libR.so)
==69519==
==69519== LEAK SUMMARY:
==69519== definitely lost: 0 bytes in 0 blocks
==69519== indirectly lost: 0 bytes in 0 blocks
==69519== possibly lost: 136 bytes in 1 blocks
==69519== still reachable: 33,149,767 bytes in 15,977 blocks
==69519== suppressed: 0 bytes in 0 blocks
==69519== Reachable blocks (those to which a pointer was found) are not shown.
==69519== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==69519==
==69519== For counts of detected and suppressed errors, rerun with: -v
==69519== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

—
Reply to this email directly or view it on GitHub
#286 (comment).

[romainfrancois]

Yep, I don't think what I see in this PR is the right way to handle the situation.

I think the data should be sent back to the R side without jumping (i.e. by calling R's stop) and stop should be called on the R side.

In short, this needs changes in:

• code generation from Rcpp::export, mostly on the R side
• not call stop from forward_exception_to_r

I guess the handling of InterruptedException with Rf_onintr would produce the same kind of leakage

[eddelbuettel]

Thanks for all the digging. It does look like we need another round of changes here, and the suggested call of R's stop() may indeed be best approach.

[kevinushey]

Any thoughts on whether something like this would work?

static SEXP exceptionSEXP = NULL;
try {
    < ... throwing code ... >
} catch (std::exception& e) {
    exceptionSEXP = exception_to_r_condition(e);
}

if (exceptionSEXP) <stop(exceptionSEXP)>
<reset(exceptionSEXP)>

(would need to fill in appropriate stop + reset functions, and also protect the SEXP passed back)

[jjallaire]

We have one handy thing already implemented that may make this not too
bad. This macro:

https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/macros/macros.h#L52-L63

Will actually return an SEXP rather than throwing or calling
Rf_onintr. This was put in to prevent throwing across module
boundaries for interfaces(cpp), as this causes crashes on Windows
because the GCC runtime library is static linked.

If we generated END_RCPP_RETURN_ERROR rather than VOID_END_RCPP in
RcppExports.cpp then the generated code in RcppExports.R would simply
need to check the returned SEXP and do the right thing (call stop,
return the value, ec.)

On Fri, Apr 10, 2015 at 12:16 PM, Kevin Ushey notifications@github.com
wrote:

Any thoughts on whether something like this would work?

static SEXP exceptionSEXP = NULL;
try {
< ... throwing code ... >
} catch (std::exception& e) {
exceptionSEXP = exception_to_r_condition(e);
}

if (exceptionSEXP) <stop(exceptionSEXP)>
<reset(exceptionSEXP)>

(would need to fill in appropriate stop + reset functions, and also
protect the SEXP passed back)

—
Reply to this email directly or view it on GitHub
#286 (comment).

[romainfrancois]

@kevinushey I'd rather do it on the R side, especially if on the C++ side we end up calling the R function anyway, as it is the case now.

[kevinushey]

Okay, I think that makes sense as well. I guess that the wrapper functions would be something of the form:

foo <- function(...) {
    result <- .Call(...)
    if (inherits(result, "rcpp-error"))
        stop(result$message)
    result
}

The only downside I can think of is that people who manually write their .Call functions, rather than relying on the attributes-generated wrappers, might be surprised that their function never throws even when they call Rcpp::stop() (since it is now the job of the wrapper to propagate the error) but I guess that could be solved with documentation.

To make things simple, maybe we could generate a function called .Rcpp.Call into RcppExports.R, and nudge people that want to write the .Call interface themselves to use that instead.

[jjallaire]

I think we're still fine because:

1. Attributes has no notion of "write your own .Call functions". The only
   way to do this would be if you delete RcppExports.R after it's generated
   (or just write alternate wrappers). We could probably check CRAN to see if
   this is done but it's not really a documented capability (all of the
   generated code is opaque and by contract not decomposable IMHO).
2. For those currently using BEGIN_RCPP / END_RCPP there would be no
   behavior change (it would still throw). We'd just be changing the generated
   attributes code to use END_RCPP_RETURN_ERROR

On Fri, Apr 10, 2015 at 1:37 PM, Kevin Ushey notifications@github.com
wrote:

Okay, I think that makes sense as well. I guess that the wrapper functions
would be something of the form:

foo <- function(...) {
result <- .Call(...)
if (inherits(result, "rcpp-error"))
stop(result$message)
result
}

The only downside I can think of is that people who manually write their
.Call functions, rather than relying on Rcpp::attributes(), might be
surprised that their function never throws even when they call
Rcpp::stop() (since it is now the job of the wrapper to propagate the
error) but I guess that could be solved with documentation.

To make things simple, maybe we could generate a function called
.Rcpp.call into RcppExports.R, and nudge people that want to write the
.Call interface themselves to use that.

—
Reply to this email directly or view it on GitHub
#286 (comment).

[romainfrancois]

one issue about the current code in END_RCPP_RETURN_ERROR is that some information is lost because of the difference between exception_to_r_condition and string_to_try_error.

inline SEXP exception_to_r_condition( const std::exception& ex){
    std::string ex_class = demangle( typeid(ex).name() ) ;
    std::string ex_msg   = ex.what() ;

    Rcpp::Shield<SEXP> cppstack( rcpp_get_stack_trace() );
    Rcpp::Shield<SEXP> call( get_last_call() );
    Rcpp::Shield<SEXP> classes( get_exception_classes(ex_class) );
    Rcpp::Shield<SEXP> condition( make_condition( ex_msg, call, cppstack, classes) );
    rcpp_set_stack_trace( R_NilValue ) ;
    return condition ;
}

inline SEXP string_to_try_error( const std::string& str){
    using namespace Rcpp;

    Rcpp::Shield<SEXP> simpleErrorExpr( Rf_lang2(::Rf_install("simpleError"), Rf_mkString(str.c_str())) );
    Rcpp::Shield<SEXP> simpleError( Rf_eval(simpleErrorExpr, R_GlobalEnv) );
    Rcpp::Shield<SEXP> tryError( Rf_mkString( str.c_str() ) );
    Rf_setAttrib( tryError, R_ClassSymbol, Rf_mkString("try-error") ) ;
    Rf_setAttrib( tryError, Rf_install( "condition") , simpleError ) ;

    return tryError;
}

This is valuable information about the exception that is not kept by string_to_try_error.

[jjallaire]

I recall us discussing this at the time we implemented
END_RCPP_RETURN_ERROR and at the time it seemed okay only because
END_RCPP_RETURN_ERROR was used rarely/obscurely. Now it does indeed
seem more pressing that we get this right.

On Fri, Apr 10, 2015 at 2:16 PM, Romain François
notifications@github.com wrote:

one issue about the current code in END_RCPP_RETURN_ERROR is that some
information is lost because of the difference between
exception_to_r_condition and string_to_try_error.

inline SEXP exception_to_r_condition( const std::exception& ex){
std::string ex_class = demangle( typeid(ex).name() ) ;
std::string ex_msg = ex.what() ;

Rcpp::Shield<SEXP> cppstack( rcpp_get_stack_trace() );
Rcpp::Shield<SEXP> call( get_last_call() );
Rcpp::Shield<SEXP> classes( get_exception_classes(ex_class) );
Rcpp::Shield<SEXP> condition( make_condition( ex_msg, call, cppstack,

classes) );
rcpp_set_stack_trace( R_NilValue ) ;
return condition ;
}

inline SEXP string_to_try_error( const std::string& str){
using namespace Rcpp;

Rcpp::Shield<SEXP> simpleErrorExpr(

Rf_lang2(::Rf_install("simpleError"), Rf_mkString(str.c_str())) );
Rcpp::Shield simpleError( Rf_eval(simpleErrorExpr, R_GlobalEnv) );
Rcpp::Shield tryError( Rf_mkString( str.c_str() ) );
Rf_setAttrib( tryError, R_ClassSymbol, Rf_mkString("try-error") ) ;
Rf_setAttrib( tryError, Rf_install( "condition") , simpleError ) ;

return tryError;

}

This is valuable information about the exception that is not kept by
string_to_try_error.

—
Reply to this email directly or view it on GitHub.

[romainfrancois]

Looking at this some more. Looking at the number of places BEGIN_RCPP, END_RCPP are being used within Rcpp itself, and given a change like what I suggested earlier would mean forcing regeneration of every single RcppExports.cpp and RcppExports.R I'm not sure anymore.

I think @kevinushey earlier plan could work, something like this:

#define BEGIN_RCPP 
    int rcpp_output_type = 0 ; 
    SEXP rcpp_output_condition = R_NilValue ; 
    try {

#define END_RCPP  
    }
    catch( Rcpp::internal::InterruptedException &__ex__) {
        rcpp_output_type = 1 ;
    } 
    catch( std::exception& __ex__ ){
       rcpp_output_type = 2 ;  
       rcpp_output_condition = PROTECT(exception_to_r_condition(__ex__)) ;
    } catch( ... ){
       rcpp_output_type = 2 ;
       rcpp_output_condition = PROTECT(string_to_try_error("c++ exception (unknown reason)")) ;
    }
    if( rcpp_output_type == 1 ){
       Rf_onintr() ;    
    }
    if( rcpp_output_type == 2 ){
       SEXP stop_sym  = Rf_install( "stop" ) ;
       SEXP expr = PROTECT( Rf_lang2( stop_sym , rcpp_output_condition ) ) ;
       Rf_eval( expr, R_GlobalEnv ) ;
    }
    return R_NilValue ;

The previous system made sure we took care of scoping all the c++ variables, except for the exception itself. This would just delay calling the jumping eval until a point where there are no C++ objects in the stack.

This could work.