The (de)initialization order is not well defined in C++, so generally it is not safe to use globals as the (de/con)structor of one global could use the (de/con)structor of another global before/after it has been (con/de)structed.
Specifically this fixes:
g_logger might not be initialized on the first use, so do that. (Fixes #15111)
Just as a way to minimize the diff (which may or may not be desirable): you can have a trivial proxy object instantiated as g_logger whose operator->() invokes GetLogger().
An alternative is to use templating and also defer instantiation to operator->(). This way the diff would be smaller and the code less verbose.
<details> ```cpp #include <string> #include <iostream>
using namespace std;
template <typename T> class COFU { T* get() { static T v; return &v; }
public: T* operator->() { return get(); } T* const operator->() const { return const_cast<COFU<T>>(this)->get(); } T& operator() { return get(); } T& operator() const { return const_cast<COFU<T>>(this)->get(); } };
string* const g_str_old = new string(); COFU<string> const g_str_new;
int main() { g_str_old->append("Hello!"); g_str_new->append("Hello!");
cout << *g_str_old << endl; cout << *g_str_new << endl;
return 0; }
</details>
Edit: oh [@sipa](/bitcoin-bitcoin/contributor/sipa/) already suggested above.
77 | }; 78 | -LockData& GetLockData() { 79 | - static LockData lockdata; 80 | - return lockdata; 81 | -} 82 | +static GLOBAL_COFU(LockData, GetLockData);
static 👍
COFU<string> const g_str_new;
It looks like you are introducing a new global for this, which again opens up the problem I am trying to solve. Either I am missing something or we are running in circles.
@MarcoFalke my suggestion is to do:
COFU<BCLog::Logger> const g_logger;
so that the following is unchanged:
g_logger->m_reopen_file = true;
Like @sipa suggested.
@MarcoFalke Not sure if that's what you're referring to, but the COFU object has no member variables to initialize, so it doesn't have any runtime initialization (in other words, it's fully constructed before any code is executed).
I don't have a strong opinion on whether such a COFU object/type is the right solution; just offering it as a possibility if reducing the size of the diff is wanted.
Ah, that makes it clearer.
Relevant section from https://en.cppreference.com/w/cpp/language/constant_initialization:
The effects of constant initialization are the same as the effects of the corresponding initialization, except that it's guaranteed that it is complete before any other initialization of a static or thread-local object begins, and it may be performed at compile time.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
4 | @@ -5,6 +5,7 @@ 5 | #include <sync.h> 6 | 7 | #include <logging.h> 8 | +#include <util/memory.h>
nit: You can delete `#include <memory>` after a few lines.
Any reason to keep the mentioned include?
It has been there before, so according to iwyu it should stay there. I'd prefer if someone jotted down some notes on how to run iwyu on Bitcoin Core, so that we wouldn't have to spend review on those.
102 | @@ -113,7 +103,7 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch, 103 | 104 | static void push_lock(void* c, const CLockLocation& locklocation) 105 | { 106 | - LockData& lockdata = GetLockData(); 107 | + LockData& lockdata{*g_lockdata.Get()};
I prefer following code rather than calling Get() directly.
LockData& lockdata = *g_lockdata;
or
LockData& lockdata{*g_lockdata};
167 | @@ -178,11 +168,7 @@ void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLi 168 | 169 | void DeleteLock(void* cs) 170 | { 171 | - LockData& lockdata = GetLockData(); 172 | - if (!lockdata.available) { 173 | - // We're already shutting down. 174 | - return; 175 | - } 176 | + LockData& lockdata{*g_lockdata.Get()};
(same above) I prefer following code rather than calling Get() directly.
LockData& lockdata = *g_lockdata;
or
LockData& lockdata{*g_lockdata};
102 | @@ -113,7 +103,7 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch, 103 | 104 | static void push_lock(void* c, const CLockLocation& locklocation) 105 | { 106 | - LockData& lockdata = GetLockData(); 107 | + LockData& lockdata{*g_lockdata.Get()}; 108 | std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
Another alternative is to ditch local lockdata and use global g_lockdata:
std::lock_guard<std::mutex> lock(g_lockdata->dd_mutex);
Would result in a bigger diff but I think it is better than "aliases".
21 | @@ -21,7 +22,7 @@ const char * const DEFAULT_DEBUGLOGFILE = "debug.log"; 22 | * This method of initialization was originally introduced in 23 | * ee3374234c60aba2cc4c5cd5cac1c0aefc2d817c. 24 | */ 25 | -BCLog::Logger* const g_logger = new BCLog::Logger(); 26 | +GlobalCOFU<BCLog::Logger> g_logger;
Could update comment above to note that the raw pointer is now in GlobalCOFU?
ACK faacd53.
15 | @@ -16,4 +16,20 @@ std::unique_ptr<T> MakeUnique(Args&&... args) 16 | return std::unique_ptr<T>(new T(std::forward<Args>(args)...)); 17 | } 18 | 19 | +/** 20 | + * Ensure that a global is initialized before its first use and never
This no-destruction property shouldn't be needed. C++ guarantees that destructors are invoked at shutdown in reverse order as they were constructed (even taking objects constructed through functions invoked from other module's global initializers into account).
Have you found evidence that this isn't enough?
@sipa, see the comments in sync and logging. Basically the order in what you need the destructor to be called might be different from the inverse order they were constructed.
See a simple example that segfaults if I dropped that requirement:
(The ~Init destructor uses the log after the logger has been destructed)
#include <iostream>
#include <memory>
struct Log{
std::unique_ptr<const std::string> m_prefix;
Log(){m_prefix=std::unique_ptr<const std::string>(new std::string{"::: "});log(__func__);};
~Log(){log(__func__);};
void log(const std::string&s){std::cout << *m_prefix << s << std::endl;}
};
Log& LogInstance() {
static Log l;
return l;
}
struct Init{
Init(){std::cout << __func__<<std::endl;}
~Init(){LogInstance().log(__func__);}
} g_init;
int main()
{
LogInstance().log(__func__);
}
@MarcoFalke because Init is constructed first. It works if you change Init constructor to Init(){LogInstance().log(__func__);}.
@MarcoFalke I think that can be resolved by calling LogInstance in the Init::Init constructor once. That forces the Log object to finished construction before Init, guaranteeing it's still available at Init's destruction time.
I know, but there is no way to enforce that (in Bitcoin Core or generally) the logger (or any other global "module") is called in each constructor when the destructor calls it.
I really prefer going over the few global objects whose destructors need other globals and fixing this, than having lingering global objects at shutdown.
Just make a call to LogInstance()?
15 | @@ -16,4 +16,20 @@ std::unique_ptr<T> MakeUnique(Args&&... args) 16 | return std::unique_ptr<T>(new T(std::forward<Args>(args)...)); 17 | } 18 | 19 | +/** 20 | + * Ensure that a global is initialized before its first use and never 21 | + * destructed, since other destructors might use it. 22 | + * This class should not be used where the destructor ~T() is non-trivial. 23 | + */ 24 | +template <typename T>
Perhaps also add a comment to explain that if multiple instances of GlobalCOFU<T> for the same T exist, they will all share the underlying T object.
20 | + * Ensure that a global is initialized before its first use and never 21 | + * destructed, since other destructors might use it. 22 | + * This class should not be used where the destructor ~T() is non-trivial. 23 | + */ 24 | +template <typename T> 25 | +struct GlobalCOFU {
This seems over-engineered. Why not just have a simple function like:
Logger& LogInstance() {
static Logger logger;
return logger;
}
and have a scripted diff replacing g_logger-> with LogInstance(). This would be equivalent to what the GlobalCOFU class is doing but more straightforward, and would actually guarantee destruction, and avoid creating landmines where separate variables of the same type point to the same instances, and avoid weird terminology (COFU == singleton?)
and would actually guarantee destruction
FWIW the same could be done with GlobalCOFU.
Reverted back to my initial patch (using the LogInstance wording suggested here #15266 (review))
20 | @@ -21,7 +21,11 @@ const char * const DEFAULT_DEBUGLOGFILE = "debug.log"; 21 | * This method of initialization was originally introduced in 22 | * ee3374234c60aba2cc4c5cd5cac1c0aefc2d817c. 23 | */ 24 | -BCLog::Logger* const g_logger = new BCLog::Logger(); 25 | +BCLog::Logger& LogInstance() 26 | +{ 27 | + static BCLog::Logger* g_logger{new BCLog::Logger()};
Would add comment here that this intentionally never calls logger destructor. Would also add a comment in the logger class saying that if a destructor is defined there it will never be called, and that any members added to the logger class will not have their destructors called either.
The comment above says that, I think. Though I added a line clarifying that it must be a trivial destructor.
utACK fa8e2b75ff7de6b09f97512256092c1d03edef01
utACK 77777c5624e2f5416d85500e82b7c80e10ed01b6. Just comment change since last review. Also the commit hash no longer begins with fa (in case that is a problem).
utACK 77777c5624e2f5416d85500e82b7c80e10ed01b6