RcppCore / Rcpp

Seamless R and C++ Integration
https://www.rcpp.org
GNU General Public License v2.0
742 stars 212 forks source link

loading a C++ class exposed with RCPP_MODULE fails under gctorture #121

Closed madams846 closed 10 years ago

madams846 commented 10 years ago

Loading the Rcpp package skeleton with 'module=TRUE' fails under gctorture. I also get this error message with other packages that use RcppModules to expose a C++ class:

> library(Rcpp)
> gctorture()
> library(testpkg)
Error in .doLoadActions(where, attach) :
  error in load action .__A__.1 for package testpkg: loadModule(module = "mod", what = TRUE, env = ns, loadNow = TRUE): Unable to load module "mod": invalid assignment for reference class field "size", should be from class "integer" or a subclass (was class "character")
Error: package or namespace load failed for 'testpkg'
>
kevinushey commented 10 years ago

A reproducible script:

library(Rcpp)
name <- "testpkg"
path <- tempdir()
pkgpath <- file.path(path, name)
Rcpp.package.skeleton(name=name, path=path, module=TRUE)
## make sure loadModule is in NAMESPACE too
system(paste("echo 'import(Rcpp)' >>", file.path(pkgpath, "NAMESPACE")))
system(paste("R CMD INSTALL", pkgpath))
gctorture(TRUE)
library(testpkg)
gctorture(FALSE)

I get the same error:

Error in .doLoadActions(where, attach) : 
  error in load action .__A__.1 for package testpkg: 
loadModule(module = "NumEx", what = TRUE, env = ns, loadNow = TRUE): 
Unable to load module "NumEx": invalid assignment for reference class field 
‘read_only’, should be from class “logical” or a subclass (was class “character”)
> sessionInfo()
R Under development (unstable) (2014-02-12 r64976)
Platform: x86_64-apple-darwin13.0.0 (64-bit)

locale:
[1] en_CA.UTF-8/en_CA.UTF-8/en_CA.UTF-8/C/en_CA.UTF-8/en_CA.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] Rcpp_0.11.0.2        knitr_1.5.15         devtools_1.4.1.99   
[4] BiocInstaller_1.13.3

loaded via a namespace (and not attached):
 [1] compiler_3.1.0 digest_0.6.4   evaluate_0.5.1 formatR_0.10   httr_0.2      
 [6] memoise_0.1    parallel_3.1.0 RCurl_1.95-4.1 stringr_0.6.2  tools_3.1.0   
[11] whisker_0.3-2 
kevinushey commented 10 years ago

The error occurs in this line of Rcpp:::Module:

classes <- .Call(Module__classes_info, xp)

Odds are it's trying to look at something that should've been protected, but wasn't. I'll try to narrow it down further.

kevinushey commented 10 years ago

Narrowed down to here so far:

CppClass( Module* p, class_Base* cl, std::string& buffer ) : S4("C++Class") {
            XP_Class clxp( cl, false, R_NilValue, R_NilValue ) ;
            slot( "module"  ) = XP( p, false ) ;
            slot( "pointer" ) = clxp ;

            buffer = "Rcpp_" ;
            buffer += cl->name ;
            slot( ".Data" ) = buffer ;

-->         slot( "fields" )      = cl->fields( clxp ) ;

            slot( "methods" )     = cl->getMethods( clxp, buffer ) ;
            slot( "constructors") = cl->getConstructors( clxp, buffer ) ;
            slot( "docstring"   ) = cl->docstring ;
            slot( "typeid" )      = cl->get_typeinfo_name() ;
            slot( "enums"  )      = cl->enums ;
            slot( "parents" )     = cl->parents ;
        }

class_Base doesn't have any R structures in it, so I guess that something is going wrong either in clxp, or in slot("fields").operator=.

kevinushey commented 10 years ago

I was a bit wrong -- the error actually occurs in the clxp constructor, which assigns to the fields slot with the line

    template <typename Class>
    class S4_field : public Rcpp::Reference {
    public:   
        typedef XPtr<class_Base> XP_Class ;
        S4_field( CppProperty<Class>* p, const XP_Class& class_xp ) : Reference( "C++Field" ){
            RCPP_DEBUG( "S4_field( CppProperty<Class>* p, const XP_Class& class_xp )" )
-->         field( "read_only" )     = p->is_readonly() ;
            field( "cpp_class" )     = p->get_class();
            field( "pointer" )       = Rcpp::XPtr< CppProperty<Class> >( p, false ) ;
            field( "class_pointer" ) = class_xp ;
            field( "docstring" )     = p->docstring ;
        }

        RCPP_CTOR_ASSIGN(S4_field)

    } ;

For whatever reason, inside the FieldProxy set method:

void set(SEXP x ) {
    SEXP dollarGetsSym = Rf_install( "$<-");
    Shield<SEXP> call( Rf_lang4( dollarGetsSym, parent, Rf_mkString(field_name.c_str()) , x ) ) ;
    parent.set__( Rf_eval( call, R_GlobalEnv ) );     
}

, it's seeing the value of x as:

<CHARSXP: "<NA>">

which then throws an R error, because R level evaluation fails as the type is no longer logical.

kevinushey commented 10 years ago

Finally tracked it down to this part of operator=, in FieldProxy.h:

template <typename T> FieldProxy& operator=(const T& rhs) {
    set( wrap(rhs) );
    return *this;
}

And for reference, set looks like this:

void set(SEXP x ) {
    SEXP dollarGetsSym = Rf_install( "$<-");
    Shield<SEXP> call( Rf_lang4( dollarGetsSym, parent, Rf_mkString(field_name.c_str()) , x ) ) ;
    parent.set__( Rf_eval( call, R_GlobalEnv ) );     
}

The output of wrap(rhs) is getting caught by the GC and unprotected while within set. If I change this to:

template <typename T> FieldProxy& operator=(const T& rhs) {
    SEXP tmp = PROTECT( wrap(rhs) );
    set(tmp);
    UNPROTECT(1);
  return *this;
}

library(testpkg) will succeed under gctorture.

My guess: set has some callbacks to R, and one of them triggers an allocation and hence a collection of the object x before it can actually be set. I imagine this is either fixed by keeping the PROTECT macros in place here as I've done, or else finding equivalent C API calls to accomplish the same task, and avoiding any allocations that trigger the GC.

kevinushey commented 10 years ago

Committed for now, but the best solution is to replace the callbacks in set with calls to R's C API -- whatever the equivalent to

parent$field_name <- x

is, for a reference class.

eddelbuettel commented 10 years ago

You continue to impress -- nice debugging work!

madams846 commented 10 years ago

kevinushey's change does fix this bug, I was wrong, sorry.

eddelbuettel commented 10 years ago

Cool, thanks for taking the time to test and confirm. Closing this now.