initialize: move the last throwing statement before the commit

doc/modules/ROOT/pages/registries_and_policies.adoc

@@ -160,7 +160,10 @@ say - every policy gets its previous state back, and nothing else in the
 registry is modified: the v-table pointers, `next` pointers and dispatch
 tables from the previous call all stay in place. The registry is marked as not
 initialized, though, since that state no longer reflects the registrations,
-and cpp:initialize[] must be called again before calling a method.
+and cpp:initialize[] must be called again before calling a method. Only a
+registry with the cpp:runtime_checks[] policy diagnoses a call made in the
+meantime; without it, the call dispatches through the previous tables, which -
+after a `dlclose` - may point into unloaded code.
 
 A registry can also be created by copying an existing registry's policies,
 using the cpp:with[] and cpp:without[] nested templates. For example,

include/boost/openmethod/initialize.hpp

@@ -727,6 +727,8 @@ struct registry<Policies...>::compiler : detail::generic_compiler {
         std::vector<group_map>::const_iterator group, const bitvec& candidates,
         bool concrete);
     void write_global_data();
+    void commit_global_data(
+        std::vector<detail::word>& new_dispatch_data) noexcept;
     void print(const method_report& report) const;
     void print_slots();
     static void select_dominant_overriders(
@@ -1766,10 +1768,10 @@ void registry<Policies...>::compiler<Options...>::write_global_data() {
     // v-table pointer is staged in its class_, where the policies read it.
     // Only then are the shared locations patched - the method_infos' slots
     // and strides, the overriders' `next`, the class_infos' static_vptr - and
-    // the dispatch data swapped in; none of that can throw. If a policy
-    // throws, the registry still holds the previous dispatch state, complete
-    // and consistent, rather than pointers into a vector that unwinding has
-    // just freed.
+    // the dispatch data swapped in, by commit_global_data(), which is
+    // `noexcept`. If a policy throws, the registry still holds the previous
+    // dispatch state, complete and consistent, rather than pointers into a
+    // vector that unwinding has just freed.
 
     auto dispatch_data_size = std::accumulate(
         methods.begin(), methods.end(), std::size_t(0),
@@ -1880,12 +1882,26 @@ void registry<Policies...>::compiler<Options...>::write_global_data() {
 
     detail::registry_state_transaction<registry> transaction;
     detail::initialize_policies<registry>::fn(*this, options);
-    transaction.commit();
-
-    // Commit. Nothing from here on can throw.
 
+    // Last statement that can throw: the trace goes through the `output`
+    // policy, which is user-supplied. After the commit it would be the very
+    // bug this arrangement exists to prevent - the policies keeping the
+    // v-table pointers they just read from `new_dispatch_data`, which
+    // unwinding frees.
     ++tr << "Installing\n";
 
+    transaction.commit();
+    commit_global_data(new_dispatch_data);
+}
+
+// The commit point. Called once every step that can fail has succeeded, and
+// `noexcept` so that a throwing statement added here terminates loudly
+// instead of leaving the policies pointing into `new_dispatch_data`, which
+// the caller destroys on the way out.
+template<class... Policies>
+template<class... Options>
+void registry<Policies...>::compiler<Options...>::commit_global_data(
+    std::vector<detail::word>& new_dispatch_data) noexcept {
     for (auto& m : methods) {
         auto first_info = m.infos[0];
 

test/test_initialize_transaction.cpp

@@ -3,11 +3,13 @@
 // See accompanying file LICENSE_1_0.txt
 // or copy at http://www.boost.org/LICENSE_1_0.txt)
 
-// initialize() is transactional: if a policy's initialize throws, the
-// registry keeps the dispatch state it had before the call - the static
-// v-table pointers, the `next` pointers, the dispatch data and every policy's
-// state - instead of pointers into a vector that unwinding has freed. It is
-// marked as not initialized, though, until a call succeeds.
+// initialize() is transactional: if anything between staging the new dispatch
+// data and the commit point throws - a policy's initialize, or the trace
+// write that announces the installation, which goes through the user-supplied
+// `output` policy - the registry keeps the dispatch state it had before the
+// call: the static v-table pointers, the `next` pointers, the dispatch data
+// and every policy's state, instead of pointers into a vector that unwinding
+// has freed. It is marked as not initialized, though, until a call succeeds.
 
 #include <boost/openmethod.hpp>
 #include <boost/openmethod/initialize.hpp>
@@ -19,8 +21,10 @@
 
 #include "test_util.hpp"
 
+#include <cstring>
 #include <stdexcept>
 #include <string>
+#include <string_view>
 #include <tuple>
 
 using boost::mp11::mp_list;
@@ -85,8 +89,10 @@ struct Animal {
     virtual ~Animal() = default;
 };
 
-struct Dog : Animal {};
+struct Carnivore : Animal {};
+struct Dog : Carnivore {};
 struct Cat : Animal {};
+struct Bird : Animal {};
 
 struct BOOST_OPENMETHOD_ID(poke);
 
@@ -104,30 +110,42 @@ auto poke_dog(Dog& dog) -> std::string {
     return poke<Registry>::template next<poke_dog<Registry>>(dog) + " bark";
 }
 
+template<class Registry>
+auto poke_carnivore(Carnivore& carnivore) -> std::string {
+    return poke<Registry>::template next<poke_carnivore<Registry>>(carnivore) +
+        " growl";
+}
+
 template<class Registry>
 struct snapshot {
     using vptr_state =
         typename Registry::template policy<policies::vptr>::state;
     using type_hash = typename Registry::template policy<policies::type_hash>;
+    using hash_state = typename type_hash::state;
 
     snapshot() :
         dispatch_data(Registry::state().dispatch_data.data()),
         dog_vptr(Registry::template static_vptr<Dog>),
         cat_vptr(Registry::template static_vptr<Cat>),
         next(poke<Registry>::template next<poke_dog<Registry>>),
-        hash_range(type_hash::hash_range()),
         policies(Registry::state().policies) {
     }
 
     auto vptrs() -> decltype(auto) {
         return (detail::get<vptr_state>(policies).vptrs);
     }
 
+    // Everything `fast_perfect_hash::initialize` writes: the factors and the
+    // control table. `hash_range()` alone would miss a rollback that restored
+    // the range but not the factors.
+    auto hash() -> decltype(auto) {
+        return (detail::get<hash_state>(policies));
+    }
+
     const detail::word* dispatch_data;
     vptr_type dog_vptr;
     vptr_type cat_vptr;
     decltype(poke<Registry>::template next<poke_dog<Registry>>) next;
-    std::pair<std::size_t, std::size_t> hash_range;
     decltype(Registry::state().policies) policies;
 };
 
@@ -138,11 +156,17 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(
         typename Registry::registry_type>;
     using vptr_state = typename snapshot<Registry>::vptr_state;
 
-    BOOST_OPENMETHOD_REGISTER(use_classes<Animal, Dog, Cat, Registry>);
+    // Dog is registered here with Animal as its direct base, although it
+    // really derives from Carnivore. The missing edge is added between the two
+    // initializes, below.
+    BOOST_OPENMETHOD_REGISTER(use_classes<Animal, Carnivore, Cat, Registry>);
+    BOOST_OPENMETHOD_REGISTER(use_classes<Animal, Dog, Registry>);
     BOOST_OPENMETHOD_REGISTER(
         typename poke<Registry>::template override<poke_animal<Registry>>);
     BOOST_OPENMETHOD_REGISTER(
         typename poke<Registry>::template override<poke_dog<Registry>>);
+    BOOST_OPENMETHOD_REGISTER(
+        typename poke<Registry>::template override<poke_carnivore<Registry>>);
 
     Dog dog;
     Cat cat;
@@ -156,6 +180,21 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(
 
     snapshot<Registry> before;
 
+    // Perturb the input of the call that is about to fail. Without this it
+    // would see exactly what the successful call saw, and recompute
+    // bit-identical values for everything compared below - `fast_perfect_hash`
+    // re-seeds a fixed PRNG over the same class set, and `next<poke_dog>`
+    // resolves to the same overrider - so the assertions would hold whether or
+    // not the transaction rolled anything back. These registrars are
+    // function-local statics: they register on first pass through the
+    // declaration, here, not before main. `Bird` changes the class set that
+    // the hash factors, the control table and the v-table pointers are
+    // computed from; the Carnivore edge inserts `poke_carnivore` between
+    // `poke_dog` and `poke_animal`, changing what `next<poke_dog>` resolves
+    // to. The final initialize below observes both.
+    BOOST_OPENMETHOD_REGISTER(use_classes<Animal, Bird, Registry>);
+    BOOST_OPENMETHOD_REGISTER(use_classes<Carnivore, Dog, Registry>);
+
     explosive::armed = true;
     BOOST_CHECK_THROW(initialize<Registry>(), std::runtime_error);
     explosive::armed = false;
@@ -171,19 +210,33 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(
     // rejects under /WX (-Wmicrosoft-cast).
     BOOST_TEST(
         (poke<Registry>::template next<poke_dog<Registry>> == before.next));
-    BOOST_TEST(
-        (snapshot<Registry>::type_hash::hash_range() == before.hash_range));
+    // Parenthesized for the same reason: Boost.Test cannot print hash factors
+    // or vectors of type ids.
+    auto& hash =
+        detail::get<typename snapshot<Registry>::hash_state>(st.policies);
+    BOOST_TEST((hash.fn.mult == before.hash().fn.mult));
+    BOOST_TEST((hash.fn.shift == before.hash().fn.shift));
+    BOOST_TEST((hash.fn.min_value == before.hash().fn.min_value));
+    BOOST_TEST((hash.fn.max_value == before.hash().fn.max_value));
+    BOOST_TEST((hash.control == before.hash().control));
     BOOST_TEST((detail::get<vptr_state>(st.policies).vptrs == before.vptrs()));
     // ...including the state of the policy that threw, after writing to it.
     BOOST_TEST(Registry::template state<explosive_policy>().generation == 1);
 
     // ...but dispatch is refused until an initialize() succeeds.
     BOOST_CHECK_THROW(poke<Registry>::fn(dog), not_initialized);
 
+    // A successful call installs what the failed one would have: the
+    // perturbation is visible in the result, which confirms that the
+    // assertions above compared values that really do differ between the two
+    // calls.
     initialize<Registry>();
     BOOST_TEST(st.initialized);
-    BOOST_TEST(poke<Registry>::fn(dog) == "silence bark");
+    BOOST_TEST(poke<Registry>::fn(dog) == "silence growl bark");
     BOOST_TEST(poke<Registry>::fn(cat) == "silence");
+    BOOST_TEST(
+        (poke<Registry>::template next<poke_dog<Registry>> != before.next));
+    BOOST_TEST((hash.control != before.hash().control));
     BOOST_TEST(Registry::template state<explosive_policy>().generation == 3);
 }
 
@@ -222,3 +275,99 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(
     BOOST_TEST(st.initialized);
     BOOST_TEST(poke<Registry>::fn(dog) == "silence bark");
 }
+
+// The other way an initialize can fail after the policies have run: the trace
+// goes through the `output` policy, which is user code, so the statement that
+// announces the installation can throw. It sits before the commit, and must
+// stay there - after it, the policies would keep the v-table pointers they
+// just read out of the staging vector, which unwinding frees.
+
+// Discards what it is given, and throws once, on the message named in `trap`.
+struct trapping_stream {
+    static inline const char* trap = nullptr;
+
+    void write(const char* str) {
+        if (trap != nullptr && std::strstr(str, trap) != nullptr) {
+            trap = nullptr;
+            throw std::runtime_error("output");
+        }
+    }
+
+    auto is_on() const -> bool {
+        return true;
+    }
+};
+
+inline auto operator<<(trapping_stream& os, const char* str)
+    -> trapping_stream& {
+    os.write(str);
+    return os;
+}
+
+inline auto operator<<(trapping_stream& os, const std::string_view&)
+    -> trapping_stream& {
+    return os;
+}
+
+inline auto operator<<(trapping_stream& os, const void*) -> trapping_stream& {
+    return os;
+}
+
+inline auto operator<<(trapping_stream& os, void (*)()) -> trapping_stream& {
+    return os;
+}
+
+inline auto operator<<(trapping_stream& os, std::size_t) -> trapping_stream& {
+    return os;
+}
+
+struct trapping_output : policies::output {
+    template<class Registry>
+    struct fn {
+        struct state {
+            trapping_stream os;
+        };
+
+        static auto& stream() {
+            return Registry::template state<trapping_output>().os;
+        }
+    };
+};
+
+template<int N>
+struct tracing_registry :
+    test_registry_<N>::template with<
+        policies::runtime_checks, policies::throw_error_handler,
+        trapping_output> {};
+
+BOOST_AUTO_TEST_CASE(a_throwing_trace_does_not_commit) {
+    using Registry = tracing_registry<__COUNTER__>;
+    using vptr_state = typename snapshot<Registry>::vptr_state;
+
+    BOOST_OPENMETHOD_REGISTER(use_classes<Animal, Dog, Cat, Registry>);
+    BOOST_OPENMETHOD_REGISTER(poke<Registry>::override<poke_animal<Registry>>);
+    BOOST_OPENMETHOD_REGISTER(poke<Registry>::override<poke_dog<Registry>>);
+
+    Dog dog;
+    auto& st = Registry::state();
+
+    initialize<Registry>();
+    BOOST_TEST(poke<Registry>::fn(dog) == "silence bark");
+
+    snapshot<Registry> before;
+
+    trapping_stream::trap = "Installing";
+    BOOST_CHECK_THROW(initialize<Registry>(trace(true)), std::runtime_error);
+    BOOST_TEST(trapping_stream::trap == nullptr); // it did throw there
+
+    BOOST_TEST(!st.initialized);
+    BOOST_TEST(st.dispatch_data.data() == before.dispatch_data);
+    BOOST_TEST(Registry::template static_vptr<Dog> == before.dog_vptr);
+    // The one that matters: had the policies been committed, these would be
+    // pointers into the staging vector, which no longer exists.
+    BOOST_TEST((detail::get<vptr_state>(st.policies).vptrs == before.vptrs()));
+
+    initialize<Registry>();
+    BOOST_TEST(st.initialized);
+    BOOST_TEST(poke<Registry>::fn(dog) == "silence bark");
+}