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

Apparent fragility in Rcpp use when not calling importFrom(Rcpp, somesymbol) #1030

Closed
eddelbuettel opened this issue Dec 4, 2019 · 7 comments
Closed

Comments

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Dec 4, 2019

I am currently running some reverse depends checks for a new RcppArmadillo minor release, and noticed a view package bonking in tests on function 'dataptr' not provided by package 'Rcpp'.

This relates to our file inst/include/Rcpp/routines.h which has it, but also carries a comment

// The 'attribute_hidden' used here is a simple precessor defined from
// ${R_HOME}/include/R_ext/Visibility.h -- it is empty when not supported
// by the compiler and otherwise '__attribute__ ((visibility ("hidden")))'

and that may recently have changed in R.

Do we care? Should we care?

Should we remove the now-no-longer empty attribute_hidden define in front of dataptr() and a number of other helpers?

@kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Dec 4, 2019

Are we sure this related to the use of attribute_hidden?

IIUC that error means that R wasn't able to find the R-level object acting as a pointer to the C-level entrypoint, which makes me think something "weird" happened to Rcpp during tests.

More directly, it means a call to R_GetCCallable() failed:

https://github.com/wch/r-source/blob/c12c7f3276c23bd6d3df5bffe1773cb39681fd2d/src/main/Rdynload.c#L1488-L1499

And we definitely register these routines here:

Rcpp/src/rcpp_init.cpp

Lines 88 to 125 in e77e79c

void registerFunctions(){
using namespace Rcpp;
using namespace Rcpp::internal;
#define RCPP_REGISTER(__FUN__) R_RegisterCCallable( "Rcpp", #__FUN__ , (DL_FUNC)__FUN__ );
RCPP_REGISTER(rcpp_get_stack_trace)
RCPP_REGISTER(rcpp_set_stack_trace)
RCPP_REGISTER(type2name)
RCPP_REGISTER(demangle)
RCPP_REGISTER(enterRNGScope)
RCPP_REGISTER(exitRNGScope)
RCPP_REGISTER(beginSuspendRNGSynchronization);
RCPP_REGISTER(endSuspendRNGSynchronization);
RCPP_REGISTER(get_Rcpp_namespace)
RCPP_REGISTER(get_cache)
RCPP_REGISTER(stack_trace)
RCPP_REGISTER(get_string_elt)
RCPP_REGISTER(char_get_string_elt)
RCPP_REGISTER(set_string_elt)
RCPP_REGISTER(char_set_string_elt)
RCPP_REGISTER(get_string_ptr)
RCPP_REGISTER(get_vector_elt)
RCPP_REGISTER(set_vector_elt)
RCPP_REGISTER(get_vector_ptr)
RCPP_REGISTER(char_nocheck)
RCPP_REGISTER(dataptr)
RCPP_REGISTER(getCurrentScope)
RCPP_REGISTER(setCurrentScope)
RCPP_REGISTER(get_string_buffer)
RCPP_REGISTER(short_file_name)
RCPP_REGISTER(mktime00)
RCPP_REGISTER(gmtime_)
RCPP_REGISTER(reset_current_error)
RCPP_REGISTER(error_occured)
RCPP_REGISTER(rcpp_get_current_error)
// RCPP_REGISTER(print)
#undef RCPP_REGISTER
}

So I'm not exactly sure what's going on.

@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Dec 4, 2019

Definitely not sure what is going on either, but just thought that it was something we did not change but which R may have changed?

That part of rcpp_init.cpp and routines.cpp is ... pretty old and unchanged too. So big hmpf from me.

@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Dec 5, 2019

Took another look, and it could definitely also be that the 'compiling' branch no longer activates. I would not know why.

Simple tests work for me:

R> cppFunction("bool getIt(NumericVector x) { void *ptr = dataptr(x); return ptr!=nullptr; }")
R> getIt(rnorm(5))
[1] FALSE
R> 
@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Dec 5, 2019

I think it is the use of Rcpp in the affected package. E.g. in the first one I looked, I noticed it did not have an importFrom("Rcpp", "evalCpp") (or alike) statement in NAMESPACE. Adding that, and hence forcing symbol registration, is all it took to make the error go away in one package:

diff -ru BatchMap.orig/DESCRIPTION BatchMap/DESCRIPTION
--- BatchMap.orig/DESCRIPTION   2017-11-12 00:55:01.000000000 +0100
+++ BatchMap/DESCRIPTION        2019-12-05 14:39:13.450765159 +0100
@@ -15,7 +15,7 @@
     "augusto.garcia@usp.br"))
 LinkingTo: Rcpp (>= 0.10.5), RcppArmadillo (>= 0.7.700)
 Depends: parallel (>= 3.2.3), ggplot2 (>= 1.0.1), R (>= 3.2.0)
-Imports: reshape2 (>= 1.4.1), methods
+Imports: reshape2 (>= 1.4.1), methods, Rcpp
 Suggests: knitr (>= 1.10), rmarkdown
 VignetteBuilder: knitr
 Encoding: UTF-8
diff -ru BatchMap.orig/NAMESPACE BatchMap/NAMESPACE
--- BatchMap.orig/NAMESPACE     2017-10-30 17:44:39.000000000 +0100
+++ BatchMap/NAMESPACE  2019-12-05 14:32:23.872820158 +0100
@@ -10,6 +10,7 @@
            "median", "na.omit", "qchisq", "sd")
 importFrom("utils", "flush.console", "head", "setTxtProgressBar",
            "tail", "txtProgressBar")
+importFrom("Rcpp", "evalCpp")
 ## Export all names
 exportPattern(".")
@eddelbuettel eddelbuettel changed the title Should we care about 'visibility' being changed by R? Apparent fragility in Rcpp use when not calling importFrom(Rcpp, somesymbol) Dec 5, 2019
@kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Dec 5, 2019

That makes sense to me. We've documented using importFrom(Rcpp, evalCpp) basically forever because of issues of this from.

@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Dec 5, 2019

Indeed. And when I looked at the package, it still had some older school direct use of SEXP, using .Call() with the symbol quoted and package name listed etc pp. In that case I think one simply needs it.

@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Dec 5, 2019

I just added to the Rcpp FAQ for this. PR coming in a moment, have a look at that to see if the writing I added is sufficient.

eddelbuettel added a commit that referenced this issue Dec 5, 2019
recommend importFrom in Rcpp FAQ (closes #1030)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.