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
Fix static initializer priority for namespaces in LTO builds #6591
Fix static initializer priority for namespaces in LTO builds #6591
Conversation
lib/cli/daemonutility.cpp
Outdated
#include "config/configcompiler.hpp" | ||
#include "config/configcompilercontext.hpp" | ||
#include "config/configitembuilder.hpp" | ||
|
||
#include <iostream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you forgot to remove some debug changes :P
lib/cli/daemonutility.cpp
Outdated
@@ -145,7 +147,14 @@ bool DaemonUtility::ValidateConfigFiles(const std::vector<std::string>& configs, | |||
if (!success) | |||
return false; | |||
|
|||
#ifdef I2_DEBUG | |||
//std::cout << "Global NS: " << Serialize(ScriptGlobal::GetGlobals()) << std::endl; | |||
#endif /* I2_DEBUG */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's on purpose, also the above include.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's dead code 💀
lib/base/scriptframe.cpp
Outdated
@@ -51,7 +55,7 @@ INITIALIZE_ONCE_WITH_PRIORITY([]() { | |||
|
|||
Namespace::Ptr internalNS = new Namespace(l_InternalNSBehavior); | |||
globalNS->SetAttribute("Internal", std::make_shared<ConstEmbeddedNamespaceValue>(internalNS)); | |||
}, 50); | |||
}, 1000); /* This. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above which explains the priority. I fear that this is changed or overseen in the future.
Tested with Debian Jessie and gcc 8.2.0. With LTO and without, with LTO Icinga 2 does not terminate properly:
This happens when run in the foreground as well as when started/stopped as a service. I'll rebuild the master with LTO enabled to find out whether this is a problem with this patch or with LTO builds themself. |
That's a known problem with debug builds, you won't see that with release builds. I'll drop the dead code for now, just for you :-P |
d9d86f1
to
19993df
Compare
Tested with both
ICINGA2_LTO_BUILD=OFF/ON
.fixes #6575