-
-
Notifications
You must be signed in to change notification settings - Fork 218
Mask definition of Rf_error to avoid longjmp issues #1402
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for putting this together! I can (and will) turn on the reverse-depends machinery but as that does not generally involve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good. I only have two "style" questions / comments. Maybe @kevinushey can be the referee :)
|
Ok, new solution with a bit of macro trickery. |
|
Wheeee ==:-) Almost worse 😜 That should work. 🤞 |
|
Mmmh, maybe I should put that into an ifdef... |
|
Ok, it should be ready now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me so far.
|
Thanks, let's wait for another pair of eyes and the revdep machine then. Meanwhile, I'll investigate how many packages need to be adapted to avoid this warning. |
|
Should we include a reference to |
|
Thumbs up on more informative message. 'That file' is technically a policy violation anyway (via a trick borrowed, if memory serves, from another posit package) and not something I get paid to care about anymore. |
|
What about something like... "Use of Rf_error() replaced with Rcpp::stop(). Calls to Rf_error() in C++ contexts are unsafe: consider using Rcpp::stop() instead, or define RCPP_NO_MASK_RF_ERROR if this is a false positive." A bit long, but... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you both
|
Ok, some first results are in this log file. It looks worse than it likely is as a) there are the usual 'new' packages for which I have to add their dependencies in a follow-up run and b) the packages already on 'deadline' with known-to-CRAN issues not from us. And then there is the remainder. I will take a closer look at those but it may take me another day or so. I have logs here to follow-up with. |
|
Hm. And I mean Hmm here. An incremental run (after adding all the packages listed as missing, and running against everything not passing in first one, i.e. those who were skipped, who failed, or are new) still has a lot of failures: I looked at two: dqrngRcppSimdJsonResults table log is here. |
|
Mind you maybe it is just like @Enchufa2 hinted: a simple edd@paul:~/git/rcppsimdjson(master)$ tt.r -f inst/tinytest/test_fparse_fload.R
test_fparse_fload.R........... 8 tests OK terminate called after throwing an instance of 'Rcpp::exception'
what(): TAPE_ERROR: The JSON document has an improper structure: missing or superfluous commas, braces, missing keys, etc. This is a fatal and unrecoverable error.
Aborted (core dumped)
edd@paul:~/git/rcppsimdjson(master)$ compAttr.r
edd@paul:~/git/rcppsimdjson(master)$ install.r
* installing *source* package found in current working directory ...
* installing *source* package ‘RcppSimdJson’ ...
** this is package ‘RcppSimdJson’ version ‘0.1.14’
** using staged installation
** libs
using C++ compiler: ‘g++-15 (Ubuntu 15-20250404-0ubuntu1) 15.0.1 20250404 (experimental) [master r15-9193-g08e803aa9be]’
using C++17
ccache g++-15 -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG -I'/usr/local/lib/R/site-library/Rcpp/include' -DSIMDJSON_NO_COMPUTED_GOTO -I../inst/include -fopenmp -fpic -O3 -Wall -pipe -pedantic -Wno-parentheses -Wno-ignored-attributes -Wno-unused-function -c RcppExports.cpp -o RcppExports.o
ccache g++-15 -std=gnu++17 -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -Wl,-z,relro -o RcppSimdJson.so RcppExports.o deserialize.o exported-utils.o internal-utils.o rcppsimdjson_utils_check.o simdjson_example.o -fopenmp -L/usr/lib/R/lib -lR
installing to /usr/local/lib/R/site-library/00LOCK-rcppsimdjson/00new/RcppSimdJson/libs
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
** checking absolute paths in shared objects and dynamic libraries
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (RcppSimdJson)
edd@paul:~/git/rcppsimdjson(master)$ tt.r -f inst/tinytest/test_fparse_fload.R
test_fparse_fload.R........... 2666 tests OK 4.2s
All ok, 2666 results (4.2s)
edd@paul:~/git/rcppsimdjson(master)$ PS And no side effects. After re-installing CRAN Rcpp, and reinstalling RcppSimdJson under it (and with its updated |
9fee4c9 to
a941564
Compare
a941564 to
40f2fca
Compare
|
Rebased, and ran another incremental run. Results summary in this commit: RcppCore/rcpp-logs@ef3a42f containing the summary file. This is a bit more involved than usual:
We probably need a new issue to triage. May revisit tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the newline
|
Ok -- we are in fact having issues here. The Edit: Oh boy that one was work. I got #include <RcppEigen.h>
using namespace Rcpp;
using namespace RcppEigen;
using namespace Eigen;and re-running See the next paragraph inside the 'details' though as running Edit 2: For completeness the very short diff for This is an updated diff. As @Enchufa2 pointed out we really only need to remove the root@c95b60d71856:/tmp/checks/BayesProject# diff -ru
data/ DESCRIPTION man/ MD5 NAMESPACE R/ src/
root@c95b60d71856:/tmp/checks/BayesProject# cd ..
root@c95b60d71856:/tmp/checks# diff -ru BayesProject.orig/ BayesProject
diff -ru BayesProject.orig/src/bayes.cpp BayesProject/src/bayes.cpp
--- BayesProject.orig/src/bayes.cpp 2020-06-01 00:45:46.000000000 +0000
+++ BayesProject/src/bayes.cpp 2025-11-05 18:58:16.780202588 +0000
@@ -1,4 +1,3 @@
-#include <R.h>
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>
root@c95b60d71856:/tmp/checks# The older / longer diff follows. root@c95b60d71856:/tmp/checks# diff -Nru BayesProject.orig/ BayesProject
diff -Nru BayesProject.orig/inst/include/BayesProject_types.h BayesProject/inst/include/BayesProject_types.h
--- BayesProject.orig/inst/include/BayesProject_types.h 1970-01-01 00:00:00.000000000 +0000
+++ BayesProject/inst/include/BayesProject_types.h 2025-11-05 12:51:23.874653154 +0000
@@ -0,0 +1,6 @@
+
+#include <RcppEigen.h>
+using namespace Rcpp;
+using namespace RcppEigen;
+using namespace Eigen;
+
diff -Nru BayesProject.orig/src/bayes.cpp BayesProject/src/bayes.cpp
--- BayesProject.orig/src/bayes.cpp 2020-06-01 00:45:46.000000000 +0000
+++ BayesProject/src/bayes.cpp 2025-11-05 12:40:49.344415413 +0000
@@ -1,4 +1,4 @@
-#include <R.h>
+/*#include <R.h>*/
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>
diff -Nru BayesProject.orig/src/Makevars BayesProject/src/Makevars
--- BayesProject.orig/src/Makevars 1970-01-01 00:00:00.000000000 +0000
+++ BayesProject/src/Makevars 2025-11-05 12:42:28.491159649 +0000
@@ -0,0 +1 @@
+PKG_CXXFLAGS += -Wno-ignored-attributes
diff -Nru BayesProject.orig/src/RcppExports.cpp BayesProject/src/RcppExports.cpp
--- BayesProject.orig/src/RcppExports.cpp 2020-09-27 00:05:44.000000000 +0000
+++ BayesProject/src/RcppExports.cpp 2025-11-05 12:45:33.389215971 +0000
@@ -1,11 +1,16 @@
// Generated by using Rcpp::compileAttributes() -> do not edit by hand
// Generator token: 10BE3573-1514-4C36-9D1C-5A225CD40393
+#include "../inst/include/BayesProject_types.h"
#include <RcppEigen.h>
#include <Rcpp.h>
using namespace Rcpp;
-using namespace Eigen;
+
+#ifdef RCPP_USE_GLOBAL_ROSTREAM
+Rcpp::Rostream<true>& Rcpp::Rcout = Rcpp::Rcpp_cout_get();
+Rcpp::Rostream<false>& Rcpp::Rcerr = Rcpp::Rcpp_cerr_get();
+#endif
// bayes_vhat
MatrixXd bayes_vhat(MatrixXd x, VectorXd timePoints, double K);
root@c95b60d71856:/tmp/checks#The Edit 3: Similar for root@c95b60d71856:/tmp/checks# diff -Nru GeneralizedWendland.orig/ GeneralizedWendland
diff -Nru GeneralizedWendland.orig/inst/doc/GeneralizedWendland.pdf.asis GeneralizedWendland/inst/doc/GeneralizedWendland.pdf.asis
--- GeneralizedWendland.orig/inst/doc/GeneralizedWendland.pdf.asis 2022-06-16 21:46:11.000000000 +0000
+++ GeneralizedWendland/inst/doc/GeneralizedWendland.pdf.asis 1970-01-01 00:00:00.000000000 +0000
@@ -1,2 +0,0 @@
-%\VignetteIndexEntry{Generalized Wendland function}
-%\VignetteEngine{R.rsp::asis}
\ No newline at end of file
diff -Nru GeneralizedWendland.orig/src/Wendland.h GeneralizedWendland/src/Wendland.h
--- GeneralizedWendland.orig/src/Wendland.h 2025-10-15 19:02:57.000000000 +0000
+++ GeneralizedWendland/src/Wendland.h 2025-11-05 13:14:59.923085742 +0000
@@ -2,7 +2,7 @@
#pragma once
#include <limits>
-#include <R.h>
+//#include <R.h>
#include <Rcpp.h>
#include <RcppEigen.h>
#include <boost/math/special_functions/beta.hpp>
root@c95b60d71856:/tmp/checks#
The vignette business can probably be ignored, I am working in a container without texlive and just skip vignettes. Edit 4: Same thing in A more minimal diff is root@c95b60d71856:/tmp/checks# diff -ru locStra.orig/ locStra
diff -ru locStra.orig/src/auxcode.cpp locStra/src/auxcode.cpp
--- locStra.orig/src/auxcode.cpp 2021-01-27 02:29:32.000000000 +0000
+++ locStra/src/auxcode.cpp 2025-11-05 19:04:15.956493072 +0000
@@ -1,4 +1,3 @@
-#include <R.h>
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>
root@c95b60d71856:/tmp/checks# The initial, longer one follows. root@c95b60d71856:/tmp/checks# diff -Nru locStra.orig/ locStra
diff -Nru locStra.orig/inst/include/locStra_types.h locStra/inst/include/locStra_types.h
--- locStra.orig/inst/include/locStra_types.h 1970-01-01 00:00:00.000000000 +0000
+++ locStra/inst/include/locStra_types.h 2025-11-05 13:45:56.648854713 +0000
@@ -0,0 +1,3 @@
+
+#include <RcppEigen.h>
+using namespace Eigen;
diff -Nru locStra.orig/src/auxcode.cpp locStra/src/auxcode.cpp
--- locStra.orig/src/auxcode.cpp 2021-01-27 02:29:32.000000000 +0000
+++ locStra/src/auxcode.cpp 2025-11-05 13:44:20.429326159 +0000
@@ -1,4 +1,4 @@
-#include <R.h>
+//#include <R.h>
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>
diff -Nru locStra.orig/src/RcppExports.cpp locStra/src/RcppExports.cpp
--- locStra.orig/src/RcppExports.cpp 2021-01-27 02:46:27.000000000 +0000
+++ locStra/src/RcppExports.cpp 2025-11-05 13:43:34.431117382 +0000
@@ -1,11 +1,16 @@
// Generated by using Rcpp::compileAttributes() -> do not edit by hand
// Generator token: 10BE3573-1514-4C36-9D1C-5A225CD40393
+#include "../inst/include/locStra_types.h"
#include <RcppEigen.h>
#include <Rcpp.h>
using namespace Rcpp;
-using namespace Eigen;
+
+#ifdef RCPP_USE_GLOBAL_ROSTREAM
+Rcpp::Rostream<true>& Rcpp::Rcout = Rcpp::Rcpp_cout_get();
+Rcpp::Rostream<false>& Rcpp::Rcerr = Rcpp::Rcpp_cerr_get();
+#endif
// powerMethodCpp
VectorXd powerMethodCpp(MatrixXd& X, VectorXd& v, double eps, int maxiter);
root@c95b60d71856:/tmp/checks# Edit 5: Same for Edit 6: Same for root@c95b60d71856:/tmp/checks# diff -ru TDA.orig/ TDA
Only in TDA.orig/build: vignette.rds
Only in TDA.orig/inst: doc
diff -ru TDA.orig/src/diag.cpp TDA/src/diag.cpp
--- TDA.orig/src/diag.cpp 2024-01-23 16:07:14.000000000 +0000
+++ TDA/src/diag.cpp 2025-11-05 19:16:24.525589991 +0000
@@ -1,6 +1,6 @@
// for R
-#include <R.h>
-#include <R_ext/Print.h>
+//#include <R.h>
+//#include <R_ext/Print.h>
// for Rcpp
#include <Rcpp.h>
root@c95b60d71856:/tmp/checks# |
|
I believe this is a bug in RcppEigen. I see: If you protect that |
Yes/no/maybe/unsure/I don't care: It goes away if you do not include A related issue is that we should probably warn when users include |
|
Ah, ok, I thought it was RcppEigen who was including |
|
I am very confused as to why I now need the It's the same as always: Also, to be more precise: |
Ah, because your are running My point was that the package, as is, can be fixed for this PR just by removing the include: diff --git a/src/bayes.cpp b/src/bayes.cpp
index c856b42..b9e5e65 100644
--- a/src/bayes.cpp
+++ b/src/bayes.cpp
@@ -1,4 +1,3 @@
-#include <R.h>
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>How the client package deals with flatten namespaces or not it's their problem.
I think we should at least warn now and at some point throw an error. |
|
Thumbs up on more minimal diffs. That may affect more than one package. Needing an Agreed on that adding a warning is probably a good. |
|
|
It's been a while. We needed it then for finetune wrappers and forwarding, it's been like this ever since. I think I should add a warning to RcppEigen. Edit Issue opened at its repo. |
Closes #1247. It turns out we are generating a call to
Rf_errorinRcppExports.cppfor C++ interfaces. I think that code path is obsolete, and it should never run with unwind-protect, but I prefer to keep this PR safe and simple. So to be able to undef, callRf_errorthen define again, I put the definition into a separate file without guards, so that it can be included more than once. Please let me know if there's a better way and/or you'd like this definition in another place.Checklist
R CMD checkstill passes all tests