Skip to content

Fix static initialization#800

Merged
stv0g merged 1 commit intomasterfrom
fix-static-initialization
Aug 5, 2024
Merged

Fix static initialization#800
stv0g merged 1 commit intomasterfrom
fix-static-initialization

Conversation

@n-eiling
Copy link
Copy Markdown
Member

fixes #799

I replaced the static constructor call of villas::logging and HostRamAllocator::allocator to a function that does lazy initialization (i.e., on the first call).
This avoid the undefined behavior mentioned in the issue and even avoid an explicit initialization order.
The implementation leak heap memory that will be implicitly cleaned up when the application finished. While currently not a problem, the destructors of Log and HostRamAllocator should not assume the existence of other static objects, because the deinitialization order is still undefined.

I implemented the C++ Foundaitions recommendation for this issue (see https://isocpp.org/wiki/faq/ctors#static-init-order)

This PR allows using link time optimization (-lto gcc flag) with VILLASnode.

@n-eiling n-eiling requested review from stv0g and windrad6 as code owners July 29, 2024 11:50
@n-eiling n-eiling force-pushed the fix-static-initialization branch 2 times, most recently from dd76497 to 25075e8 Compare July 29, 2024 12:08
Comment thread common/include/villas/memory.hpp
Copy link
Copy Markdown
Contributor

@stv0g stv0g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have a small request :)

But thanks a lot for the effort and debugging the issue :)

@n-eiling n-eiling force-pushed the fix-static-initialization branch from 25075e8 to 2fe3b92 Compare July 29, 2024 13:30
@n-eiling
Copy link
Copy Markdown
Member Author

I updated the PR. Was this what you thought of?

@stv0g
Copy link
Copy Markdown
Contributor

stv0g commented Jul 29, 2024

Yes great :)

Just wondering did you want to keep the inconsistency intentionally?

HostRamAllocator::getAllocator vs Log::get()

I would opt for one of the following:

  • HostRamAllocator::get() & Log::get()
  • HostRamAllocator::getAllocator() & Log::getLogger()

@n-eiling
Copy link
Copy Markdown
Member Author

n-eiling commented Jul 29, 2024

Hmm difficult ... The choice is
HostRam::HostRamAllocator::getInstance()
or
HostRam::HostRamAllocator::get()
or
HostRam::getAllocator()

I think I still prefer the last one.

For Log there is now
Log::getInstance() to get what was originally villas::logger, and Log::get() for what was originally villas::logger.get(). This avoids one function call compared to Log::getInstance().get(), which should be slightly better for performance. (But shouldn't matter with -lto).

@n-eiling n-eiling force-pushed the fix-static-initialization branch from 2fe3b92 to 3af1372 Compare July 29, 2024 14:14
@stv0g
Copy link
Copy Markdown
Contributor

stv0g commented Jul 30, 2024

I think I still prefer the last one.

Okay, then lets go also with Log::getLogger(). Especially if there is also Log::getInstance(). Because otherwise, I would ask myself: what do we get() here?

@n-eiling n-eiling force-pushed the fix-static-initialization branch from 3af1372 to b12ac09 Compare July 31, 2024 09:54
@n-eiling
Copy link
Copy Markdown
Member Author

n-eiling commented Aug 1, 2024

Was this ok now, or do you want me to change anything?

@n-eiling
Copy link
Copy Markdown
Member Author

n-eiling commented Aug 5, 2024

@stv0g

Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
@n-eiling n-eiling force-pushed the fix-static-initialization branch from b12ac09 to cb085db Compare August 5, 2024 10:33
@stv0g stv0g merged commit f25e1dd into master Aug 5, 2024
@stv0g stv0g deleted the fix-static-initialization branch August 5, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

static logger initialization and subsequent static initializations using logging.get is undefined behavior

3 participants