Possible Rcpp and ALTREP interaction

[eddelbuettel]

Over in this StackOverflow post we see that a sequence like 1:10 get sum()ed in R seemingly on SEXP header notion.

When Rcpp inserts a value, the sequence gets broken but the bit does not get unset. Thanks to @rstub to making it plain via .Internal(inspect). In short:

R> v <- 1:10
R> sum(v)
[1] 55
R> .Internal(inspect(v))
@559843d446c0 13 INTSXP g0c0 [NAM(3)]  1 : 10 (compact)
R> 
R> 
R> v[5] <- 100
R> sum(v)
[1] 150
R> .Internal(inspect(v))
@55984953b748 14 REALSXP g0c5 [NAM(1)] (len=10, tl=0) 1,2,3,4,100,...
R> 

In short we do not make the transition from the 1 : 10 (compact) representation to the enumerated materialized set -- yet sum() in R only looks at that representation bit and then returns the wrong result.

So with Rcpp (and the changed code by the OP):

R> Rcpp::cppFunction("IntegerVector modvec(IntegerVector x) { x[4] = 100; return(x); }")
R> v <- 1:10; sum(v); .Internal(inspect(v))
[1] 55
@559846e37070 13 INTSXP g0c0 [NAM(3)]  1 : 10 (compact)
R> 
R> modvec(v)
 [1]   1   2   3   4 100   6   7   8   9  10
R> 
R> sum(v); .Internal(inspect(v))           # so sum() now lies to us as we hide change
[1] 55       
@559846e37070 13 INTSXP g0c0 [NAM(3)]  1 : 10 (expanded)
R> 
R> v[5] <- 101
R> sum(v); .Internal(inspect(v))
[1] 151
@55984618c628 14 REALSXP g0c5 [NAM(1)] (len=10, tl=0) 1,2,3,4,101,...
R> 

@gmbecker If you have quick pointers... I still have your 'well ALTREP should not require user packages changes' ringing in my ear...

[kevinushey]

I'm guessing that the issue is similar to this:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
SEXP naughty(SEXP data) {
  int* ptr = INTEGER(data);
  ptr[5] = 100;
  return data;
}

/*** R
v <- 1:10
naughty(v)
v
sum(v)
*/

which gives me as output:

> Rcpp::sourceCpp('~/scratch/naughty.cpp')
> v <- 1:10
> naughty(v)
 [1]   1   2   3   4   5 100   7   8   9  10
> v
 [1]   1   2   3   4   5 100   7   8   9  10
> sum(v)
[1] 55

In other words, because we modify the data 'directly' through INTEGER() / DATAPTR(), we skip R's normal ALTREP checks and so we end up with an R object in an inconsistent state.

I think ALTREP is normally supposed to force materialization of the underlying vector in these cases though?

[eddelbuettel]

Not just similar, this is the same issue -- but your example is nicely minimal and shows we can also shoot ourselves in the foot using the C API of R.

[rstub]

A quote from the R sources:

/* By default, compact integer sequences are marked as not mutable at
   creation time. Thus even when expanded the expanded data will
   correspond to the original integer sequence (unless it runs into
   mis-behaving C code). If COMPACT_INTSEQ_MUTABLE is defined, then
   the sequence is not marked as not mutable. Once the DATAPTR has
   been requested and releases, the expanded data might be modified by
   an assignment and no longer correspond to the original sequence. */
//#define COMPACT_INTSEQ_MUTABLE

https://github.com/wch/r-source/blob/9b2a6fc19ca853cff5ae09cb0efcfb179c6da3a5/src/main/altclasses.c#L60-L67

And later on:

