Fix protect-unwind #859
Fix protect-unwind #859
Conversation
…sable it now" This reverts commit 81c8339.
So the exception object can be destructed properly
A LongjumpException might be sent by another package called via cpp interface
Codecov Report
@@ Coverage Diff @@
## master #859 +/- ##
==========================================
- Coverage 90.49% 90.46% -0.03%
==========================================
Files 70 70
Lines 3240 3251 +11
==========================================
+ Hits 2932 2941 +9
- Misses 308 310 +2
Continue to review full report at Codecov.
|
|
Hi @lionel- -- thanks for working on this. Am a little tied up with R/Finance as well as the R 3.5.0 binary transition in Debian so it may take me a day or two or three to get to this. But much appreciated. If you have better / more ideas for additional test beds / platforms I'd be all ears. |
|
No worries. It'd be helpful to have a few additional Travis targets with old R versions, ideally one for each major release supported by Rcpp. Windows CI with Appveyor would be great as well. |
|
Could script something using Rocker and older r-base builds? I am more worried about not-so-sane platforms ie the one starting with win and ending with dows, or our old friend Slowlaris, or ... |
That'd be great. I forgot to check on old R versions and I just encountered a crash with R CMD check on R 3.4. This seems to be the same Edit: R CMD check passes on R 3.4 when the embeddedR test is disabled. |
| namespace Rcpp { | ||
| namespace internal { | ||
|
|
||
| #ifdef RCPP_USE_PROTECT_UNWIND | ||
|
|
||
| // Store the jump buffer as a static variable in function scope | ||
| // because inline variables are a C++17 extension. | ||
| inline std::jmp_buf* get_jmpbuf_ptr() { |
There was a problem hiding this comment.
Is it okay for this to be an inline static? It seems like things could go wrong if there were recursive calls to Rcpp_eval(), e.g. if you were to Rcpp_eval() an expression that itself called R functions that call Rcpp_eval().
I wonder if instead this could be part of the EvalData struct or similar?
There was a problem hiding this comment.
doh, good point! I will write a failing test and fix this, the jumpbuf should indeed be on the stack and we'd pass a pointer as data to the cleanup callback.
There was a problem hiding this comment.
Hi @lionel- -- do you plan to add that test to this PR?
There was a problem hiding this comment.
Awesome, no rush. Was just wondering as there was such a rush here, and then a slowdown.
| struct EvalData { | ||
| SEXP expr; | ||
| SEXP env; | ||
| EvalData(SEXP expr_, SEXP env_) : expr(expr_), env(env_) { } | ||
| }; | ||
|
|
||
| // First jump back to the protected context with a C longjmp because |
There was a problem hiding this comment.
Are the i386 checks performed by R CMD check sufficient? Or should I set up another virtual box with a 32 bit Windows to check that architecture properly?
There was a problem hiding this comment.
Good point, I think that will be sufficient. I also validated this works as expected on my own VM. Nice!
One other thought: do we need to worry about this interfering with other ways that R might jump (e.g. interrupts, browser)? I think we discussed this already and we're pretty sure that we'll do the right thing as long as R_ContinueUnwind() is called but curious if you've already thought through these scenarios.
There was a problem hiding this comment.
do we need to worry about this interfering with other ways that R might jump (e.g. interrupts, browser)?
All sorts of longjumps are covered by protect-unwind.
I've been wondering about longjumps occurring in destructors during a LongjumpException unwind. According to R semantics the last longjump should win but in C++ it's not possible to rethrow from a destructor. I think if C++11's current_exception() always returned a reference to the current exception we could update the token but unfortunately this is implementation-dependent and the exception_ptr might point to a copy.
So I think code in destructors that might cause longjumps should be unwind-protected and guarded with catch(...), and the new longjumps should simply be discarded. The original token contains all the information needed to accomplish the first longjump safely. Too bad that exception handling in C++ is so wonky and we can't do better.
There was a problem hiding this comment.
unfortunately this is implementation-dependent and the exception_ptr might point to a copy.
oh hmm, even if the exception is copied it will still point to the same token SEXP, so we could just memcopy all the RAWSXP data from one token into the other. But then the structure of the token should become part of the API.
It may not be worth worrying about this too much as it's unlikely that complex R code will be run from destructors?
There was a problem hiding this comment.
I think most C++ destructors will not (or at least, should not) contain lots of R code. I imagine most destructors out there that do interact with R from a destructor involve R object lifetime / resource management (e.g. calling R_ReleaseObject(); or maybe cleaning up external pointers or something similar)
I think we don't need to worry about this too much since at least even with this PR we still have a substantial improvement over the status quo; anyone who does run into a problem of this form could likely work around it.
I think we can punt on this issue until we hear of a reported problem in the wild.
There was a problem hiding this comment.
That makes good sense, thanks! We used to have complex R code in a dplyr dtor but that is no longer the case IIRC.
| namespace internal { | ||
|
|
||
| inline SEXP longjumpSentinel(SEXP token) { | ||
| SEXP sentinel = PROTECT(Rf_allocVector(VECSXP, 1)); |
There was a problem hiding this comment.
Just curious why we're using a length-one list rather than a length-one character vector directly?
There was a problem hiding this comment.
The sentinel wraps the longjump token that is passed to R_ContinueUnwind().
There was a problem hiding this comment.
Right! Sorry for the silly question.
|
No issues whatsoever on a full rev.dep check on Linux. Do we need to test this more on other platforms? Any views, @kevinushey @lionel- ? |
I think we should also test with |
|
Last commit implements stack-local jump buffers. @eddelbuettel Could you run the revdeps again with |
|
@eddelbuettel do your revdeps run on 3.4 or 3.5? I noticed today that your Travis still runs on 3.4. |
|
@lionel- For the revdeps I still use 3.4.4. It is a Debian box and we are in the middle of the transition; it mostly uses local CRAN packages but I keep it "stateful" (ie not what your revdepper in devtools does as I don't have all the time in the world to wait for the depends of 1360+ package to get reinstalled each time) and for that I need a rebuild of "all". Haven't had time yet. But will do -- both the new rev.dep, and "eventually" the switch to 3.5.0 That said, you could play now with Rocker images. We have a bunch of 3.* images of base. As Rcpp has no depends itself it should be quick-ish to set up. |
Will all 1400 revdeps build cleanly with these images? Just asking out of curiosity, I think this can wait until your box is updated. |
|
@lionel- That may have been a misunderstanding. For rev.deps, I do "full one-level horizontol mode", ie all direct import/depends/linkingto. Not recursive. Large-ish skip list for unbuildable or otherwise stooopid packages (maybe 60, list is public). But it may help to check "vertical" against other R version but I think it would wholly sufficient to just do a full |
|
LGTM with the most recent commit! |
|
@eddelbuettel I have now run R CMD check on old R versions up until 3.1. Besides the fact that I randomly run into the same problems as on win-builder and r-hub (really long vignette building time, (when I can build those) or the embeddedR crash), they all pass except the 3.1 build which assumes existence of the new |
|
@lionel- That is awesome, thanks for doing that. I'd call that a thumbs-up then. Weird about the long vignette time though. |
|
No new issues in rev.dep with the change below (on top of the patch) so this is going in now. edd@debbuilder:~/git/rcpp(pr/859)$ git diff inst/include/Rcpp.h
diff --git a/inst/include/Rcpp.h b/inst/include/Rcpp.h
index 0d2b9dbc..4951ac8c 100644
--- a/inst/include/Rcpp.h
+++ b/inst/include/Rcpp.h
@@ -23,6 +23,9 @@
#ifndef Rcpp_hpp
#define Rcpp_hpp
+// enable new feature
+#define RCPP_PROTECTED_EVAL
+
/* it is important that this comes first */
#include <RcppCommon.h>
edd@debbuilder:~/git/rcpp(pr/859)$ |
Unshield the protect-unwind changes from Use protect-unwind API and add Rcpp_fast_eval() #789
Don't throw exceptions across the C stack. I believe this is what caused the issues on Windows. Instead we now skip the C frames using a longjump and only then throw the longjump exception across the C++ stack.
Add unit tests for generating attributes in packages. One mock package exports a function via
Rcpp::interfaces(cpp)and the other calls it. The tests check that the C++ stack is properly unwound on longjumps issued from the exported function.Win-builder and r-hub don't seem to be reliable for testing these changes. Even with current master the former always gets stuck and the latter randomly hangs when running the tests or rebuilding vignettes. However I have set up a local Windows 10 and Rcpp successfully passes the tests. With #789 I get a
terminate called after throwing an instance of 'Rcpp::LongjumpException'crash. I don't know if all problems are fixed but hopefully we still have some time before the next release to test this further.