initialize: close five holes in the transaction

doc/modules/ROOT/pages/registries_and_policies.adoc

@@ -158,9 +158,14 @@ cpp:initialize[] is transactional. If a policy's `initialize` throws -
 `fast_perfect_hash` failing to find hash factors under `throw_error_handler`,
 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
+tables are left exactly as the call found them. That need not be a state the
+registry can dispatch through - after a cpp:finalize[] it is a torn-down one -
+only the state that was there before. 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

@@ -124,27 +124,98 @@ struct initialize_policies<Registry, mp11::mp_list<Policies...>> {
     }
 };
 
+// Selects the policies whose state the transaction has to save: those that
+// have a `state`, *and* whose `initialize` will actually be called for this
+// Context and Options - the same test initialize_policy makes, so the two
+// cannot disagree about which policies run.
+template<class Context, class Options>
+struct policy_state_is_volatile_q {
+    template<class PolicyFn>
+    using fn = mp11::mp_bool<
+        has_policy_state<PolicyFn>::value &&
+        has_initialize<PolicyFn, const Context&, const Options&>>;
+};
+
 // Saves the policies' states on construction, and puts them back on
 // destruction unless commit() was called - so a policy's `initialize` that
 // throws, after itself or another policy has already written to shared
 // state, leaves the registry as it was. The registry's mutable state is one
