abort() called when passing NULL as parameter

[jrgfogh]

R crashes when I pass NULL to a function taking a StringVector, when the function is compiled as part of a package.

I have tracked the cause to r_cast.h in r_true_cast<>():

#ifndef NDEBUG
            REprintf(fmt, Rf_type2char(TYPEOF(x)));
            abort();
#else
            throw ::Rcpp::not_compatible(fmt, Rf_type2char(TYPEOF(x)));
#endif

The issue does not occur when I compile the same function using cppFunction() or sourceCpp(), presumably because these
functions define NDEBUG.

MCVE:

Run:

library(usethis)

create_package('tmp')
use_rcpp()

Paste the following code into src/code.cpp:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
RObject crash(StringVector v)
{
    auto x = 10;
    return wrap(x);
}

Paste the following code into R/tmp-package.R:

## usethis namespace: start
#' @exportPattern "^[^\\.]"
#' @useDynLib tmp, .registration = TRUE
#' @importFrom Rcpp sourceCpp
## usethis namespace: end
NULL

Now run:

devtools::document()
devtools::load_all('.')
tmp::crash(NULL)

Output from sessionInfo():

R version 4.0.2 (2020-06-22)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 18363)

Matrix products: default

locale:
[1] LC_COLLATE=Danish_Denmark.1252  LC_CTYPE=Danish_Denmark.1252    LC_MONETARY=Danish_Denmark.1252
[4] LC_NUMERIC=C                    LC_TIME=Danish_Denmark.1252    

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

other attached packages:
[1] usethis_1.6.1        RevoUtils_11.0.2     RevoUtilsMath_11.0.0

loaded via a namespace (and not attached):
 [1] Rcpp_1.0.5       rstudioapi_0.11  knitr_1.29       xml2_1.3.2       magrittr_1.5     roxygen2_7.1.1   R6_2.3.0        
 [8] rlang_0.4.6      fansi_0.4.1      stringr_1.4.0    tools_4.0.2      xfun_0.15        cli_2.0.2        git2r_0.27.1    
[15] ellipsis_0.3.1   rprojroot_1.3-2  assertthat_0.2.1 tibble_3.0.2     lifecycle_0.2.0  crayon_1.3.4     purrr_0.3.4     
[22] vctrs_0.3.1      fs_1.4.2         glue_1.4.1       stringi_1.4.6    compiler_4.0.2   pillar_1.4.6     desc_1.2.0      
[29] backports_1.1.7  pkgconfig_2.0.3

[Enchufa2]

In order to isolate variables, could you try with an updated library? Especially with an up-to-date version of Rcpp, because I see you have an old one. I cannot reproduce the issue on Linux, I see

> tmp::crash(NULL)
Error in tmp::crash(NULL) : Not compatible with STRSXP: [type=NULL].

as expected.

[Enchufa2]

Oh, but the Linux build has NDEBUG defined, of course. Without NDEBUG:

> tmp::crash(NULL)
Not compatible with STRSXP: [type=NULL].Aborted (core dumped)

I see no segfault or other kind of crashes. The program just aborts, which is expected given the call to abort() in that code path, which is in turn intended for debugging.

[jrgfogh]

My understanding is that abort() should only be called when a supposedly unreachable code path is reached.
In this case, the code path is reachable by passing in NULL directly from R.

[Enchufa2]

The code path should be unreachable, because NDEBUG should be set. Here's a function in a CRAN package that accepts a StringVector as input. Package installed from CRAN in a Windows machine:

> colourvalues:::rcpp_convert_hex_to_rgb(NULL)
Error in colourvalues:::rcpp_convert_hex_to_rgb(NULL) :
  Not compatible with STRSXP: [type=NULL].

which means that CRAN correctly sets NDEBUG for Windows builds too.

[eddelbuettel]

@jrgfogh Your example is also not minimal. I don't use the wrappers you deployed; these are sometimes trying to be extra helpful so that they end up not being standard by defining other tools and helpers which then create a dependency on them (NOT_CRAN being an example). Maybe this tool chain makes change affecting NDEBUG?

