memory: Construct globals on first use by maflcko · Pull Request #15266 · bitcoin/bitcoin

maflcko · 2019-01-25T21:01:21Z

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 Avoid UB (member call on nullptr) when failing to read randomness in the startup process #15111)

sipa · 2019-01-25T21:22:47Z

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().

promag

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 #include

using namespace std;

template
class COFU
{
T* get() {
static T v;
return &v;
}

public:
T* operator->() {
return get();
}
T* const operator->() const {
return const_cast<COFU>(this)->get();
}
T& operator() {
return get();
}
T& operator() const {
return const_cast<COFU>(this)->get();
}
};

string* const g_str_old = new string();
COFU 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 already suggested above.

src/sync.cpp

maflcko · 2019-01-26T00:41:47Z

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.

promag · 2019-01-26T00:46:29Z

@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.

sipa · 2019-01-26T00:51:07Z

@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.

maflcko · 2019-01-26T00:52:37Z

Ah, that makes it clearer.

sipa · 2019-01-26T00:55:24Z

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.

DrahtBot · 2019-01-26T01:12:31Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15329 (Fix InitError() and InitWarning() content by hebasto)
#14169 (add -debuglogsize= option by SuckShit)
#13746 (-masterdatadir for datadir bootstrapping by kallewoof)
#13088 (Log early messages with -printtoconsole by ajtowns)

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.

src/sync.cpp

promag

ACK faacd53.

src/logging.cpp

src/sync.cpp

src/util/memory.h

maflcko · 2019-01-29T19:20:56Z

Reverted back to my initial patch (using the LogInstance wording suggested here #15266 (comment))

ryanofsky

utACK fa8e2b75ff7de6b09f97512256092c1d03edef01

src/logging.cpp

ryanofsky

utACK 77777c5. Just comment change since last review. Also the commit hash no longer begins with fa (in case that is a problem).

jonasschnelli · 2019-02-04T19:07:50Z

utACK 77777c5

77777c5 log: Construct global logger on first use (MarcoFalke) Pull request description: 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 bitcoin#15111) Tree-SHA512: eb9c22f4baf31ebc5b0b9ee6a51d1354bae1f0df186cc0ce818b4483c7b5a7f90268d2b549ee96b4c57f8ef36ab239dc6497f74f3e2ef166038f7437c368297d

e1b6436 Fix build after pr 15266 merged (Hennadii Stepanov) Pull request description: Ref: #15266 Tree-SHA512: 6647264c3e3d94f0f10dc3bed1b82dfe8ed1192906270b0bb79f4d018807e06cb42286c86ced7fee0e2b38284739657336672c5a30650b6473ffafd65c315349

77777c5 log: Construct global logger on first use (MarcoFalke) Pull request description: 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 bitcoin#15111) Tree-SHA512: eb9c22f4baf31ebc5b0b9ee6a51d1354bae1f0df186cc0ce818b4483c7b5a7f90268d2b549ee96b4c57f8ef36ab239dc6497f74f3e2ef166038f7437c368297d

Merge bitcoin#15334, bitcoin#13076, bitcoin#15266, bitcoin#13128

maflcko force-pushed the Mf1901-cofu branch from fa889e8 to fae59d2 Compare January 25, 2019 21:01

maflcko added Refactoring Utils/log/libs labels Jan 25, 2019

maflcko force-pushed the Mf1901-cofu branch from fae59d2 to fadf76a Compare January 25, 2019 21:52

promag reviewed Jan 25, 2019

View reviewed changes

src/sync.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the Mf1901-cofu branch 2 times, most recently from fa5e96c to fad73c1 Compare January 26, 2019 02:07

AkioNak reviewed Jan 26, 2019

View reviewed changes

src/sync.cpp Outdated Show resolved Hide resolved

src/sync.cpp Outdated Show resolved Hide resolved

src/sync.cpp Outdated Show resolved Hide resolved

promag reviewed Jan 26, 2019

View reviewed changes

src/sync.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the Mf1901-cofu branch 2 times, most recently from fa169ff to faacd53 Compare January 27, 2019 03:37

promag reviewed Jan 27, 2019

View reviewed changes

src/logging.cpp Outdated Show resolved Hide resolved

src/sync.cpp Outdated Show resolved Hide resolved

sipa reviewed Jan 28, 2019

View reviewed changes

src/util/memory.h Outdated Show resolved Hide resolved

sipa reviewed Jan 28, 2019

View reviewed changes

src/util/memory.h Outdated Show resolved Hide resolved

ryanofsky reviewed Jan 28, 2019

View reviewed changes

src/util/memory.h Outdated Show resolved Hide resolved

maflcko force-pushed the Mf1901-cofu branch 3 times, most recently from fa978d1 to fa8e2b7 Compare January 29, 2019 19:15

ryanofsky approved these changes Jan 29, 2019

View reviewed changes

src/logging.cpp Outdated Show resolved Hide resolved

log: Construct global logger on first use

77777c5

maflcko force-pushed the Mf1901-cofu branch from fa8e2b7 to 77777c5 Compare January 29, 2019 20:30

maflcko added this to the 0.18.0 milestone Feb 1, 2019

practicalswift mentioned this pull request Feb 1, 2019

Avoid UB (member call on nullptr) when failing to read randomness in the startup process #15111

Closed

ryanofsky approved these changes Feb 4, 2019

View reviewed changes

maflcko merged commit 77777c5 into bitcoin:master Feb 4, 2019

maflcko deleted the Mf1901-cofu branch February 4, 2019 19:30

hebasto mentioned this pull request Feb 4, 2019

Fix build after pr 15266 merged #15347

Merged

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Sep 16, 2021

Merge pull request #4420 from vijaydasmp/backport_v18_vijay_batch3_1

78e04b9

Merge bitcoin#15334, bitcoin#13076, bitcoin#15266, bitcoin#13128

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

maflcko commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jan 25, 2019

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Jan 26, 2019

Uh oh!

promag commented Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 26, 2019

Uh oh!

sipa commented Jan 26, 2019

Uh oh!

DrahtBot commented Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jan 29, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Feb 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maflcko commented Jan 25, 2019 •

edited

Loading

promag left a comment •

edited

Loading

promag commented Jan 26, 2019 •

edited

Loading

sipa commented Jan 26, 2019 •

edited

Loading

DrahtBot commented Jan 26, 2019 •

edited

Loading