diff --git a/doc/modules/ROOT/pages/registries_and_policies.adoc b/doc/modules/ROOT/pages/registries_and_policies.adoc index db0f9ed0..a691c415 100644 --- a/doc/modules/ROOT/pages/registries_and_policies.adoc +++ b/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, diff --git a/include/boost/openmethod/initialize.hpp b/include/boost/openmethod/initialize.hpp index 50d4ce56..ac385556 100644 --- a/include/boost/openmethod/initialize.hpp +++ b/include/boost/openmethod/initialize.hpp @@ -727,6 +727,8 @@ struct registry::compiler : detail::generic_compiler { std::vector::const_iterator group, const bitvec& candidates, bool concrete); void write_global_data(); + void commit_global_data( + std::vector& 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::compiler::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::compiler::write_global_data() { detail::registry_state_transaction transaction; detail::initialize_policies::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 +template +void registry::compiler::commit_global_data( + std::vector& new_dispatch_data) noexcept { for (auto& m : methods) { auto first_info = m.infos[0]; diff --git a/test/test_initialize_transaction.cpp b/test/test_initialize_transaction.cpp index 551618df..900038a1 100644 --- a/test/test_initialize_transaction.cpp +++ b/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 #include @@ -19,8 +21,10 @@ #include "test_util.hpp" +#include #include #include +#include #include 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,18 +110,24 @@ auto poke_dog(Dog& dog) -> std::string { return poke::template next>(dog) + " bark"; } +template +auto poke_carnivore(Carnivore& carnivore) -> std::string { + return poke::template next>(carnivore) + + " growl"; +} + template struct snapshot { using vptr_state = typename Registry::template policy::state; using type_hash = typename Registry::template policy; + using hash_state = typename type_hash::state; snapshot() : dispatch_data(Registry::state().dispatch_data.data()), dog_vptr(Registry::template static_vptr), cat_vptr(Registry::template static_vptr), next(poke::template next>), - hash_range(type_hash::hash_range()), policies(Registry::state().policies) { } @@ -123,11 +135,17 @@ struct snapshot { return (detail::get(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(policies)); + } + const detail::word* dispatch_data; vptr_type dog_vptr; vptr_type cat_vptr; decltype(poke::template next>) next; - std::pair 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::vptr_state; - BOOST_OPENMETHOD_REGISTER(use_classes); + // 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); + BOOST_OPENMETHOD_REGISTER(use_classes); BOOST_OPENMETHOD_REGISTER( typename poke::template override>); BOOST_OPENMETHOD_REGISTER( typename poke::template override>); + BOOST_OPENMETHOD_REGISTER( + typename poke::template override>); Dog dog; Cat cat; @@ -156,6 +180,21 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( snapshot 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` + // 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` resolves + // to. The final initialize below observes both. + BOOST_OPENMETHOD_REGISTER(use_classes); + BOOST_OPENMETHOD_REGISTER(use_classes); + explosive::armed = true; BOOST_CHECK_THROW(initialize(), std::runtime_error); explosive::armed = false; @@ -171,8 +210,15 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( // rejects under /WX (-Wmicrosoft-cast). BOOST_TEST( (poke::template next> == before.next)); - BOOST_TEST( - (snapshot::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::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(st.policies).vptrs == before.vptrs())); // ...including the state of the policy that threw, after writing to it. BOOST_TEST(Registry::template state().generation == 1); @@ -180,10 +226,17 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( // ...but dispatch is refused until an initialize() succeeds. BOOST_CHECK_THROW(poke::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(); BOOST_TEST(st.initialized); - BOOST_TEST(poke::fn(dog) == "silence bark"); + BOOST_TEST(poke::fn(dog) == "silence growl bark"); BOOST_TEST(poke::fn(cat) == "silence"); + BOOST_TEST( + (poke::template next> != before.next)); + BOOST_TEST((hash.control != before.hash().control)); BOOST_TEST(Registry::template state().generation == 3); } @@ -222,3 +275,99 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( BOOST_TEST(st.initialized); BOOST_TEST(poke::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 + struct fn { + struct state { + trapping_stream os; + }; + + static auto& stream() { + return Registry::template state().os; + } + }; +}; + +template +struct tracing_registry : + test_registry_::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::vptr_state; + + BOOST_OPENMETHOD_REGISTER(use_classes); + BOOST_OPENMETHOD_REGISTER(poke::override>); + BOOST_OPENMETHOD_REGISTER(poke::override>); + + Dog dog; + auto& st = Registry::state(); + + initialize(); + BOOST_TEST(poke::fn(dog) == "silence bark"); + + snapshot before; + + trapping_stream::trap = "Installing"; + BOOST_CHECK_THROW(initialize(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 == 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(st.policies).vptrs == before.vptrs())); + + initialize(); + BOOST_TEST(st.initialized); + BOOST_TEST(poke::fn(dog) == "silence bark"); +}