RcppCore / Rcpp

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

infinite loop (leads to segfault due to stack overflow) ... what's wrong? #178

Closed nixn closed 10 years ago

nixn commented 10 years ago

I'm starting with RInside and Rcpp (so I have the latest packages of Rcpp and RInside), but my first trivial program fails. The program is as follows (c++11):

vector<double> data(15);
for_each(data.begin(), data.end(), [](double &val) { val = rand(); });
RInside R;
R["data"] = data;

The last line causes the problem. In gdb I tracked down the infinite loop. The function dataptr resides at 0x4a5194 (see bottom of gdb output), GET_CALLABLE("dataptr") returns this same address, then calling it.

(gdb) bt
#0  dataptr (x=0x92b848) at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/routines.h:199
#1  0x00000000004ac546 in Rcpp::internal::r_vector_start<14> (x=0x92b848) at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/r_vector.h:32
#2  0x00000000004af3cf in Rcpp::internal::primitive_range_wrap__impl__nocast<__gnu_cxx::__normal_iterator<double const*, std::vector<double, std::allocator<double> > >, double> (first=512912162, 
    last=8.7449619313900638e-322) at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:119
#3  0x00000000004af358 in Rcpp::internal::primitive_range_wrap__impl<__gnu_cxx::__normal_iterator<double const*, std::vector<double, std::allocator<double> > >, double> (first=512912162, 
    last=8.7449619313900638e-322) at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:161
#4  0x00000000004af32a in Rcpp::internal::range_wrap_dispatch___impl<__gnu_cxx::__normal_iterator<double const*, std::vector<double, std::allocator<double> > >, double> (first=512912162, 
    last=8.7449619313900638e-322) at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:173
#5  0x00000000004af2fc in Rcpp::internal::range_wrap_dispatch<__gnu_cxx::__normal_iterator<double const*, std::vector<double, std::allocator<double> > >, double> (first=512912162, 
    last=8.7449619313900638e-322) at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:418
#6  0x00000000004af28e in Rcpp::internal::range_wrap<__gnu_cxx::__normal_iterator<double const*, std::vector<double, std::allocator<double> > > > (first=512912162, last=8.7449619313900638e-322)
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:428
#7  0x00000000004af134 in Rcpp::internal::wrap_range_sugar_expression<std::vector<double, std::allocator<double> > > (object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:543
#8  0x00000000004af07b in Rcpp::internal::wrap_dispatch_unknown_iterable__logical<std::vector<double, std::allocator<double> > > (object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:551
#9  0x00000000004aefee in Rcpp::internal::wrap_dispatch_unknown_iterable__matrix_interface<std::vector<double, std::allocator<double> > > (object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:559
#10 0x00000000004aef64 in Rcpp::internal::wrap_dispatch_unknown_iterable<std::vector<double, std::allocator<double> > > (object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:661
#11 0x00000000004aea95 in Rcpp::internal::wrap_dispatch_unknown<std::vector<double, std::allocator<double> > > (object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:732
#12 0x00000000004ae6dd in Rcpp::internal::wrap_dispatch_eigen<std::vector<double, std::allocator<double> > > (object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:770
#13 0x00000000004ad9fb in Rcpp::internal::wrap_dispatch_unknown_importable<std::vector<double, std::allocator<double> > > (object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:787
#14 0x00000000004ac314 in Rcpp::internal::wrap_dispatch<std::vector<double, std::allocator<double> > > (object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:807
#15 0x00000000004aa6d1 in Rcpp::wrap<std::vector<double, std::allocator<double> > > (object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap_end.h:30
#16 0x00000000004a8276 in Rcpp::BindingPolicy<Rcpp::Environment_Impl<Rcpp::PreserveStorage> >::Binding::operator=<std::vector<double, std::allocator<double> > > (this=0x7fffffffe320, 
    rhs=std::vector of length 15, capacity 15 = {...}) at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/proxy/Binding.h:54