static SEXP compact_intseq_Sum(SEXP x, Rboolean narm)
{
#ifdef COMPACT_INTSEQ_MUTABLE
    /* If the vector has been expanded it may have been modified. */
    if (COMPACT_SEQ_EXPANDED(x) != R_NilValue) 
	return NULL;
#endif
...

https://github.com/wch/r-source/blob/9b2a6fc19ca853cff5ae09cb0efcfb179c6da3a5/src/main/altclasses.c#L258-L264

Somehow this looks like intended behavior?!

I do not understand, though, why it does work as expected when called from R:

	    int *px = INTEGER(x);
	    VECTOR_ASSIGN_LOOP(px[ii] = INTEGER_ELT(y, iny););

https://github.com/wch/r-source/blob/9b2a6fc19ca853cff5ae09cb0efcfb179c6da3a5/src/main/subassign.c#L714-L715

Or am I looking at the wrong place?

[eddelbuettel]

We may need a second (and third) set of eyes. I'll email @gmbecker @kalibera @ltierney later with a link to this and the StackOverflow issue referenced above.

[kalibera]

Please make sure you are not violating the value semantics of R when changing the object. See WRE 5.9.10.

[eddelbuettel]

@kalibera I am not sure I see how 5.9.10 applies. This is about modifcation in place, ie

SEXP naughty(SEXP data) {
  int* ptr = INTEGER(data);
  ptr[5] = 100;
  return data;
}

This code appears to be too minimal. How should be done in an ALTREP world?

[kalibera]

Well you can only modify "data" when it is not referenced (!MAYBE_REFERENCED, NAMED value is 0), otherwise you should duplicate and modify the copy - and then also return the copy from the function. That part is independent of ALTREP but also ALTREP relies on that these rules are followed, so it would be best to have reproducible example that follows them, too (and does not use any non-base packages, including rcpp). Other than that, you can now use SET_INTEGER_ELT instead of INTEGER/DATAPTR but that should only matter for performance, and you still need to check for references and copy accordingly.

[eddelbuettel]

Thanks, that was more helpful though I unsure where you see non base R in the now-twice-quoted C++ snippet. We use Rcpp for convenience; I am apparently unable to call a symbol from a DLL loaded after R CMD SHLIB and dyn.load -- the .Call() complains about an unreg symbol. Anyway...

When I use SET_INTEGER_ELT I can still fool the ALTREP-aware sum() in R:

R> Rcpp::cppFunction("SEXP nice(SEXP x) { SET_INTEGER_ELT(x, 4, 100); return x; }")
R> x <- 1:10
R> sum(x)
[1] 55
R> x <- nice(x)
R> sum(x)
[1] 55
R> x
 [1]   1   2   3   4 100   6   7   8   9  10
R> 

I hope you forgive for use Rcpp to materialize the code. What matters is the use of SET_INTEGER_ELT. It alone seem insufficient for (re-)setting the right bits even though the status changes from 'compact' to 'expanded':

R> .Internal(inspect(x))                # same x as above with 100 hiding in the middle
@1570ee8 13 INTSXP g0c0 [NAM(3)]  1 : 10 (expanded)
R> .Internal(inspect(1:10))           # where sequence is compact
@13caa88 13 INTSXP g0c0 [NAM(3)]  1 : 10 (compact)
R> 

[ltierney]

Rcpp::cppFunction("SEXP nice(SEXP x) { if (MAYBE_SHARED(x)) Rf_error(\"you are being naughty\"); INTEGER(x)[4] = 100; return x;}") nice(1)

Error in nice(1) : you are being naughty

nice(1:10)

Error in nice(1:10) : you are being naughty

nice(1 + 0)

Error in nice(1 + 0) : you are being naughty DO NOT MODIFY AN OBJECT IF MAYBE_SHARED RETURNS TRUE!!! Your nice convenient wrapper produces a closure. At least with the current NAMED framework MAYBE_SHARED will ALWAYS return TRUE for arguments passed into your C/C++ code this way. So: DO NOT MODIFY OBJECTS PASSED TO YOUR C/C++ code this way!!! If you do { x <- ...; .Call(foo, x) } then your 'foo' MAY see an object with MAYBE_SHARED(X) FALSE; it may be OK to modify that one. Compact sequences are always marked as immutable -- they have to be duplicated before modifying. This could have been done differently, and may change, but this is how it is now, and if you follow the rules it is not a problem. If an ALTREP object that is marked as immutable returns a data pointer into a read-only memory segment (as a memory-mapped file object might) and you don't follow the rules and write anyway then you know what will happen. So follow the rules. Best, luke

…

On Thu, 28 Feb 2019, Dirk Eddelbuettel wrote: Thanks, that was more helpful (though I unsure where you see non base R in the now-twice-quoted C++ snippet. We use Rcpp for convenience; I am apparently unable to call a symbol from a DLL loaded after R CMD SHLIB and dyn.load -- the .Call() complains about an unreg symbol. Anyway... When I use SET_INTEGER_ELT I can still fool the ALTREP-aware sum() in R: R> Rcpp::cppFunction("SEXP nice(SEXP x) { SET_INTEGER_ELT(x, 4, 100); return x; }") R> x <- 1:10 R> sum(x) [1] 55 R> x <- nice(x) R> sum(x) [1] 55 R> x [1] 1 2 3 4 100 6 7 8 9 10 R> I hope you forgive for use Rcpp to materialize the code. What matters is the use of SET_INTEGER_ELT. It alone seem insufficient for (re-)setting the right bits even though the status changes from 'compact' to 'expanded': R> .Internal(inspect(x)) # same x as above with 100 hiding in the middle @1570ee8 13 INTSXP g0c0 [NAM(3)] 1 : 10 (expanded) R> .Internal(inspect(1:10)) # where sequence is compact @13caa88 13 INTSXP g0c0 [NAM(3)] 1 : 10 (compact) R> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.[ADvaVOY2mjbw4o8nc81Y9bYg_tU7ogR8ks5vR_5lgaJpZM4bUh2K.gif]

-- Luke Tierney Ralph E. Wareham Professor of Mathematical Sciences University of Iowa Phone: 319-335-3386 Department of Statistics and Fax: 319-335-3017 Actuarial Science 241 Schaeffer Hall email: luke-tierney@uiowa.edu Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[eddelbuettel]

Thanks, @ltierney. I think Rcpp may generally be a code review step behind the enhanced reference counting that provided the 'base layer' for ALTREP. We are getting by because the pattern at the start of this more recent thread (ie in the StackOverflow question) is rare. We don't often get sequence as input and output whil also being modified. But the overall need for paying more attention to MAYBE_SHARED(x) is clear. The devil, however, is in the detail as it is not clear how to operate on a FALSE that can be ignored (as in your last example).

[ltierney]

Just to be clear: this is now new with ALTREP. The MAYBE_SHARED test is a bit more recent, still well before ALTREP, but the basic idea what C code should not modify objects that might be shared has been true since day 1. Violations were more likely to go unnoticed in the days when R did more duplicating. But as otherwise unnecessary duplicate() calls have been removed, more of these issues are unmasked.

…

On Thu, 28 Feb 2019, Dirk Eddelbuettel wrote: Thanks, @ltierney. I think Rcpp may generally be a code review step behind the enhanced reference counting that provided the 'base layer' for ALTREP. We are getting by because the pattern at the start of this more recent thread (ie in the StackOverflow question) is rare. We don't often get sequence as input and output whil also being modified. But the overall need for paying more attention to MAYBE_SHARED(x) is clear. The devil, however, is in the detail as it is not clear how to operate on a FALSE that can be ignored (as in your last example). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.[ADvaVACT1Nk1fpwpP9qLiE24rDxr2EQ9ks5vSAzmgaJpZM4bUh2K.gif]

-- Luke Tierney Ralph E. Wareham Professor of Mathematical Sciences University of Iowa Phone: 319-335-3386 Department of Statistics and Fax: 319-335-3017 Actuarial Science 241 Schaeffer Hall email: luke-tierney@uiowa.edu Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[eddelbuettel]

Sure. "Empirically speaking", everybody seems to get by because the main usage patterns appears to mostly be "functional" in the sense that incoming variable are not modified, and newly created variable are returned. But as has been pointed out before, the internal organization of the code could be stricter in enforcing that. I would be extremely receptive to constructive help but I understand that everybody is busy. So are we, so no ETA for when this may get improved...

[gmbecker]

@eddelbuettel et al,

So there's a couple of things here, most of which @ltierney and/or @kalibera alluded to but which perhaps can benefit from more concrete possible coding patterns.

The crux of the matter here is inline modifying the payload of a SEXP by writing to elements of its DATAPTR-addressed memory. Is it ever ok to do (yes), can it be safe to do without confirming/being guaranteed its ok to do (no, never). And for objects created in the same chunk of C/C++n code, you may have such a priori guarantees, but you're not going to for objects passed down from R.

To be concrete, I don't know of any times it would be ok to write to the pointer returned by INTEGER() on a SEXP that lives in R-space without first checking MAYBE_SHARED(). If MAYBE_SHARED(x) returns FALSE, then it's ok, and you can proceed writing to the pointer, exactly as your code did.

If MAYBE_SHARED(x) == TRUE, then you need to duplicate, operate on the copy, and then return that. When you're in C/C++ code, at the level of grabbing data pointers to things, then you, your code needs to explicitly cause that duplication, protect the new duplicated result, etc.

Now the reason that this particular thing is happening is in the compact sequence case is that, unless R itself is build a specific non-default way, compact sequences ALWAYS have NAMED(x) == MAXNAMED (edit: ie 7, not 2 as I previously stated), from the point of creation. As Luke pointed out, this could change, but is currently by design. So even in a .Call situation where the closure wasn't forcing the NAMED count up, compact sequences will always need duplication before inline modification. And while this is a choice we could have made different in the compact sequence case, Luke's point about other ALTREPs is more important.

There may be ALTREP SEXPs where the memory pointed to by the pointer returned by, e.g., INTEGER is literally not writable memory for one reason or another. The way that those ALTREP classes will declare that is by doing the same thing the compact sequences do, by marking themselves as "IMMUTABLE", ie by doing MARK_NOT_MUTABLE(x) (which currently sets NAMED to MAXNAMED, but which is future-proof against eventual change to reference counting). This declares the contract that the SEXP must be duplicated before any code that grabs the datapointer and writes to it.

Ultimately, I agree this is a really weird unexpected behavior, but its due to a failure to meet the contract which has always been there. It may have been safe(ish, and I still doubt it) to ignore/be lax about in certain cases in the past, but with the advent of ALTREP it now must always be followed for the reasons outlined in this thread.

So all all all code thats going to grab a dataptr from a pre-existing (R-level) SEXP and write to it must follow a pattern along the lines of (or equivalently careful to):

SEXP awesomefun(SEXP x) 
{
    int nprot = 0;
    if(MAYBE_SHARED(x)) {
        PROTECT(x = duplicate(x)); nprot++; 
    }
    /* do awesome things to x that modify it inline
        protect other things as necessary but always increment nprot when you do, 
        decrement nprot if you ever unprotect */
    if(nprot) UNPROTECT(nprot);
    return x;
}

Any code which writes to datapointers retrieved from SEXPs it didn't itself create (ie anything that comes down from the R side) without doing this was already violating the C-API contract but is now also fatally ALTREP unsafe, as the motivating example showed.

And, one more time, remember, compact sequences could behave differently so that that code happened to work, but other ALTREP classes (Luke mentions memory-mapped-file backed ALTREPS) couldn't, so the behavior of compact sequences isn't actually the issue here.

I hope that is helpful and makes things clearer.

Best