Use tag types to generate attribute specific internals and avoid ODR violations (#493)

* Add failing test

* Use tag types to generate attribute specific internals

* NEWS bullet

* Formatting

Author: DavisVaughan

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

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

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`))
 }

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<cpp11::decay_t<SEXP>>(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}

cpp11test/src/template-1-stop.cpp

@@ -0,0 +1,3 @@
+#include "cpp11/protect.hpp"
+
+[[cpp11::register]] void test_template_stop() { cpp11::stop("%s", "stop"); }

cpp11test/src/template-2-warn.cpp

@@ -0,0 +1,3 @@
+#include "cpp11/protect.hpp"
+
+[[cpp11::register]] void test_template_warning() { cpp11::warning("%s", "warning"); }

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
+

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()
+  })
+})

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 <size_t...>
 struct index_sequence {
   using type = index_sequence;
@@ -113,29 +159,30 @@ struct make_index_sequence
 template <>
 struct make_index_sequence<0> : index_sequence<> {};
 
-template <typename F, typename... Aref, size_t... I>
+template <typename ReturnTag, typename F, typename... Aref, size_t... I>
 decltype(std::declval<F&&>()(std::declval<Aref>()...)) apply(
     F&& f, std::tuple<Aref...>&& a, const index_sequence<I...>&) {
   return std::forward<F>(f)(std::get<I>(std::move(a))...);
 }
 
-template <typename F, typename... Aref>
+template <typename ReturnTag, typename F, typename... Aref>
 decltype(std::declval<F&&>()(std::declval<Aref>()...)) apply(F&& f,
                                                              std::tuple<Aref...>&& a) {
-  return apply(std::forward<F>(f), std::move(a), make_index_sequence<sizeof...(Aref)>{});
+  return apply<ReturnTag>(std::forward<F>(f), std::move(a),
+                          make_index_sequence<sizeof...(Aref)>{});
 }
 
 // overload to silence a compiler warning that the (empty) tuple parameter is set but
 // unused
-template <typename F>
+template <typename ReturnTag, typename F>
 decltype(std::declval<F&&>()()) apply(F&& f, std::tuple<>&&) {
   return std::forward<F>(f)();
 }
 
-template <typename F, typename... Aref>
+template <typename ReturnTag, typename F, typename... Aref>
 struct closure {
   decltype(std::declval<F*>()(std::declval<Aref>()...)) operator()() && {
-    return apply(ptr_, std::move(arefs_));
+    return apply<ReturnTag>(ptr_, std::move(arefs_));
   }
   F* ptr_;
   std::tuple<Aref...> arefs_;
@@ -149,17 +196,13 @@ struct protect {
     template <typename... A>
     decltype(std::declval<F*>()(std::declval<A&&>()...)) operator()(A&&... a) const {
       // workaround to support gcc4.8, which can't capture a parameter pack
-      return unwind_protect(
-          detail::closure<F, A&&...>{ptr_, std::forward_as_tuple(std::forward<A>(a)...)});
+      return unwind_protect(detail::closure<detail::return_tag, F, A&&...>{
+          ptr_, std::forward_as_tuple(std::forward<A>(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 <typename F>
   constexpr function<F> operator[](F* raw) const {
     return {raw};
@@ -170,15 +213,18 @@ struct protect {
     template <typename... A>
     void operator() [[noreturn]] (A&&... a) const {
       // workaround to support gcc4.8, which can't capture a parameter pack
-      unwind_protect(
-          detail::closure<F, A&&...>{ptr_, std::forward_as_tuple(std::forward<A>(a)...)});
+      unwind_protect(detail::closure<detail::no_return_tag, F, A&&...>{
+          ptr_, std::forward_as_tuple(std::forward<A>(a)...)});
       // Compiler hint to allow [[noreturn]] attribute; this is never executed since
       // the above call will not return.
       throw std::runtime_error("[[noreturn]]");
     }
     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 <typename F>
   constexpr noreturn_function<F> noreturn(F* raw) const {
     return {raw};