#17 0x000000000049e26f in main (argc=2, argv=0x7fffffffe4c8) at [*hidden*]/main.cpp:131
(gdb) p dataptr
$15 = {void *(SEXP)} 0x4a5194 <dataptr(SEXPREC*)>
(gdb) p fun
$16 = (Fun) 0x4a5194 <dataptr(SEXPREC*)>
(gdb) f
#0  dataptr (x=0x92b848) at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/routines.h:199
199     return fun(x) ;

What's wrong here? I don't get it :(

kevinushey commented 10 years ago

Best guess: something weird is happening in the wrap logic, which happens behind the scenes when you try to assign an vector<double> to the RInside object (or, more precisely, the proxy generated by R["data"].

Note that the vector lengths are wonky between lines 6 and 7:

#6  <...snip...> (first=512912162, last=8.7449619313900638e-322)
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:428
#7  <...snip...>(object=std::vector of length 15, capacity 15 = {...})
    at /usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/wrap.h:543

My suggestion is to explicitly make a NumericVector and then assign that, e.g.

NumericVector nv = wrap(data);
R["data"] = nv;

or maybe just explicitly doing the wrap yourself might help:

R["data"] = wrap(data);

But otherwise we can try to investigate a bit more.

eddelbuettel commented 10 years ago

Kevin just beat me to suggesting something related. See inst/examples/standard/*cpp where several files do use std::vector<> containers. Something is going amiss here with the automatic conversion. So try a more defensive approach first while we figure out what is amiss.

eddelbuettel commented 10 years ago

Using a complete program, I see no issue here. I changed the for loop to compile with defaults as we have not plugins for RInside.

// -*- mode: C++; c-indent-level: 4; c-basic-offset: 4; indent-tabs-mode: nil; -*-
//
// cf https://github.com/RcppCore/Rcpp/issues/178

#include <RInside.h>                    // for the embedded R via RInside

int main(int argc, char *argv[]) {

    try {

        RInside R(argc, argv);          // create an embedded R instance 

        std::vector<double> data(15);
        //for_each(data.begin(), data.end(), [](double &val) { val = rand(); });
        for (int i=0; i<15; i++) data[i] = rand();
        R["data"] = data;

        R.parseEvalQ("print(summary(data))");

    } catch(std::exception& ex) {
        std::cerr << "Exception caught: " << ex.what() << std::endl;
    } catch(...) {
        std::cerr << "Unknown exception caught" << std::endl;
    }

    exit(0);
}

which yields on my (Ubuntu 14.04) system (with ccache):

edd@max:~/git/rinside/inst/examples/standard(master)$ make rinside_issue178
ccache g++-4.8 -I/usr/share/R/include -I/usr/local/lib/R/site-library/Rcpp/include -I/usr/local/lib/R/site-library/RInside/include -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -g -O3 -Wall -pipe -Wno-unused -pedantic -Wall    rinside_issue178.cpp  -Wl,--export-dynamic -fopenmp  -L/usr/lib/R/lib -lR -lpcre -llzma -lbz2 -lz -lrt -ldl -lm  -lblas -llapack  -L/usr/local/lib/R/site-library/RInside/lib -lRInside -Wl,-rpath,/usr/local/lib/R/site-library/RInside/lib -o rinside_issue178
edd@max:~/git/rinside/inst/examples/standard(master)$ ./rinside_issue178 
     Min.   1st Qu.    Median      Mean   3rd Qu.      Max. 
8.588e+07 4.596e+08 6.948e+08 9.573e+08 1.575e+09 1.955e+09 
edd@max:~/git/rinside/inst/examples/standard(master)$ 

So I think we can close this.

kevinushey commented 10 years ago

I tried your example too (with -std=c++11 to double-check; thanks for putting it together) and worked fine for me. So I think we can close this as well.

@nixn, please re-open this if you can supply a full reproducible example (and please include your R sessionInfo() as well, so we know what system + versions of libraries you have).

nixn commented 10 years ago

Thanks for your input. It must be some environmental influence or something then. If the problem persists, I will try to give you more information on it.

eddelbuettel commented 10 years ago

I independently confirmed this under -std=c++11 as well, so closing this now.

nixn commented 10 years ago

Finally I found the cause of this error. I was compiling my code with a cmake-generated Makefile, which adds -rdynamic to g++ for linking. Without -rdynamic everything seems to work fine. But I don't understand (yet), what the implications of -rdynamic are and why the code then fails.

tkluck commented 9 years ago

I ran into this same issue: I got a stack overflow when calling one of the functions in Rcpp/routines.h when I was linking with --export-dynamic. I disabled that for now, but it probably makes sense to reopen this issue?

If that is helpful, I could write a minimal test case for you.

eddelbuettel commented 9 years ago

"Reopen" ? Why?

As best as I can tell, both @nixn and you (@tkluck) got an error after you overrode the package defaults and ended up using an inappropriate linker flag. I do not see our error in that. Am I missing something?

tkluck commented 9 years ago

Thanks for replying, @eddelbuettel ! In my case at least, the error occurred not because I compiled Rcpp with that compile flag (I didn't), but because I compiled my own program with that flag, and linked it against RInside. As it is, I didn't really need that flag, so I could easily remove it. But it's not hard to imagine someone actually needed it. That's why I suggest reopening it.

eddelbuettel commented 9 years ago

I still don't follow. You say: "I compiled my own program with that flag, and linked it against RInside"

How do you want me to stop you or others from doing so? It's your error, and once you stopped doing it things worked.

nixn commented 9 years ago

@eddelbuettel This way you exclude all people who need this flag in the linker (for their own program) from using RInside. I can't imagine you really want that.

tkluck commented 9 years ago

I'm not sure why you consider it an error to compile with that flag? What if I link Rcpp into my own shared library whose symbols I want to export dynamically?

Of course, it's a solution to just document that you don't support this. But I respectfully disagree that this is a user error.

tkluck commented 9 years ago

Example: if you ask R how it wants you to link against it, it will tell you to use --export-dynamic:

tkluck@ultrabook-tjk:~$ /usr/bin/R CMD config --ldflags
-Wl,--export-dynamic -fopenmp  -L/usr/lib/R/lib -lR -lpcre -llzma -lbz2 -lz -lrt -ldl -lm
eddelbuettel commented 9 years ago

Agreed on R CMD config --ldflags, it does the same on my box.

That would make it an R issue, and not an Rcpp issue, no?

I am still lost as to why and where you see a bug with WHAT exactly? I am spending this morning, when I am not lost in this discussion, going over Rcpp build tests against its reverse dependies, and I am not having any issues.

Could you be so kind and re-explain to me a) what fails, and b) why that is Rcpp's fault?

eddelbuettel commented 9 years ago

And as RInside was brought up:

edd@max:~/git/rinside/inst/examples/standard(master)$ rm rinside_sample0
edd@max:~/git/rinside/inst/examples/standard(master)$ make rinside_sample0
ccache g++-4.8 -I/usr/share/R/include -I/usr/local/lib/R/site-library/Rcpp/include -I/usr/local/lib/R/site-library/RInside/include -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -g -O3 -Wall -pipe -Wno-unused -pedantic -Wall    rinside_sample0.cpp  -Wl,--export-dynamic -fopenmp  -L/usr/lib/R/lib -lR -lpcre -llzma -lbz2 -lz -lrt -ldl -lm  -lblas -llapack  -L/usr/local/lib/R/site-library/RInside/lib -lRInside -Wl,-rpath,/usr/local/lib/R/site-library/RInside/lib -o rinside_sample0
edd@max:~/git/rinside/inst/examples/standard(master)$ ./rinside_sample0
Hello, world!
edd@max:~/git/rinside/inst/examples/standard(master)$ 

I am not seeing failures here either.

tkluck commented 9 years ago

Sorry for taking a while to get back to you, @eddelbuettel . I wrote a minimal example to exhibit this error. Usage: run it and watch the test program segfault due to stack overflow. Uncomment any of the two suggested fixes and run again: watch how the test program doesn't segfault.

The tricky part is that the problem does not occur with Debian's R package. That's why the "minimal example" actually needs to compile R from scratch.

It does not occur with the Debian package because it compiles R with -Wl,-Bsymbolic-functions. That's just a global option for all debian packages, coming from dpkg-buildflags --get LDFLAGS. In other words, if you compile R with the "default" build options, the problem does occur.

The reasons why I think this is an Rcpp issue (as opposed to a user error) are (1) these build options can be necessary for real use cases, and it makes Rcpp less useful if it doesn't support them, and (2) because Rcpp is doing funky business in Rcpp/routines.h, defining the same function differently depending on what's being compiled.

<guesswork>It looks like the code in Rcpp/routines.h assumes that inline ensures that a functions doesn't make it to the symbol table, which isn't true. It could even be a coincidence that this often works: it might depend on compiler/linker implementation details which of the two symbols is retrieved when calling R_GetCCallable.</guesswork>

As I said, I don't really need this to be fixed at the moment: --no-export-dynamic is an adequate workaround for me. I just spent some time on documenting this because I'm sure it will be helpful for other people like @nixn and me in the future.

#!/bin/bash

set -e

# one fix: compile R with -Wl,-Bsymbolic-functions, like debian does
#R_CONFIGURE_ARGS="LDFLAGS=-Wl,-Bsymbolic-functions"
# another fix: compile our test program without --export-dynamic
#MY_LD_FLAGS="-Wl,--no-export-dynamic"

test -f R-3.1.2.tar.gz || wget http://cran.r-project.org/src/base/R-3/R-3.1.2.tar.gz
tar -xf R-3.1.2.tar.gz
cd R-3.1.2 && ./configure --enable-R-shlib --with-blas --with-lapack $R_CONFIGURE_ARGS && make -j3 && sudo make install
R="`which R`"
RSCRIPT="`which Rscript`"

test -f Rcpp_0.11.3.tar.gz || wget http://cran.r-project.org/src/contrib/Rcpp_0.11.3.tar.gz
test -f RInside_0.2.11.tar.gz || wget http://cran.r-project.org/src/contrib/RInside_0.2.11.tar.gz

sudo "$R" CMD INSTALL Rcpp_0.11.3.tar.gz
sudo "$R" CMD INSTALL RInside_0.2.11.tar.gz

cat > rcpp-stackoverflow.cpp <<CPP
#include <RInside.h>
#include <Rcpp.h>

int main(int argc, char *argv[]) {
    RInside R(argc, argv);
    reset_current_error();
    return 0;
}
CPP

g++ -o rcpp-stackoverflow \
`"$RSCRIPT" -e 'cat(Rcpp:::CxxFlags())'` \
`"$R" CMD config --cppflags` \
-I/usr/local/lib/R/site-library/RInside/include \
rcpp-stackoverflow.cpp \
-lRInside \
`"$R" CMD config --ldflags` \
`"$RSCRIPT" -e 'cat(Rcpp:::LdFlags())'` \
-L/usr/local/lib/R/site-library/RInside/lib \
-Wl,-rpath,/usr/local/lib/R/site-library/RInside/lib \
-Wl,-rpath,/usr/local/lib/R/lib \
$MY_LD_FLAGS

# will segfault because of stack overflow, unless one of the fixes noted at the
# top of this file is applied
./rcpp-stackoverflow
eddelbuettel commented 9 years ago

I appreciate the follow-up. I see more clearly where you are coming from.

I am not so sure where, if anywhere, this is a bug. If I understand you correctly, this in an RInside issue but you claim it ought to be fixed at the Rcpp level.

Now, Rcpp is a "pure" client of R. We cannot change how R is built. So the best I think we can here is maybe document this somewhere.

I am not quite sure where though.

tkluck commented 9 years ago

Thanks for your quick reply! Yes, I agree that this is a tricky one.

It looks like the entire issue could be avoided by modifying Rcpp/routines.h to not overload the symbol name reset_current_error for both an "internal" version and the "api" version. Something like this might do the trick (haven't tried it, and it would also have to be done for the other functions in routines.h):

diff --git a/inst/include/Rcpp/routines.h b/inst/include/Rcpp/routines.h
index 862201f..877556b 100644
--- a/inst/include/Rcpp/routines.h
+++ b/inst/include/Rcpp/routines.h
@@ -53,7 +53,7 @@ const char* char_nocheck( SEXP x) ;
 void* dataptr(SEXP x) ;
 Rcpp::Module* getCurrentScope() ;
 void setCurrentScope( Rcpp::Module* mod ) ;
-SEXP reset_current_error() ;
+SEXP _reset_current_error_internal() ;
 int error_occured() ;
 SEXP rcpp_get_current_error() ;

@@ -219,7 +219,7 @@ inline int* get_cache( int n ){

 inline SEXP reset_current_error(){
     typedef SEXP (*Fun)(void) ;
-    static Fun fun = GET_CALLABLE("reset_current_error") ;
+    static Fun fun = GET_CALLABLE("_reset_current_error_internal") ;
     return fun() ;
 }

diff --git a/src/Rcpp_init.cpp b/src/Rcpp_init.cpp
index 71a4559..0f36590 100644
--- a/src/Rcpp_init.cpp
+++ b/src/Rcpp_init.cpp
@@ -113,7 +113,7 @@ void registerFunctions(){
     RCPP_REGISTER(short_file_name)
     RCPP_REGISTER(mktime00)
     RCPP_REGISTER(gmtime_)
-    RCPP_REGISTER(reset_current_error)
+    RCPP_REGISTER(_reset_current_error_internal)
     RCPP_REGISTER(error_occured)
     RCPP_REGISTER(rcpp_get_current_error)
     #undef RCPP_REGISTER
diff --git a/src/barrier.cpp b/src/barrier.cpp
index 1344371..ffe7cca 100644
--- a/src/barrier.cpp
+++ b/src/barrier.cpp
@@ -153,7 +153,7 @@ SEXP init_Rcpp_cache(){
 }

 // [[Rcpp::register]]
-SEXP reset_current_error(){
+SEXP _reset_current_error_internal(){
     SEXP cache = get_rcpp_cache() ;

     // error occured
eddelbuettel commented 9 years ago

First, is it possible that you are misunderstand the conditional compilation there? There is both use for Rcpp itself as well Rcpp being used by others. We do that with a lot of other functions.

Second, what would renaming accomplish?

Third, you missed two other instances:

edd@max:~/git/rcpp(master)$ ag reset_current_error     # ag is grep -r on steroids
inst/include/Rcpp/routines.h
56:SEXP reset_current_error() ;
220:inline SEXP reset_current_error(){
222:    static Fun fun = GET_CALLABLE("reset_current_error") ;

inst/include/Rcpp/api/meat/Rcpp_eval.h
26:        reset_current_error() ;

src/barrier.cpp
156:SEXP reset_current_error(){

src/Rcpp_init.cpp
116:    RCPP_REGISTER(reset_current_error)
edd@max:~/git/rcpp(master)$ 
eddelbuettel commented 9 years ago

Lastly, I don't have time or interest to debate this to death. If you want to make changes, fork Rcpp, make the change and prior to a pull request please demonstrate that your change has no side effect with the (as of today) 310 CRAN packages using Rcpp.

To me, this still looks like a purely local deployment issue dependent on how R was built -- over which we have no control.

tkluck commented 9 years ago

Lastly, I don't have time or interest to debate this to death.

That is good, because neither have I :) I'm satisfied with having documented this here.

eddelbuettel commented 9 years ago

:)

So where do you think we need README? This only comes up in RInside, no? What you did here is pretty comprehensive, so should we just add it as a HOWTO vignette (in markdown or latex) to RInside?

tkluck commented 9 years ago

I think it might also come up in other uses of Rcpp, it's just that I didn't know how to initialize Rcpp without RInside, so that's why it's there :)

But since that makes RInside is the only properly documented case, I guess either project's README is fine. Maybe something like

Getting a stack overflow / infinite recursion in one of Rcpp's functions? Adding `-Wl,--no-export-dynamic` might fix that for you. See <link to this issue> for more details.

How does that sound to you?