Skip to content

add template specializations to avoid DATAPTR use #1310

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

Merged
merged 15 commits into from
Jun 22, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
2024-06-11 Kevin Ushey <kevinushey@gmail.com>

* inst/include/Rcpp/internal/r_vector.h: Use template specializations to
avoid DATAPTR usage
* inst/include/Rcpp/vector/traits.h: Implement bounds checks in
r_vector_cache access
* inst/tinytest/cpp/Vector.cpp: Add unit tests
* inst/tinytest/test_vector.R: Add unit tests
* tests/tinytest.R: Test in serial by default

2024-06-02 Dirk Eddelbuettel <edd@debian.org>

* inst/include/Rcpp/internal/export.h: More R_xlen_t switching
7 changes: 6 additions & 1 deletion inst/NEWS.Rd
Original file line number Diff line number Diff line change
@@ -9,9 +9,14 @@
\itemize{
\item Set R_NO_REMAP if not already defined (Dirk in \ghpr{1296})
\item Add variadic templates to be used instead of generated code
(Andrew Johnson in \ghpr{1303}
(Andrew Johnson in \ghpr{1303})
\item Count variables were switches to \code{size_t} to avoid warnings
about conversion-narrowing (Dirk in \ghpr{1307})
\item Rcpp now avoids the usage of the (non-API) DATAPTR function when
accessing the contents of Rcpp Vector objects where possible. (Kevin in
\ghpr{1310})
\item Rcpp now emits an R warning on out-of-bounds Vector accesses. This
may become an error in a future Rcpp release. (Kevin in \ghpr{1310})
}
\item Changes in Rcpp Deployment:
\itemize{
15 changes: 15 additions & 0 deletions inst/include/Rcpp/internal/r_vector.h
Original file line number Diff line number Diff line change
@@ -32,6 +32,21 @@ typename Rcpp::traits::storage_type<RTYPE>::type* r_vector_start(SEXP x) {
return reinterpret_cast<pointer>(dataptr(x));
}

// add specializations to avoid use of dataptr
#define RCPP_VECTOR_START_IMPL(__RTYPE__, __ACCESSOR__) \
template <> \
inline typename Rcpp::traits::storage_type<__RTYPE__>::type* r_vector_start<__RTYPE__>(SEXP x) { \
return __ACCESSOR__(x); \
}

RCPP_VECTOR_START_IMPL(LGLSXP, LOGICAL);
RCPP_VECTOR_START_IMPL(INTSXP, INTEGER);
RCPP_VECTOR_START_IMPL(RAWSXP, RAW);
RCPP_VECTOR_START_IMPL(CPLXSXP, COMPLEX);
RCPP_VECTOR_START_IMPL(REALSXP, REAL);

#undef RCPP_VECTOR_START_IMPL

/**
* The value 0 statically casted to the appropriate type for
* the given SEXP type
7 changes: 2 additions & 5 deletions inst/include/Rcpp/vector/Subsetter.h
Original file line number Diff line number Diff line change
@@ -133,9 +133,9 @@ class SubsetProxy {

private:

#ifndef RCPP_NO_BOUNDS_CHECK
template <typename IDX>
void check_indices(IDX* x, R_xlen_t n, R_xlen_t size) {
#ifndef RCPP_NO_BOUNDS_CHECK
for (IDX i=0; i < n; ++i) {
if (x[i] < 0 or x[i] >= size) {
if(std::numeric_limits<IDX>::is_integer && size > std::numeric_limits<IDX>::max()) {
@@ -144,11 +144,8 @@ class SubsetProxy {
stop("index error");
}
}
#endif
}
#else
template <typename IDX>
void check_indices(IDX* x, IDX n, IDX size) {}
#endif

void get_indices( traits::identity< traits::int2type<INTSXP> > t ) {
indices.reserve(rhs_n);
47 changes: 34 additions & 13 deletions inst/include/Rcpp/vector/traits.h
Original file line number Diff line number Diff line change
@@ -35,22 +35,36 @@ namespace traits{
typedef typename r_vector_const_proxy<RTYPE>::type const_proxy ;
typedef typename storage_type<RTYPE>::type storage_type ;

r_vector_cache() : start(0){} ;
r_vector_cache() : start(0), size(0) {} ;

inline void update( const VECTOR& v ) {
start = ::Rcpp::internal::r_vector_start<RTYPE>(v) ;
start = ::Rcpp::internal::r_vector_start<RTYPE>(v) ;
size = v.size();
}

inline iterator get() const { return start; }
inline const_iterator get_const() const { return start; }

inline proxy ref() { return *start ;}
inline proxy ref(R_xlen_t i) { return start[i] ; }
inline proxy ref() { check_index(0); return start[0] ;}
inline proxy ref(R_xlen_t i) { check_index(i); return start[i] ; }

inline proxy ref() const { check_index(0); return start[0] ;}
inline proxy ref(R_xlen_t i) const { check_index(i); return start[i] ; }

inline proxy ref() const { return *start ;}
inline proxy ref(R_xlen_t i) const { return start[i] ; }
private:

private:
iterator start ;
void check_index(R_xlen_t i) const {
#ifndef RCPP_NO_BOUNDS_CHECK
if (i >= size) {
warning("subscript out of bounds (index %s >= vector size %s)", i, size);
}
#endif
}

iterator start ;
R_xlen_t size ;
} ;

template <int RTYPE, template <class> class StoragePolicy = PreserveStorage>
class proxy_cache{
public:
@@ -66,17 +80,24 @@ namespace traits{
p = const_cast<VECTOR*>(&v) ;
}
inline iterator get() const { return iterator( proxy(*p, 0 ) ) ;}
// inline const_iterator get_const() const { return const_iterator( *p ) ;}
inline const_iterator get_const() const { return const_iterator( const_proxy(*p, 0) ) ; }

inline proxy ref() { return proxy(*p,0) ; }
inline proxy ref(R_xlen_t i) { return proxy(*p,i);}
inline proxy ref() { check_index(0); return proxy(*p,0) ; }
inline proxy ref(R_xlen_t i) { check_index(i); return proxy(*p,i);}

inline const_proxy ref() const { return const_proxy(*p,0) ; }
inline const_proxy ref(R_xlen_t i) const { return const_proxy(*p,i);}
inline const_proxy ref() const { check_index(0); return const_proxy(*p,0) ; }
inline const_proxy ref(R_xlen_t i) const { check_index(i); return const_proxy(*p,i);}

private:
VECTOR* p ;

void check_index(R_xlen_t i) const {
#ifndef RCPP_NO_BOUNDS_CHECK
if (i >= p->size()) {
warning("subscript out of bounds (index %s >= vector size %s)", i, p->size());
}
#endif
}
} ;

// regular types for INTSXP, REALSXP, ...
10 changes: 10 additions & 0 deletions inst/tinytest/cpp/Vector.cpp
Original file line number Diff line number Diff line change
@@ -886,3 +886,13 @@ bool CharacterVector_test_equality_crosspolicy(CharacterVector x, Vector<STRSXP,

return std::equal(x.begin(), x.end(), y.begin());
}

// [[Rcpp::export]]
double NumericVector_test_out_of_bounds_read(NumericVector v, R_xlen_t i) {
return v[i];
}

// [[Rcpp::export]]
SEXP CharacterVector_test_out_of_bounds_read(CharacterVector v, R_xlen_t i) {
return v[i];
}
4 changes: 2 additions & 2 deletions inst/tinytest/test_packageversion.R
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

## Copyright (C) 2019 - 2022 Dirk Eddelbuettel
## Copyright (C) 2019 - 2024 Dirk Eddelbuettel
##
## This file is part of Rcpp.
##
@@ -30,7 +30,7 @@ v <- as.integer(unlist(strsplit(pvstr, "\\.")))
relstr <- as.character(as.package_version(paste(v[1:3], collapse=".")))

## call C++ function returning list of six values, three each for 'release' and 'dev' version
res <- checkVersion(v)
res <- checkVersion(v[1:min(4, length(v))])


## basic check: is the #defined version equal to the computed version (issue #1014)
2 changes: 1 addition & 1 deletion inst/tinytest/test_rcpp_package_skeleton.R
Original file line number Diff line number Diff line change
@@ -69,7 +69,7 @@ for (file in grep("RcppExports.R", R_files, invert=TRUE, value=TRUE)) {
code <- readLines(file)
fn <- eval(parse(text=paste(code, collapse="\n")))
fn_name <- gsub(".*/(.*)\\.R$", "\\1", file)
checkIdentical(fn, get(fn_name),
checkIdentical(fn, base::get(fn_name),
sprintf("we parsed the function '%s' correctly", fn_name)
)
}
8 changes: 8 additions & 0 deletions inst/tinytest/test_vector.R
Original file line number Diff line number Diff line change
@@ -694,3 +694,11 @@ expect_equal(data, data2)
# test.CharacterVector_test_equality <- function(){
expect_true( !CharacterVector_test_equality("foo", "bar") )
expect_true( !CharacterVector_test_equality_crosspolicy("foo", "bar") )

# https://github.com/RcppCore/Rcpp/issues/1308
# tests disabled since these could trigger UBSAN warnings / crashes
#expect_warning(NumericVector_test_out_of_bounds_read(numeric(0), 0))
#expect_warning(NumericVector_test_out_of_bounds_read(numeric(1), 1))
#expect_warning(CharacterVector_test_out_of_bounds_read(character(0), 0))
#expect_warning(CharacterVector_test_out_of_bounds_read(character(1), 1))

2 changes: 1 addition & 1 deletion tests/tinytest.R
Original file line number Diff line number Diff line change
@@ -43,5 +43,5 @@ if (requireNamespace("tinytest", quietly=TRUE)) {
## there are several more granular ways to test files in a tinytest directory,
## see its package vignette; tests can also run once the package is installed
## using the same command `test_package(pkgName)`, or by director or file
tinytest::test_package("Rcpp", ncpu=getOption("Ncpus", 1))
tinytest::test_package("Rcpp")
}