For what it is worth, my system has /etc/R/Makeconf (a convenience softlink to R's file, all goes back to the Debian package I maintain and whose structure drives the Ubuntu binary I run here) and it knows it:

edd@rob:~$ grep NDEBUG /etc/R/Makeconf 
R_XTRA_CPPFLAGS =  -I"$(R_INCLUDE_DIR)" -DNDEBUG
ALL_CPPFLAGS =  -I"$(R_INCLUDE_DIR)" -DNDEBUG $(PKG_CPPFLAGS) $(CLINK_CPPFLAGS) $(CPPFLAGS) 
edd@rob:~$ 

When I then re-create your demo only using standard R CMD ... steps as well as one extrememly minimal wrappers I wrote (here: pkgKitten::kitten to call the skeleton generator) then nothing bad happens, just as @Enchufa2 already demonstrated.

Could you have an issue that arose from using usethis and devtools ?

[Enchufa2]

In other words, @jrgfogh, NDEBUG controls the presence of asserts, and you can see the code path above as a manual assert (write something, call abort). Therefore, you need to ensure that your configuration/toolchain is not stripping out that flag for non-debug builds. So if there's nothing else from the Rcpp side, I think we should close this.

[jrgfogh]

I understand that. However, switching off an assert doesn't solve the problem that the thing being asserted still isn't true.

[Enchufa2]

Sorry, I'm not following you here.

[jrgfogh]

The point of an assert is to ensure that some invariant isn't broken. In this case, that a certain function is never called with NULL. But the function can be called with NULL. Switching off assertions does not change this.

[Enchufa2]

The point of an assert is to ensure that some invariant isn't broken. In this case, that a certain function is never called with NULL.

Which is exactly the case here in debugging mode for testing, when we are not supposed to feed wrong types.

But the function can be called with NULL. Switching off assertions does not change this.

Switching off assertions makes the code handle the case gracefully with an exception that is shown as an informative error message. Of course the function can be called with NULL, this is a programming interface: the function can be called with all sorts of stupid things, and that's why there's an exception in place. I really don't see what you expect Rcpp to do here.

[eddelbuettel]

Also, check Writing R Extensions. Calls such as abort() and exit() are generally not allowed because, when added to a dynamically loaded R extension package like Rcpp or one using Rcpp, they will terminate the calling R process. I am surprised we were allowed to keep that one above / the test must be smart enough to not get triggered. [ Realizing while typing that is a grep on the object code, not the source code, and as we argues tirelessly above, the segment will be #ifdefed away under normal builds. ]

[jrgfogh]

The point of an assert is to ensure that some invariant isn't broken. In this case, that a certain function is never called with NULL.

Which is exactly the case here in debugging mode for testing, when we are not supposed to feed wrong types.

I would argue that feeding in incorrect values is usually the most important part of testing. With the current setup the error handling can not be tested without the NDEBUG flag. Things such as: "Will the error be reported to the user or cause an issue somewhere else?" or "is the error message correct?"

In any case, it seems that we all understand what the code currently does. The question is what, if anything, should be changed. I think the code should behave the same with or without NDEBUG. You are of course free to disagree.

[eddelbuettel]

With all due respect, I think this has run its course.

Code on CRAN cannot have an abort() in the compiled code (under default settings). Here is from Section 1.6 of Writing R Extensions (and quoting from info renderer because of editor prefs, there is html for if you want it)

• Under no circumstances should your compiled code ever call 'abort'
  or 'exit'(11): these terminate the user's R process, quite possibly
  losing all unsaved work. One usage that could call 'abort' is the
  'assert' macro in C or C++ functions, which should never be active
  in production code. The normal way to ensure that is to define the
  macro 'NDEBUG', and 'R CMD INSTALL' does so as part of the
  compilation flags. If you wish to use 'assert' during development.
  you can include '-UNDEBUG' in 'PKG_CPPFLAGS'. Note that your own
  'src/Makefile' or makefiles in sub-directories may also need to
  define 'NDEBUG'.

  This applies not only to your own code but to any external software
  you compile in or link to.

[eddelbuettel]

Before closing this -- did you go back the two PR enabling behaviour on either side of this? This seems to be a clear case

• @coatless adding the more explicit "well what type was it?" to the "wrong type" message (a really useful PR we profit from daily)
• @jackwasey adding a variant to abort() if NDEBUG is not set (your bone of contention here).

I do not from the top of my head recall the context and motivation for Jack's PR but he has my trust for a series of contributions across different projects. So in sum this all seems to work as a) designed and b) permitted by policy and c) not trigger the error you saw (which to the best of my understanding may be cause by your choice of toolchain which happens to be one we do not explicitly mention or endorse or recommended in our documentation).

I may be missing something still. If so, feel free to set me straight.