diff --git a/NEWS.md b/NEWS.md index 755bb991..6eab4cbd 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # cpp11 (development version) +* Fixed an issue where `cpp11::stop()` and `cpp11::warning()` calls with the same template instantiation could cause a crash on some systems (#491, #295). + * `cpp_source()` now works with multiple `file`s (#492). # cpp11 0.5.4 diff --git a/cpp11test/DESCRIPTION b/cpp11test/DESCRIPTION index d1d05665..efde1512 100644 --- a/cpp11test/DESCRIPTION +++ b/cpp11test/DESCRIPTION @@ -1,7 +1,7 @@ Package: cpp11test Title: A test suite and benchmark code for 'cpp11' Version: 0.0.0.9000 -Authors@R: +Authors@R: c(person(given = "Jim", family = "Hester", role = c("aut", "cre"), @@ -21,3 +21,4 @@ Suggests: LazyData: true Roxygen: list(markdown = TRUE) RoxygenNote: 7.1.1 +Config/testthat/edition: 3 diff --git a/cpp11test/R/cpp11.R b/cpp11test/R/cpp11.R index 038e7b76..d23a589b 100644 --- a/cpp11test/R/cpp11.R +++ b/cpp11test/R/cpp11.R @@ -236,6 +236,14 @@ rcpp_push_and_truncate_ <- function(size_sxp) { .Call(`_cpp11test_rcpp_push_and_truncate_`, size_sxp) } +test_template_stop <- function() { + invisible(.Call(`_cpp11test_test_template_stop`)) +} + +test_template_warning <- function() { + invisible(.Call(`_cpp11test_test_template_warning`)) +} + test_destruction_inner <- function() { invisible(.Call(`_cpp11test_test_destruction_inner`)) } diff --git a/cpp11test/src/cpp11.cpp b/cpp11test/src/cpp11.cpp index 0eb9809c..9f813844 100644 --- a/cpp11test/src/cpp11.cpp +++ b/cpp11test/src/cpp11.cpp @@ -444,6 +444,22 @@ extern "C" SEXP _cpp11test_rcpp_push_and_truncate_(SEXP size_sxp) { return cpp11::as_sexp(rcpp_push_and_truncate_(cpp11::as_cpp>(size_sxp))); END_CPP11 } +// template-1-stop.cpp +void test_template_stop(); +extern "C" SEXP _cpp11test_test_template_stop() { + BEGIN_CPP11 + test_template_stop(); + return R_NilValue; + END_CPP11 +} +// template-2-warn.cpp +void test_template_warning(); +extern "C" SEXP _cpp11test_test_template_warning() { + BEGIN_CPP11 + test_template_warning(); + return R_NilValue; + END_CPP11 +} // test-protect-nested.cpp void test_destruction_inner(); extern "C" SEXP _cpp11test_test_destruction_inner() { @@ -534,6 +550,8 @@ static const R_CallMethodDef CallEntries[] = { {"_cpp11test_sum_int_foreach_", (DL_FUNC) &_cpp11test_sum_int_foreach_, 1}, {"_cpp11test_test_destruction_inner", (DL_FUNC) &_cpp11test_test_destruction_inner, 0}, {"_cpp11test_test_destruction_outer", (DL_FUNC) &_cpp11test_test_destruction_outer, 0}, + {"_cpp11test_test_template_stop", (DL_FUNC) &_cpp11test_test_template_stop, 0}, + {"_cpp11test_test_template_warning", (DL_FUNC) &_cpp11test_test_template_warning, 0}, {"_cpp11test_upper_bound", (DL_FUNC) &_cpp11test_upper_bound, 2}, {"run_testthat_tests", (DL_FUNC) &run_testthat_tests, 1}, {NULL, NULL, 0} diff --git a/cpp11test/src/template-1-stop.cpp b/cpp11test/src/template-1-stop.cpp new file mode 100644 index 00000000..14c5d369 --- /dev/null +++ b/cpp11test/src/template-1-stop.cpp @@ -0,0 +1,3 @@ +#include "cpp11/protect.hpp" + +[[cpp11::register]] void test_template_stop() { cpp11::stop("%s", "stop"); } diff --git a/cpp11test/src/template-2-warn.cpp b/cpp11test/src/template-2-warn.cpp new file mode 100644 index 00000000..da5a2e18 --- /dev/null +++ b/cpp11test/src/template-2-warn.cpp @@ -0,0 +1,3 @@ +#include "cpp11/protect.hpp" + +[[cpp11::register]] void test_template_warning() { cpp11::warning("%s", "warning"); } diff --git a/cpp11test/tests/testthat/_snaps/template.md b/cpp11test/tests/testthat/_snaps/template.md new file mode 100644 index 00000000..0ab5c38d --- /dev/null +++ b/cpp11test/tests/testthat/_snaps/template.md @@ -0,0 +1,16 @@ +# equivalently templated `cpp11::stop()` and `cpp11::warning()` can coexist (#491) + + Code + test_template_stop() + Condition + Error: + ! stop + +--- + + Code + test_template_warning() + Condition + Warning: + warning + diff --git a/cpp11test/tests/testthat/test-template.R b/cpp11test/tests/testthat/test-template.R new file mode 100644 index 00000000..4b753beb --- /dev/null +++ b/cpp11test/tests/testthat/test-template.R @@ -0,0 +1,13 @@ +test_that("equivalently templated `cpp11::stop()` and `cpp11::warning()` can coexist (#491)", { + # It is important the the C++ files be named `template-1-stop` and + # `template-2-warn` because the `cpp11::stop()` call needs to be linked in + # before the `cpp11::warning()` call to reproduce the original issue, + # otherwise the templates underlying `cpp11::warning()` will get instantiated + # first and be reused in `cpp11::stop()` via ODR and that "works" fine. + expect_snapshot(error = TRUE, { + test_template_stop() + }) + expect_snapshot({ + test_template_warning() + }) +}) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index 3058ecba..e3b8ce81 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -95,6 +95,52 @@ unwind_protect(Fun&& code) { namespace detail { +// Tag types to force templated `struct closure` and `apply()` infrastructure shared +// across `struct function` and `struct noreturn_function` to generate different +// attribute specific `struct closure` and `apply()` variants. +// +// Consider: +// +// ``` +// cpp11::stop("error: %s", message) +// cpp11::warning("warning: %s", message) +// ``` +// +// These both end up constructing the exact same templated `struct closure` and `apply()` +// functions. The `args` for the underlying `Rf_errorcall()` and `Rf_warningcall()` are: +// - `R_NilValue` +// - `const char* fmt` +// - `const char* message` +// +// The only difference is that `cpp11::stop()` is marked as `[[noreturn]]` because the +// underlying `Rf_errorcall()` is also marked as `[[noreturn]]` / +// `__attribute__((noreturn))`. +// +// But this causes issues! Due to C++'s ODR (One Definition Rule), only 1 variant of +// `apply()` and `struct closure` can be created per template combination. If the +// `cpp11::stop()` variant is linked in first, then some compilers use the `[[noreturn]]` +// hint on `cpp11::stop()` and `operator()` of `noreturn_function` to assert that the +// `apply()` function also cannot return, and returning is deemed unreachable. So then +// when `cpp11::warning()` tries to return from its call to `apply()`, a crash occurs. We +// see this output under ASAN: `execution reached an unreachable program point`. +// +// We've seen this issue on macOS and Linux under clang (gcc does not seem to reproduce +// this). To reproduce, you must have `cpp11::stop()` and `cpp11::warning()` calls in +// different translation units / files and the file containing `cpp11::stop()` must be +// linked first. Putting it first alphabetically seems to be enough, which is why we have +// `template-1-stop.cpp` and `template-2-warn.cpp` in our tests, along with +// `test-template.R` to test this exact issue. You also need to compile with `-O0`, +// otherwise you'll just get a hang rather than a crash. +// +// Adding the tag into the template definition forces `safe[fn]()` and +// `safe.noreturn[fn]()` calls to generate different `apply()` variants, avoiding this +// issue. +// +// https://github.com/r-lib/cpp11/issues/491 +// https://github.com/r-lib/cpp11/issues/295 +struct return_tag {}; +struct no_return_tag {}; + template struct index_sequence { using type = index_sequence; @@ -113,29 +159,30 @@ struct make_index_sequence template <> struct make_index_sequence<0> : index_sequence<> {}; -template +template decltype(std::declval()(std::declval()...)) apply( F&& f, std::tuple&& a, const index_sequence&) { return std::forward(f)(std::get(std::move(a))...); } -template +template decltype(std::declval()(std::declval()...)) apply(F&& f, std::tuple&& a) { - return apply(std::forward(f), std::move(a), make_index_sequence{}); + return apply(std::forward(f), std::move(a), + make_index_sequence{}); } // overload to silence a compiler warning that the (empty) tuple parameter is set but // unused -template +template decltype(std::declval()()) apply(F&& f, std::tuple<>&&) { return std::forward(f)(); } -template +template struct closure { decltype(std::declval()(std::declval()...)) operator()() && { - return apply(ptr_, std::move(arefs_)); + return apply(ptr_, std::move(arefs_)); } F* ptr_; std::tuple arefs_; @@ -149,17 +196,13 @@ struct protect { template decltype(std::declval()(std::declval()...)) operator()(A&&... a) const { // workaround to support gcc4.8, which can't capture a parameter pack - return unwind_protect( - detail::closure{ptr_, std::forward_as_tuple(std::forward(a)...)}); + return unwind_protect(detail::closure{ + ptr_, std::forward_as_tuple(std::forward(a)...)}); } F* ptr_; }; - /// May not be applied to a function bearing attributes, which interfere with linkage on - /// some compilers; use an appropriately attributed alternative. (For example, Rf_error - /// bears the [[noreturn]] attribute and must be protected with safe.noreturn rather - /// than safe.operator[]). template constexpr function operator[](F* raw) const { return {raw}; @@ -170,8 +213,8 @@ struct protect { template void operator() [[noreturn]] (A&&... a) const { // workaround to support gcc4.8, which can't capture a parameter pack - unwind_protect( - detail::closure{ptr_, std::forward_as_tuple(std::forward(a)...)}); + unwind_protect(detail::closure{ + ptr_, std::forward_as_tuple(std::forward(a)...)}); // Compiler hint to allow [[noreturn]] attribute; this is never executed since // the above call will not return. throw std::runtime_error("[[noreturn]]"); @@ -179,6 +222,9 @@ struct protect { F* ptr_; }; + // To be used when wrapping functions tagged with `[[noreturn]]`, such as + // `Rf_errorcall()`, to force generation of attribute specific `struct closure` and + // `apply()` variants, see `struct return_tag` documentation for more details. template constexpr noreturn_function noreturn(F* raw) const { return {raw};