-// variable, registry_state<Registry>::st; its `policies` tuple is copied
-// whole, states of policies without an `initialize` included, since
-// restoring an untouched state is harmless, and simpler than picking. The
-// other members need no saving: initialize() only reads the class and
-// method lists, and write_global_data() replaces dispatch_data at commit
-// time only, after which nothing can throw - on rollback it still holds the
-// previous tables, which the classes' static_vptrs point into. (That is
-// also why a copy could not stand in for it: it would be another buffer.)
-template<class Registry>
+// variable, registry_state<Registry>::st. Its other members need no saving:
+// initialize() only reads the class and method lists, and write_global_data()
+// replaces dispatch_data at commit time only, after which nothing can throw -
+// on rollback it still holds the previous tables, which the classes'
+// static_vptrs point into. (That is also why a copy could not stand in for
+// it: it would be another buffer.)
+//
+// Only the states of the policies that are about to be initialized are saved.
+// Copying the `policies` tuple whole is simpler, but it is not harmless: a
+// state that no `initialize` touches is not derived data this call is about to
+// replace, it is configuration the caller owns. The error handler is the case
+// that bites - it is *called* from inside the window, by design
+// (fast_perfect_hash reports a search failure through it), so a handler that
+// disarms itself with set() before throwing would have that undone on the way
+// out. Saving wide also forces every policy state in the registry to be
+// copyable, including those of policies that have no `initialize` at all -
+// which rules out the std::ostringstream an `output` policy written to the
+// documented state pattern naturally holds - and copies each of them, vectors
+// and all, on every successful initialize().
+template<class Registry, class Context, class Options>
 class registry_state_transaction {
+    using policy_fns = mp11::mp_transform_q<
+        policy_fn_q<Registry>, typename Registry::policy_list>;
+    using saved_states = mp11::mp_transform<
+        policy_state_t,
+        mp11::mp_filter_q<
+            policy_state_is_volatile_q<Context, Options>, policy_fns>>;
+    using saved_type = mp11::mp_apply<detail::tuple, saved_states>;
+
+    static_assert(
+        mp11::mp_all_of<saved_states, std::is_copy_assignable>::value,
+        "the `state` of a policy that defines `initialize` must be copyable: "
+        "initialize() saves it, and puts it back if a policy throws");
+
+    // The restore runs from the destructor, while an exception is in flight,
+    // and a destructor is noexcept by default: a move-assignment that threw
+    // there would call std::terminate, destroying the very error the
+    // transaction exists to let through. It is not a theoretical shape - a
+    // state holding a std::map with a stateful, non-always-equal allocator
+    // degrades to an element-wise move that allocates, and `vptr_map<MapFn>`
+    // lets a caller supply exactly that. Refuse it here, where the message can
+    // say why, rather than terminate at run time in the one configuration no
+    // test covers.
+    static_assert(
+        mp11::mp_all_of<saved_states, std::is_nothrow_move_assignable>::value,
+        "the `state` of a policy that defines `initialize` must be nothrow "
+        "move-assignable: initialize() puts it back from a destructor, while "
+        "an exception is in flight, where throwing would terminate");
+
+    // Element-wise: `saved` holds a subset of the registry's tuple.
+    template<class Tuple>
+    struct each;
+
+    template<class... States>
+    struct each<detail::tuple<States...>> {
+        static void save(detail::tuple<States...>& to) {
+            (...,
+             (detail::get<States>(to) =
+                  detail::get<States>(Registry::state().policies)));
+        }
+
+        static void restore(detail::tuple<States...>& from) {
+            (...,
+             (detail::get<States>(Registry::state().policies) =
+                  std::move(detail::get<States>(from))));
+        }
+    };
+
   public:
-    registry_state_transaction() : saved(Registry::state().policies) {
+    registry_state_transaction() {
+        each<saved_type>::save(saved);
     }
 
     ~registry_state_transaction() {
         if (!committed) {
-            Registry::state().policies = std::move(saved);
+            each<saved_type>::restore(saved);
         }
     }
 
@@ -157,7 +228,7 @@ class registry_state_transaction {
     }
 
   private:
-    typename registry_state_type<Registry>::policies_type saved;
+    saved_type saved;
     bool committed = false;
 };
 
@@ -727,6 +798,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(
@@ -756,11 +829,22 @@ void registry<Policies...>::compiler<Options...>::install_global_tables() {
         abort();
     }
 
-    write_global_data();
-
+    // Report before installing, not after. Everything printed here is
+    // compiler-local - the report gathered during compile(), the slot
+    // assignment, the class lattices - so none of it needs the new tables to
+    // be in place; and all of it can throw, through the user-supplied `output`
+    // policy, or out of the containers print_slots() builds. Run after
+    // write_global_data() it would throw past the commit, leaving the new
+    // tables installed while initialize() never reaches
+    // `st.initialized = true` - a state the exception-safety contract does not
+    // describe, and one that makes the next call abort with `not_initialized`
+    // under runtime_checks. Before it, a throw is just another failure the
+    // transaction rolls back.
     print(report);
     print_slots();
     ++tr << "Finished\n";
+
+    write_global_data();
 }
 
 template<class... Policies>
@@ -780,8 +864,11 @@ template<class... Policies>
 template<class... Options>
 void registry<Policies...>::compiler<Options...>::initialize() {
     // Clear the flag up front, and set it only once everything has succeeded.
-    // A run that throws leaves the previous dispatch state in place, complete
-    // and consistent (see write_global_data()) - but not marked initialized:
+    // A run that throws leaves the dispatch state exactly as the call found it
+    // (see write_global_data()) - which is not the same as a working one: if a
+    // finalize() came in between, what the call found was already torn down,
+    // the classes' static_vptrs still set and the dispatch data cleared.
+    // Either way it is not marked initialized:
     // those tables do not reflect the registrations that prompted the call,
     // and after a dlclose (the documented re-initialize flow) they may point
     // into unloaded code, so require_initialized() must keep refusing to
@@ -1766,10 +1853,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 dispatch
+    // state it had on entry - whatever that was - 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),
@@ -1878,14 +1965,30 @@ void registry<Policies...>::compiler<Options...>::write_global_data() {
 
     ++tr << rflush(4, dispatch_data_size) << " " << gv_iter << " end\n";
 
-    detail::registry_state_transaction<registry> transaction;
+    detail::registry_state_transaction<
+        registry, compiler, std::tuple<Options...>>
+        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];
 
@@ -2222,8 +2325,14 @@ void registry<Policies...>::compiler<Options...>::print_slots() {
 //! call: no static v-table pointer, `next` pointer, dispatch table or policy
 //! state is modified. The registry is nonetheless marked as not initialized,
 //! since that state does not reflect the current registrations; `initialize`
-//! must be called again, successfully, before any method is called. Policy
-//! states are restored from a copy, so a policy's `state` must be copyable.
+//! must be called again, successfully, before any method is called.
+//!
+//! Only the policies that define `initialize` have their `state` saved and
+//! restored, so only those states have to be copyable. A state that no
+//! `initialize` writes to is configuration, not derived data, and is left
+//! alone: an @ref error_handler policy is *called* during `initialize`, and a
+//! handler that changes the configuration - installing a different handler
+//! with `set`, say - keeps that change whether the call succeeds or throws.
 //!
 //! @par Example
 //!

test/compile_fail_policy_state_throwing_move.cpp

@@ -0,0 +1,62 @@
+// Copyright (c) 2017-2026 Jean-Louis Leroy
+// Distributed under the Boost Software License, Version 1.0.
+// See accompanying file LICENSE_1_0.txt
+// or copy at http://www.boost.org/LICENSE_1_0.txt)
+
+// Expected diagnostic, as a CMake regex (see CMakeLists.txt).
+// expected-error: nothrow move-assignable
+
+#include <stdexcept>
+#include <tuple>
+
+#include <boost/openmethod.hpp>
+#include <boost/openmethod/initialize.hpp>
+
+using namespace boost::openmethod;
+
+// initialize() saves this policy's state and puts it back, from the
+// transaction's destructor, if another policy throws. That destructor runs
+// while an exception is in flight and is noexcept by default, so a
+// move-assignment that can throw would terminate the program instead of
+// letting the original error through. The transaction refuses the policy at
+// compile time rather than leave that in the program.
+struct throwing_move_policy {
+    using category = throwing_move_policy;
+
+    template<class Registry>
+    struct fn {
+        struct state {
+            state() = default;
+            state(const state&) = default;
+            auto operator=(const state&) -> state& = default;
+
+            state(state&&) {
+                throw std::runtime_error("move");
+            }
+
+            auto operator=(state&&) -> state& {
+                throw std::runtime_error("move");
+            }
+
+            int generation = 0;
+        };
+
+        template<class Context, class... Options>
+        static void initialize(const Context&, const std::tuple<Options...>&) {
+            ++Registry::template state<throwing_move_policy>().generation;
+        }
+    };
+};
+
+struct bad_registry : default_registry::with<throwing_move_policy> {};
+
+struct Animal {
+    virtual ~Animal() = default;
+};
+
+BOOST_OPENMETHOD_REGISTER(use_classes<Animal, bad_registry>);
+
+int main() {
+    initialize<bad_registry>();
+    return 0;
+}