VILLASframework / node

Connecting real-time power grid simulation equipment
https://fein-aachen.org/projects/villas-node/
Apache License 2.0
13 stars 7 forks source link

Fix static initialization #800

Closed n-eiling closed 3 months ago

n-eiling commented 3 months ago

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 commented 3 months ago

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

stv0g commented 3 months ago

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:

n-eiling commented 3 months ago

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

stv0g commented 3 months ago

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 commented 3 months ago

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

n-eiling commented 3 months ago

@stv0g