Fix for singleton in Profiler #694
Conversation
Signed-off-by: Maksim Derbasov <[email protected]>
|
@osrf-jenkins run tests please |
|
Not sure how to check whole report, but I kinda agree with ABI checker Before this symbol links inside each translation unit and now they(other libraries) have to call endpoint in profiler library to get profiler. And it can be only fixed by rebuilding other libraries (but for enabling profile we have to rebuild it anyway) (sorry, used my working account to answer, wrong browser window) |
| static Profiler profiler; | ||
| return &profiler; |
There was a problem hiding this comment.
| static Profiler profiler; | |
| return &profiler; | |
| return SingletonT<Profiler>::Instance(); |
I think doing this might maintain ABI compatibility. Can you give it a try?
There was a problem hiding this comment.
AFAIS report complaining on code, not on ABI (which I mentioned in comment above).
I believe ABI checker checking for disappearing some symbols, but here we have new symbol which should be fine
I built both versions (with and w/o change) and checked difference in symbols in libgz-common5-profiler.so.5.7.1 library.
And difference is couple o new symbols (some of them are public(capital letter), some of them private)
# diff clear_build/lib/out.txt branch/lib/out.txt
51a52
> b _ZGVZN2gz6common8Profiler8InstanceEvE8profiler
72a74,75
> T _ZN2gz6common8Profiler8InstanceEv
> t _ZN2gz6common8Profiler8InstanceEv.cold
116a120
> b _ZZN2gz6common8Profiler8InstanceEvE8profiler
187a192,194
> U __cxa_guard_abort@CXXABI_1.3
> U __cxa_guard_acquire@CXXABI_1.3
> U __cxa_guard_release@CXXABI_1.3
command to get symbols w/o addresses: gz-common/branch/lib# nm libgz-common5-profiler.so.5.7.1 | cut -c18- > out.txt
I will try proposed code but I have big doubts about CI result change in this case
From source perspective it's big change, before symbol was inlined in every translation unit and now implementation hidden inside
There was a problem hiding this comment.
The code clearly removes the SingletonT<Profiler>::__ct [C1] ( SingletonT<Profiler>const& p1 ) symbol because it no longer uses creates a SingletonT<Profiler> object, so in that sense the ABI checker is correct. However, like you said since those symbols are inlined, it's probably not a problem. But for added safety, I'd prefer if we can get our ABI checker ✅ .
There was a problem hiding this comment.
Interesting mangling, first time see this way, which platform is it? I guess it's a copy constructor for class template Singleton. Which I guess will be or private symbol, or inlined.
I pushed proposed version and got same result https://build.osrfoundation.org/job/gz_common-abichecker-any_to_any-ubuntu-jammy-amd64/133/:
Binary compatibility: 100%
Source compatibility: 99.1%
I guess it's all about hiding Instance member function from Singleton parent.
If you OK, I'd prefer to proceed with static variable. As I said before, I'd really suggest to get rid of Singleton class due to it's just making more confusion and technically trying to replace 2 line function
There was a problem hiding this comment.
Okay. Thanks for trying. I was hoping we would get a green CI. But since that's not going to happen, I would suggest a slightly different approach of using the gz::utils::NeverDestroyed class https://github.com/gazebosim/gz-sim/blob/f4a3b61b333ae822d5afbd2f8cae43d9ad361004/src/ComponentFactory.cc#L22-L26
This class is designed to work well with address sanitizers and avoids the static initialization order fiasco.
Also, I totally agree that we should remove the SingletonT class. If you are interested in helping out with removing the SingletonT class from our code base, please go right ahead :) (target main branches please).
There was a problem hiding this comment.
About static initialization order fiasco, it refer to objects in global scope, which initialization happens before main(). Here we have a Meyers Singleton which provide a lazy initialization on 1st access. Also it happens thread-safe from C++11. So it's not this case
https://www.modernescpp.com/index.php/c-20-static-initialization-order-fiasco/
There was a problem hiding this comment.
And about NeverDestroyed (very short version). IMHO it's a very dirty hack.
- Valgrind will see indirect memory leak if object inside it allocates memory.
- AFAIU it was invented for cases of library loading with dlopen and unloaded before termination. In this case code of destructor of static object is unloaded and attempt to call it will lead to crash on termination (I saw similar case with destructor of exception on plugin loading mechanism in one of project where I worked)
- I guess it can be treated as UB. Can deep dive in standard to find more accurate interpretation, but it will take a lot of time
There was a problem hiding this comment.
Also, I totally agree that we should remove the SingletonT class. If you are interested in helping out with removing the SingletonT class from our code base, please go right ahead :) (target main branches please).
I guess I can, just want to close other PRs which I opened (2 in this repo, 1 in gz-sim)
Signed-off-by: Maksim Derbasov <[email protected]>
Signed-off-by: Maksim Derbasov <[email protected]>
🦟 Bug fix
Fixes #
Summary
Recurring problem with dangerous
SingletonT:OptixRenderEngineis still affectedIn this PR I propose similar to MeshManager change (keep inheritance, add method which hides method from base class).
Also added comment to
SingletonT(tbh, I'd rather remove usage of it across the product, deprecate and remove)I target it to common5 to start discussion about better way of handling this problem. But just merging is still fine, I'm just user.
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.