[Rcpp-devel] RCPP_USE_UNWIND_PROTECT by default

[Dirk Eddelbuettel]

On 6 February 2022 at 17:40, Jeroen Ooms wrote:
| We can try to take V8 out of the equation, and see what actually
| causes the change. V8 uses (and tests!) the Rcpp feature to call an R
| function from C++. This behaves quite differently when using
| RCPP_UNWIND_PROTECT.
| 
| Here is a dummy package to demonstrate this: https://github.com/jeroen/uptest
| 
| The use case is simple: we want to call some R function from C++, and
| if it fails, catch the error message and deal with it in C++. I
| suspect there are more packages doing this, but perhaps they are not
| testing this case.
| 
| The example from uptest above show that when we compile with
| RCPP_UNWIND_PROTECT, any R error is always printed directly to the
| console. Even if we catch the Rcpp::LongjumpException in C++, the R
| error still seems to bubble up (as Iñaki also noted). I think this is
| at least surprising.
| 
| Then I am not sure what we do in C++ now with this
| Rcpp::LongjumpException if we catch it. We certainly don't want to
| unwind all the way in the middle of running the javascript code. The
| JavaScript code should be able to catch the R error, and continue
| running the script.
| 
| Anyway if you want to make RCPP_UNWIND_PROTECT the default, I can
| obviously opt-out in V8, but as an Rcpp user I am not convinced this
| is an improvement.

Thanks for the feedback. I think there is an easy-to-understand, easy-to-make
error in a sample package (but kudos for creating one!).  Because Rcpp will
always wrap try/catch around the function you create with its mechanism, the
layer inside is redundant.  So I suggest the C++ function file becomes this
simpler / shorter variant:

  #include <Rcpp.h>
  using namespace Rcpp;

  // [[Rcpp::export]]
  Rcpp::NumericVector call_r_from_rcpp() {
    Rcpp::Function callback = Rcpp::Environment::namespace_env("uptest")["callback"];
    callback();
    return Rcpp::NumericVector::create(42);
  }

which behaves fine for me in both cases.

First, "as I found the repo" so with the new RCPP_UNWIND_PROTECT added:

  edd at rob:/tmp/uptest(master)$ install.r .                  # the usual wrapper from littler
  * installing *source* package ‘uptest’ ...
  ** using staged installation
  ** libs                                                                                                                                                                                                                                                                                                                                      
  rm -f uptest.so RcppExports.o callback.o                                                                                                                                                                             
  ccache g++-11 -I"/usr/share/R/include" -DNDEBUG -DRCPP_USE_UNWIND_PROTECT -I'/usr/local/lib/R/site-library/Rcpp/include'    -fpic  -g -O3 -Wall -pipe -pedantic  -c RcppExports.cpp -o RcppExports.o
  ccache g++-11 -I"/usr/share/R/include" -DNDEBUG -DRCPP_USE_UNWIND_PROTECT -I'/usr/local/lib/R/site-library/Rcpp/include'    -fpic  -g -O3 -Wall -pipe -pedantic  -c callback.cpp -o callback.o
  ccache g++-11 -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -Wl,-z,relro -o uptest.so RcppExports.o callback.o -L/usr/lib/R/lib -lR
  installing to /usr/local/lib/R/site-library/00LOCK-uptest/00new/uptest/libs                                                                                                                                                                              
  ** R                                                                                                     
  ** byte-compile and prepare package for lazy loading 
  ** help
  *** installing help indices
  ** building package indices
  ** testing if installed package can be loaded from temporary location
  ** checking absolute paths in shared objects and dynamic libraries
  ** testing if installed package can be loaded from final location
  ** testing if installed package keeps a record of temporary installation path
  * DONE (uptest)
  edd at rob:/tmp/uptest(master)$ R -q
  > library(uptest)
  > uptest()
  Error in (function ()  : Ouch from R
  > qn
  edd at rob:/tmp/uptest(master)$

Second, after editing src/Makevars and removing of object files:


  edd at rob:/tmp/uptest(master)$ be src/Makevars                      # an alias for emacsclient inside byobu
  edd at rob:/tmp/uptest(master)$ rm src/*.o src/*.so
  edd at rob:/tmp/uptest(master)$ install.r .
  * installing *source* package ‘uptest’ ...
  ** using staged installation
  ** libs
  rm -f uptest.so RcppExports.o callback.o
  ccache g++-11 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/Rcpp/include'    -fpic  -g -O3 -Wall -pipe -pedantic  -c RcppExports.cpp -o RcppExports.o
  ccache g++-11 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/Rcpp/include'    -fpic  -g -O3 -Wall -pipe -pedantic  -c callback.cpp -o callback.o
  ccache g++-11 -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -Wl,-z,relro -o uptest.so RcppExports.o callback.o -L/usr/lib/R/lib -lR
  installing to /usr/local/lib/R/site-library/00LOCK-uptest/00new/uptest/libs
  ** R
  ** byte-compile and prepare package for lazy loading
  ** help
  *** installing help indices
  ** building package indices
  ** testing if installed package can be loaded from temporary location
  ** checking absolute paths in shared objects and dynamic libraries
  ** testing if installed package can be loaded from final location
  ** testing if installed package keeps a record of temporary installation path
  * DONE (uptest)
  edd at rob:/tmp/uptest(master)$ R -q
  > library(uptest)
  > uptest()
  Error in call_r_from_rcpp() : Evaluation error: Ouch from R.
  > qn
  edd at rob:/tmp/uptest(master)$ 

So in short

  Rcpp on CRAN:      Error in call_r_from_rcpp() : Evaluation error: Ouch from R.

  Rcpp proposed:     Error in (function ()  : Ouch from R

which thanks to the hard work by Lionel a while back is both faster and nicer.

It's Sunday and I only had one pot of coffee so far so it is plausible and
possible I omitted something, but it looks like this is actually net better.

Cheers, Dirk

-- 
https://dirk.eddelbuettel.com | @eddelbuettel | edd at debian.org