Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memory: Construct globals on first use #15266

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
7 participants
@MarcoFalke
Copy link
Member

commented Jan 25, 2019

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)

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-cofu branch from fa889e8 to fae59d2 Jan 25, 2019

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-cofu branch from fae59d2 to fadf76a Jan 25, 2019

@promag
Copy link
Member

left a comment

An alternative is to use templating and also defer instantiation to operator->(). This way the diff would be smaller and the code less verbose.

```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.
Show resolved Hide resolved src/sync.cpp Outdated
@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2019

Ah, that makes it clearer.

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2019

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.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-cofu branch 2 times, most recently from fa5e96c to fad73c1 Jan 26, 2019

Show resolved Hide resolved src/sync.cpp Outdated
Show resolved Hide resolved src/sync.cpp Outdated
Show resolved Hide resolved src/sync.cpp Outdated
Show resolved Hide resolved src/sync.cpp Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-cofu branch 2 times, most recently from fa169ff to faacd53 Jan 26, 2019

@promag
Copy link
Member

left a comment

ACK faacd53.

Show resolved Hide resolved src/logging.cpp Outdated
Show resolved Hide resolved src/sync.cpp Outdated
Show resolved Hide resolved src/util/memory.h Outdated
Show resolved Hide resolved src/util/memory.h Outdated
Show resolved Hide resolved src/util/memory.h Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-cofu branch 3 times, most recently from fa978d1 to fa8e2b7 Jan 29, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

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

@ryanofsky
Copy link
Contributor

left a comment

utACK fa8e2b7

Show resolved Hide resolved src/logging.cpp

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-cofu branch from fa8e2b7 to 77777c5 Jan 29, 2019

@ryanofsky
Copy link
Contributor

left a comment

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

utACK 77777c5

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 4, 2019

Merge bitcoin#15266: memory: Construct globals on first use
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

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1901-cofu branch Feb 4, 2019

laanwj added a commit that referenced this pull request Feb 5, 2019

Merge #15347: Fix build after pr 15266 merged
e1b6436 Fix build after pr 15266 merged (Hennadii Stepanov)

Pull request description:

  Ref: #15266

Tree-SHA512: 6647264c3e3d94f0f10dc3bed1b82dfe8ed1192906270b0bb79f4d018807e06cb42286c86ced7fee0e2b38284739657336672c5a30650b6473ffafd65c315349
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.