From f876ad973f792fc7cd53b6212bc8cce09a5c6ee3 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Mon, 14 Jul 2014 17:21:46 -0700 Subject: [PATCH] =?UTF-8?q?Fix=20static=5Finitializer:=20=E2=80=A6=20*=20P?= =?UTF-8?q?revents=20double=20construction,=20invalid=20access=20*=20Unit?= =?UTF-8?q?=20test=20works=20on=20MSVC=20and=20non=20MSVC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Builds/VisualStudio2013/RippleD.vcxproj | 30 +-- .../VisualStudio2013/RippleD.vcxproj.filters | 12 +- src/beast/beast/utility/Utility.unity.cpp | 1 + src/beast/beast/utility/static_initializer.h | 199 +++++++++++----- .../utility/tests/static_initializer.test.cpp | 216 ++++++++++++++++++ src/ripple/module/app/ledger/OrderBookDB.cpp | 2 +- 6 files changed, 390 insertions(+), 70 deletions(-) create mode 100644 src/beast/beast/utility/tests/static_initializer.test.cpp diff --git a/Builds/VisualStudio2013/RippleD.vcxproj b/Builds/VisualStudio2013/RippleD.vcxproj index 1954bdffe4..01eb306b12 100644 --- a/Builds/VisualStudio2013/RippleD.vcxproj +++ b/Builds/VisualStudio2013/RippleD.vcxproj @@ -1147,6 +1147,8 @@ + + @@ -1161,6 +1163,9 @@ True + + True + True @@ -2037,6 +2042,8 @@ True + + True @@ -2206,9 +2213,6 @@ - - True - @@ -3203,16 +3207,16 @@ - Document - protoc --cpp_out=..\..\build\proto --proto_path=%(RelativeDir) %(Identity) - ..\..\build\proto\ripple.pb.h;..\..\build\proto\ripple.pb.cc - protoc --cpp_out=..\..\build\proto --proto_path=%(RelativeDir) %(Identity) - false Document protoc --cpp_out=..\..\build\proto --proto_path=%(RelativeDir) %(Identity) ..\..\build\proto\ripple.pb.h;..\..\build\proto\ripple.pb.cc protoc --cpp_out=..\..\build\proto --proto_path=%(RelativeDir) %(Identity) false + Document + protoc --cpp_out=..\..\build\proto --proto_path=%(RelativeDir) %(Identity) + ..\..\build\proto\ripple.pb.h;..\..\build\proto\ripple.pb.cc + protoc --cpp_out=..\..\build\proto --proto_path=%(RelativeDir) %(Identity) + false @@ -3447,8 +3451,8 @@ - ..\..\src\hyperleveldb;..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) ..\..\src\hyperleveldb;..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) + ..\..\src\hyperleveldb;..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) @@ -3457,8 +3461,8 @@ - ..\..\src\leveldb;..\..\src\leveldb\include;..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) ..\..\src\leveldb;..\..\src\leveldb\include;..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) + ..\..\src\leveldb;..\..\src\leveldb\include;..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) @@ -3467,8 +3471,8 @@ - ..\..\src\leveldb\include;..\..\src\rocksdb\include;%(AdditionalIncludeDirectories) ..\..\src\leveldb\include;..\..\src\rocksdb\include;%(AdditionalIncludeDirectories) + ..\..\src\leveldb\include;..\..\src\rocksdb\include;%(AdditionalIncludeDirectories) @@ -3489,8 +3493,8 @@ - ..\..\src\rocksdb;..\..\src\rocksdb\include;..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) ..\..\src\rocksdb;..\..\src\rocksdb\include;..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) + ..\..\src\rocksdb;..\..\src\rocksdb\include;..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) @@ -3503,8 +3507,8 @@ - ..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) ..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) + ..\..\src\snappy\config;..\..\src\snappy\snappy;%(AdditionalIncludeDirectories) diff --git a/Builds/VisualStudio2013/RippleD.vcxproj.filters b/Builds/VisualStudio2013/RippleD.vcxproj.filters index 5a8b7f190d..f31331d692 100644 --- a/Builds/VisualStudio2013/RippleD.vcxproj.filters +++ b/Builds/VisualStudio2013/RippleD.vcxproj.filters @@ -1926,6 +1926,9 @@ beast\utility + + beast\utility + beast\utility @@ -1941,6 +1944,9 @@ beast\utility\tests + + beast\utility\tests + beast\utility\tests @@ -3030,6 +3036,9 @@ ripple\module\app\ledger + + ripple\module\app\ledger + ripple\module\app\ledger @@ -3234,9 +3243,6 @@ ripple\module\app\misc - - ripple\module\app\misc - ripple\module\app\misc diff --git a/src/beast/beast/utility/Utility.unity.cpp b/src/beast/beast/utility/Utility.unity.cpp index a0e29bcd79..468418e2d0 100644 --- a/src/beast/beast/utility/Utility.unity.cpp +++ b/src/beast/beast/utility/Utility.unity.cpp @@ -32,5 +32,6 @@ #include #include #include +#include #include #include diff --git a/src/beast/beast/utility/static_initializer.h b/src/beast/beast/utility/static_initializer.h index 72aa0dbf4f..dc9531bca3 100644 --- a/src/beast/beast/utility/static_initializer.h +++ b/src/beast/beast/utility/static_initializer.h @@ -20,14 +20,16 @@ #ifndef BEAST_UTILITY_STATIC_INITIALIZER_H_INCLUDED #define BEAST_UTILITY_STATIC_INITIALIZER_H_INCLUDED -#ifdef _MSC_VER - #include -#include -#include +#include + +#ifdef _MSC_VER +#include #include #include #include +#include +#endif namespace beast { @@ -44,7 +46,129 @@ namespace beast { } @endcode */ -template +#ifdef _MSC_VER +template < + class T, + class Tag = void +> +class static_initializer +{ +private: + struct data_t + { + // 0 = unconstructed + // 1 = constructing + // 2 = constructed + long volatile state; + + typename std::aligned_storage ::value>::type storage; + }; + + struct destroyer + { + T* t_; + explicit destroyer (T* t) : t_(t) { } + ~destroyer() { t_->~T(); } + }; + + static + data_t& + data() noexcept; + +public: + template + explicit static_initializer (Args&&... args); + + T& + get() noexcept; + + T& + operator*() noexcept + { + return get(); + } + + T* + operator->() noexcept + { + return &get(); + } +}; + +//------------------------------------------------------------------------------ + +template +auto +static_initializer ::data() noexcept -> + data_t& +{ + static data_t _; // zero-initialized + return _; +} + +template +template +static_initializer ::static_initializer (Args&&... args) +{ + data_t& _(data()); + + // Double Checked Locking Pattern + + if (_.state != 2) + { + T* const t (reinterpret_cast(&_.storage)); + + for(;;) + { + long prev; + prev = InterlockedCompareExchange(&_.state, 1, 0); + if (prev == 0) + { + try + { + ::new(t) T (std::forward(args)...); + static destroyer on_exit (t); + InterlockedIncrement(&_.state); + } + catch(...) + { + // Constructors that throw exceptions are unsupported + std::terminate(); + } + } + else if (prev == 1) + { + std::this_thread::yield(); + } + else + { + assert(prev == 2); + break; + } + } + } +} + +template +T& +static_initializer ::get() noexcept +{ + data_t& _(data()); + for(;;) + { + if (_.state == 2) + break; + std::this_thread::yield(); + } + return *reinterpret_cast(&_.storage); +} + +#else +template < + class T, + class Tag = void +> class static_initializer { private: @@ -52,53 +176,8 @@ private: public: template - static_initializer(Args&&... args) - { - static std::aligned_storage ::value>::type storage; - instance_ = reinterpret_cast(&storage); - - // double checked lock - static bool volatile initialized; - if (! initialized) - { - static std::atomic_flag lock; - while (lock.test_and_set()) - std::this_thread::sleep_for ( - std::chrono::milliseconds(10)); - if (! initialized) - { - try - { - ::new(instance_) T(std::forward(args)...); - - struct destroyer - { - T* t_; - - destroyer (T* t) - : t_(t) - { - } - - ~destroyer() - { - t_->~T(); - } - }; - - static destroyer on_exit (instance_); - } - catch(...) - { - lock.clear(); - throw; - } - initialized = true; - } - lock.clear(); - } - } + explicit + static_initializer (Args&&... args); T& get() noexcept @@ -109,12 +188,26 @@ public: T& operator*() noexcept { - return *instance_; + return get(); + } + + T* + operator->() noexcept + { + return &get(); } }; +template +template +static_initializer ::static_initializer (Args&&... args) +{ + static T t (std::forward(args)...); + instance_ = &t; } #endif +} + #endif diff --git a/src/beast/beast/utility/tests/static_initializer.test.cpp b/src/beast/beast/utility/tests/static_initializer.test.cpp new file mode 100644 index 0000000000..c8d834e217 --- /dev/null +++ b/src/beast/beast/utility/tests/static_initializer.test.cpp @@ -0,0 +1,216 @@ +//------------------------------------------------------------------------------ +/* + This file is part of Beast: https://github.com/vinniefalco/Beast + Copyright 2013, Vinnie Falco + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include +#include +#include + +namespace beast { + +static_assert(__alignof(long) >= 4, ""); + +class static_initializer_test : public unit_test::suite +{ +public: + // Used to create separate instances for each test + struct cxx11_tag { }; + struct beast_tag { }; + template + struct Case + { + enum { count = N }; + typedef Tag type; + }; + + struct Counts + { + Counts() + : calls (0) + , constructed (0) + , access (0) + { + } + + // number of calls to the constructor + std::atomic calls; + + // incremented after construction completes + std::atomic constructed; + + // increment when class is accessed before construction + std::atomic access; + }; + + /* This testing singleton detects two conditions: + 1. Being accessed before getting fully constructed + 2. Getting constructed twice + */ + template + class Test; + + template + static + void + run_many (std::size_t n, Function f); + + template + void + test (cxx11_tag); + + template + void + test (beast_tag); + + template + void + test(); + + void + run (); +}; + +//------------------------------------------------------------------------------ + +template +class static_initializer_test::Test +{ +public: + explicit + Test (Counts& counts); + + void + operator() (Counts& counts); +}; + +template +static_initializer_test::Test::Test (Counts& counts) +{ + ++counts.calls; + std::this_thread::sleep_for (std::chrono::milliseconds (10)); + ++counts.constructed; +} + +template +void +static_initializer_test::Test::operator() (Counts& counts) +{ + if (! counts.constructed) + ++counts.access; +} + +//------------------------------------------------------------------------------ + +template +void +static_initializer_test::run_many (std::size_t n, Function f) +{ + std::atomic start (false); + std::vector threads; + + threads.reserve (n); + + for (auto i (n); i-- ;) + { + threads.emplace_back([&]() + { + while (! start.load()) + std::this_thread::yield(); + f(); + }); + } + std::this_thread::sleep_for (std::chrono::milliseconds (10)); + std::this_thread::yield(); + start.store (true); + for (auto& thread : threads) + thread.join(); +} + +template +void +static_initializer_test::test (cxx11_tag) +{ + testcase << "cxx11 " << Tag::count << " threads"; + + Counts counts; + + run_many (Tag::count, [&]() + { + static Test t (counts); + t(counts); + }); + +#ifdef _MSC_VER + // Visual Studio 2013 and earlier can exhibit both double + // construction, and access before construction when function + // local statics are initialized concurrently. + // + expect (counts.constructed > 1 || counts.access > 0); + +#else + expect (counts.constructed == 1 && counts.access == 0); + +#endif +} + +template +void +static_initializer_test::test (beast_tag) +{ + testcase << "beast " << Tag::count << " threads"; + + Counts counts; + + run_many (Tag::count, [&counts]() + { + static static_initializer > t (counts); + (*t)(counts); + }); + + expect (counts.constructed == 1 && counts.access == 0); +} + +template +void +static_initializer_test::test() +{ + test (typename Tag::type {}); +} + +void +static_initializer_test::run () +{ + test > (); + test > (); + test > (); + test > (); + + test > (); + test > (); + test > (); + test > (); +} + +//------------------------------------------------------------------------------ + +BEAST_DEFINE_TESTSUITE(static_initializer,utility,beast); + +} diff --git a/src/ripple/module/app/ledger/OrderBookDB.cpp b/src/ripple/module/app/ledger/OrderBookDB.cpp index 703fbe8a6a..ce39a95bd3 100644 --- a/src/ripple/module/app/ledger/OrderBookDB.cpp +++ b/src/ripple/module/app/ledger/OrderBookDB.cpp @@ -33,9 +33,9 @@ void OrderBookDB::invalidate () void OrderBookDB::setup (Ledger::ref ledger) { - auto seq = ledger->getLedgerSeq (); { ScopedLockType sl (mLock); + auto seq = ledger->getLedgerSeq (); // Do a full update every 256 ledgers if (mSeq != 0)