Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements for unwind-protect #877

Merged
merged 7 commits into from Jul 12, 2018

Conversation

Projects
None yet
3 participants
@lionel-
Copy link
Contributor

lionel- commented Jul 10, 2018

  • Add a plugin for enabling unwind-protect. Using // [[Rcpp::plugins(unwindProtect)]] is safer than #define because the plugin ensures unwind-protect is enabled in all compilation units including RcppExports.cpp. This avoids linking issues.

  • Make LongjumpException public. Users should catch and rethrow it before catch (...) so longjumps are not ignored.

  • Move unwindProtect() to Rcpp/unwindProtect.h. Include it early in RcppCommon to make it available in other header files. This will be useful for calling into R APIs for initialising static variables etc (more on that later).

  • Rename RCPP_PROTECTED_EVAL to RCPP_USE_UNWIND_PROTECT. Then RCPP_USING_UNWIND_PROTECT is defined in headers.h if the R version is sufficient. The latter define can be used in the rest of header files.

  • Refactor unit tests so that the unwound indicator is an environment rather than a logical vector. I got into trouble with the latter because the bytecode compiler replaces scalar logical with globally shared TRUE or FALSE values. Mutating these values corrupts the runtime.

  • Pass ... from unitTestSetup() to sourceCpp() for easy debugging.

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jul 10, 2018

@eddelbuettel Would you like to rename Rcpp::Rcpp_fast_eval() here? If yes do you want to try Rcpp::eval()? Alternatively Rcpp::evalProtect() might not be too bad a name.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 10, 2018

Codecov Report

Merging #877 into master will not change coverage.
The diff coverage is 36.84%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #877   +/-   ##
=======================================
  Coverage   90.09%   90.09%           
=======================================
  Files          71       71           
  Lines        3271     3271           
=======================================
  Hits         2947     2947           
  Misses        324      324
Impacted Files Coverage Δ
inst/include/RcppCommon.h 100% <ø> (ø) ⬆️
inst/include/Rcpp/Function.h 80% <ø> (ø) ⬆️
inst/include/Rcpp/api/meat/Rcpp_eval.h 66.66% <ø> (ø) ⬆️
src/attributes.cpp 98.5% <ø> (ø) ⬆️
inst/include/Rcpp/r_cast.h 100% <ø> (ø) ⬆️
R/Attributes.R 90.66% <ø> (ø) ⬆️
inst/include/Rcpp/proxy/NamesProxy.h 75% <0%> (ø) ⬆️
inst/include/Rcpp/exceptions.h 7.69% <0%> (ø) ⬆️
R/unit.tests.R 100% <100%> (ø) ⬆️
inst/include/Rcpp/Environment.h 100% <100%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5591c5...2ba5095. Read the comment docs.

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jul 10, 2018

oops R CMD check fails on Windows with this branch, will investigate.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jul 10, 2018

Hi @lionel- -- there is a lot to unpack here. I am wondering if we should split it over several smaller PRs.

Lots of it looks really good and like something we can / should do (the plugin, moving to a normal header included included early, ...) but it seems we are wrapping this up with turning more things on automatically which may still be risky.

I was also thinking about a release "soon" so maybe doing this in parts may be safer?

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jul 11, 2018

it seems we are wrapping this up with turning more things on automatically which may still be risky

Sorry I didn't understand that. What do you mean?

I was also thinking about a release "soon" so maybe doing this in parts may be safer?

I have put off all new/risky things for later PRs that I will send after the release. I think almost all of this should be in the next release:

  • Making LongjumpException public is essential for compatibility of unwind-protect with code that uses catch (...), so users can avoid swallowing the LJ exception.
  • Enabling unwindProtect with the plugin ensures the correct Makevars flag is used to avoid UB issues.
  • It is safer to remove the scalar logical mutation from the unit tests. Though the unit tests do work for the current version of R.

On the other hand including unwindProtect() earlier in the build process is not essential right now. Would you prefer to remove this commit?

Oh and also I meant to remove the default argument of Rcpp_fast_eval(). The global env is often not the right default and I think we should force users to think about where they want to evaluate (their own namespace, the base env, the global env?). Any objection? It is important to do this before release so we don't modify the API later on.

Likewise, it would be preferable to rename fast_eval to make it consistent with the rest of the Rcpp API before release.

@lionel- lionel- force-pushed the lionel-:impl-unwind-protect-plugin branch from 60472f3 to 40872fc Jul 11, 2018

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jul 11, 2018

oops R CMD check fails on Windows with this branch, will investigate.

It was not Windows but on 3.5 in general, my bad for not checking the last commit properly. Now fixed.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jul 11, 2018

Sorry I didn't understand that. What do you mean?

Maybe I misread above at first. Does it, or does it not, turn the new behaviour on by default? Last time we still had three important packages fail. Can you clarify?

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jul 11, 2018

The plugin is for enabling unwind-protect, which is disabled by default. The three package fails were actually normal failures but I've prepared some tools to help fix those. I will send a new PR after the release.

Any objection to including the following commit to this PR? lionel-@2ba5095 It removes the default argument for fast_eval(). This commit passes R CMD check on mac, Linux, Windows, with R 3.4 and R 3.5.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jul 11, 2018

Yes toss it in. May as well test all at once.

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jul 11, 2018

What about renaming to Rcpp::eval() or Rcpp::evalProtect()? Or do you prefer to keep the current name?

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jul 11, 2018

Yes we can do that but we don't have to do it for this round of testing. We have time to do that I think.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jul 12, 2018

"No change to worse", ie no new rev.dep failures in this PR relative to the master branch, so merging in.

@eddelbuettel eddelbuettel merged commit 64290a7 into RcppCore:master Jul 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.