Underlying CHARSXP in String class needs gc protection

[kevinushey]

This code shows that CHARSXPs can be garbage collected:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
NumericVector test() {
    Function gc("gc");

    SEXP character = Rf_mkChar("ouch");
    SEXP string = Rf_mkString("ouch");

    Shield<SEXP> other(Rf_allocVector(INTSXP, 1E6));
    gc();

    Rprintf("CHARSXP:\n");
    Rf_PrintValue(character);

    Rprintf("STRSXP:\n");
    Rf_PrintValue(string);

    return wrap(other);
}

/*** R
gctorture(TRUE)
invisible(test())
gctorture(FALSE)
*/

> Rcpp::sourceCpp('~/scratch/ouch.cpp')
> gctorture(TRUE)
> invisible(test())
CHARSXP:
[1] FALSE
STRSXP:
<garbage>
> gctorture(FALSE)

The CHARSXP has been replaced with garbage, as it has been GCed.

[kevinushey]

My guess is that this could be resolved by inheritting from the PreserveStorage 'class' to manage the underlying storage, and use the Storage:: 'API' functions to manage the underlying data. It's used extensively in the Vector class so that can be used as a template: https://github.com/RcppCore/Rcpp/blob/a2fc8a3b9f1b26843b8c341c32fa967ff406b642/inst/include/Rcpp/vector/Vector.h

[thirdwing]

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
NumericVector test() {
    Function gc("gc");

    SEXP character = Rf_mkChar("ouch");
    String string = String("ouch");

    // force big allocation + gc
    Shield<SEXP> other(Rf_allocVector(INTSXP, 1E6));
    gc();

    Rprintf("CHARSXP:\n");
    Rf_PrintValue(character);

    Rprintf("STRSXP:\n");
    Rf_PrintValue(wrap(string));

    return wrap(other);
}

> library(Rcpp)
> sourceCpp("ouch.cpp")
> gctorture(TRUE)
> invisible(test())
CHARSXP:
$warning
function (w)
{
    .warningsEnv$warnings <- append(.warningsEnv$warnings, w$message)
    invokeRestart("muffleWarning")
}
<environment: namespace:Rcpp>

STRSXP:
[1] "ouch"

Just a little change to your code.

[kevinushey]

@thirdwing, I think we're just getting lucky there. You can exercise a failure with e.g.

String rcppString("ouch");
String otherString = rcppString;

When I try to print otherString the underlying CHARSXP is now <NA>.

[kevinushey]

In fact, the reason why the constructor from const char* works without failure is because the underlying SEXP is never constructed; instead the underlying buffer is used: https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/String.h#L109-L112

        /** from a const char* */
        String( const char* s) : buffer(s), valid(false), buffer_ready(true), enc(CE_NATIVE){
            RCPP_STRING_DEBUG( "String(const char*)" ) ;
        }

Since we bypass using a SEXP as storage in that case, no protection worries ever occur.

[thirdwing]

You are right!

We need class String : public StoragePolicy< String > or something similar.

[thirdwing]

@kevinushey Since PreserveStorage uses R_PreserveObject to protect underlying SEXP.

In my repo (https://github.com/thirdwing/Rcpp), I provide a simple solution.

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
NumericVector test() {
    Function gc("gc");

    SEXP character = Rf_mkChar("ouch");
    String string = String("ouch string");
    String otherString = string;

    // force big allocation + gc
    Shield<SEXP> other(Rf_allocVector(INTSXP, 1E6));
    gc();

    Rprintf("CHARSXP:\n");
    Rf_PrintValue(character);

    Rprintf("STRSXP:\n");
    Rf_PrintValue(wrap(string));

    Rf_PrintValue(wrap(otherString));

    return wrap(other);
}

> Rcpp::sourceCpp("ouch.cpp")
> gctorture(TRUE)
> invisible(test())
CHARSXP:
$warning
function (w)
{
    .warningsEnv$warnings <- append(.warningsEnv$warnings, w$message)
    invokeRestart("muffleWarning")
}
<environment: namespace:Rcpp>

STRSXP:
[1] "ouch string"
[1] "ouch string"

Rewriting String will be much more elegant, but I am afraid it may break existing code.

[thirdwing]

I think we can close this, right?

[coatless]

This issue was addressed in PR #322 .

Please close. (Tagged #506)