-
Notifications
You must be signed in to change notification settings - Fork 449
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
Add new library class #116
Conversation
…form way. The native OS calls are wrapped behind a class interface. Highlights: - handling of file suffixes (.dll or .so) - automatic unload of library on program exit - etrieving of loaded types in library - avoid of unncessary loading calls, when same plugin is loaded multiple times additional: - added unit tests - added example of loading a plugin
calling "dlclose()" does not unload the library immediately. We have to define a "constructor" and "destructor" function, in order to initialize and deinitialize our plugings. Additionally, for gcc based compilers we have to use a compiler flag: "-fno-gnu-unique"
auto var = t.create(); | ||
auto value = t.invoke("some_method", var, {}); | ||
std::cout << value.to_string() << std::endl; | ||
} // all data has to be remove before unload the library, otherwise we get UB |
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.
is this generally like this, or is this a UB due to rttr?
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.
This is general, when you unload the library, how can you access the data anymore (e.g. the dtor to remove the data from the variant
?
However, you are safe to ignore the explicit unload()
call, it will be called automatically on application exit. But when you unload the library explicitly, you have to make sure every data is removed.
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.
ah makes sense :-)
@gabyx |
} | ||
m_state_saver.save_state_begin(); | ||
|
||
auto result = load_native(); |
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.
if I understand correctly and as far as I see so far: the load method can be called from multiple threads?
if so, isnt there some troubles with thread-safety, maybe I am missing something: A thread A can potentially be in load_native() which is just about to set the m_handle and another thread B can be at line 73 which then loads the library again, right? (thats not what you want right?)
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.
You are absolutely right. This is missing. There were some compilers which didn't support threading at all, I think it was minGW 4.9.1.
Mhm, I guess I have to lift the requirements for this. Or at a fallback with using boost.
/~https://github.com/meganz/mingw-std-threads
When you know something, please tell me.
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.
hm.. ok, I am suspecting that the whole function should be guarded (isnt it?) (not sure about the m_state_saver
.) So to be absolutely sure that no other thread is currently trying to load the library.
(I have never loaded a library with dlopen so far, what I understand is that if one thread loads it, the other thread can use it too because its in shared memory, because of your singelton implementation).
I think the only thing we need here is some mutex or thread guard for the library_manager
such to prevent race condition of threads loading / unloading the library at the same time (isnt it?). std::mutex
?
src/rttr/library.cpp
Outdated
|
||
/*! | ||
* A simple manager class, to hold all created library_private pointer objects. A private singleton. | ||
* After application exit he will unload all libs without any reference. |
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.
exit, it will
src/rttr/library.cpp
Outdated
* A simple manager class, to hold all created library_private pointer objects. A private singleton. | ||
* After application exit he will unload all libs without any reference. | ||
*/ | ||
class library_manager |
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.
is this library manager thread safe?
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.
i mean the remove_item and create_or_find_library...
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.
not thread safe atm.
|
||
CHECK(does_plugin_type_exist() == true); | ||
|
||
CHECK(lib1.unload() == false); |
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.
after lib1.unload(), lib1 is not more valid (right?)
but there is lib2 which is still there (the same actually)
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 library
class has no valid or invalid state.
However, the same library is still loaded by another instance (ref count increased)
That's why the unload fails.
CHECK(does_plugin_type_exist() == true); | ||
|
||
CHECK(lib1.unload() == false); | ||
CHECK(lib1.unload() == false); |
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.
another unload -> false because there is still lib2 there
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 second unload does nothing, it was still unloaded.
And yes lib2 is still loaded, so no unload at all possible.
|
||
CHECK(does_plugin_type_exist() == true); | ||
|
||
CHECK(lib2.unload() == true); |
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.
ok now all load counters decreased to zero -> library unloaded
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.
correct.
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.
As far as my competence allows: there are some comments which are probably non-sense, but it would be helpful anyway to get your answer. a part from that: impecable style!
I was mainly distracted from the atomic load counter, which seems to be there only for multiple threads?, if so I am not sure about thread safetyness.
Also, this plugin functionality is probably extremely useful. Easy way of loading the library, and getting the types and then construct them
@gabyx |
@acki-m: I have only xtools 8.2 on my mac at home (10 years old), the warnings come from 9.2, I will see if I can find the issue... |
…bles not supported
and also fixed not loading of plugins, because of wrong implementation errors in "std::codecvt_utf8_utf16<wchar_t>" for mingw (fixed in >7.0.0)
and increased compiler for mingw test on appveyor
…LUGIN_REGISTRATION added more library tests
- made it possible to bin raw arrays as property - added another cpp file for the test plugin, in order to test if these types will be also loaded
@gabyx For all other system it looks good |
I experience something fishy: Sometimes the
Smells like an uninitialized variable? |
src/rttr/library.cpp
Outdated
@@ -29,6 +29,9 @@ | |||
#include "rttr/detail/library/library_p.h" | |||
|
|||
#include <map> | |||
#include <mutex> | |||
|
|||
static std::mutex g_library_mutex; |
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.
wouldn't it be better to put this variable in the library_manager
since it is already a singleton and physically belongs to it (why a global?, ok it is a static
meaning only in this translation unit when I understand correct) and thread-safe initialized because of the magic static
s static library_manager obj;
I see the following:
var_point_list has been initialized with the
initialized here the I initialized the array with {1,2,3...,100} to better see it in the debugger (debugging with VS Code, not so handy but it works somehow...) |
Thanks you Gabriel for looking into it. Why are you using VS code? Is the native XCode debugger that bad? When you have a little bit more time, can you check something for me? |
Ahh wrong: Its a random bug, need to catch it when its false... |
the data is completely invalid. So it seems to be used after it was freed. How can this be... |
nono, dont trust the lldb, its visualizer are garbage... |
or do you have another indication some integrals or so which are invalid? |
are address sanitizers enabled for clang?... will see |
compiled with clang 7.0 and address and leak sanitzer enabled. -> all tests pass multiple times -> no errors... I have no clue. The problem somehow is there with clang 6.0, I will see what the problem is tomorrow or on Sunday... |
@gabyx rttr/src/rttr/detail/type/type_data.h Line 358 in d32394c
I create in every module a static invalid type_data object. There from the address will be stored for invalid types (default ctor of ``type` class): rttr/src/rttr/detail/type/type_impl.h Line 52 in d32394c
In one of the last PR; I adjusted the behaviour how types are compared, instead of using a incremental index, I just use directly the pointer address: rttr/src/rttr/detail/type/type_impl.h Line 110 in d32394c
That must have resulted to some problem in the code where types were compared. Don't know yet where. The memory order must be different on apple clang. A very tricky bug. Damn it. Thanks for looking into it. I hope I can merge it this WE. |
So when you compare types: you compare pointers, so does this explain why I see sometimes valid=true or false (why?) |
The problem was that the invalid type_data object was created inside the library module statically. Because we switched to using the pointer compare of types, instead of using and separate ID, this lead to problems when comparing two invalid types. They were the same. Which specific code which lead to the problem is unclear. However, this behaviour was wrong and is now fixed. Additional: no clang memory or address sanitizer could find the source. The problem was only poping up in MacOSX with apple clang.
So we use "MultiByteToWideChar" and "WideCharToMultiByte" like recommend from MSVC.
Yes, the |
The error is still present.
|
so the variant_sequential_view has been deleted before (or is it some submember?) |
so the type_data is somehow gone...? |
What I see is, the data accessed is already freed in test ____C_A_T_C_H____T_E_S_T____0() What do you mean with |
@gabyx PS: I could write a whole blog post about this bug...took me sooo long |
This is great - a very common use case! I'd love an example of using this for a plug-in, as described in the FAQ
I am puzzling though tutorial, docs, and code |
Actually the problem was also present under linux, (with gcc or clang). This situation was following: 1. A plugin was loaded with type "int[100]" and registered 2. The plugin was unloaded => "int[100]" was removed from the type system and deleted 3. The type[100] was used in the unit_test later and was accessing the memory of the already deleted type_data The reason for this behavioue lies in the usage of local static member variables (which is used to cache the retrieving and registering of type_data information). This variable was process wide unique, this lead to problems when unloading a plugin. The static was no reinitialized when unloading the plugin. So the variable was pointing to type_data information which was alreadyl deleted. Now we are using RTTR_LOCAL to hide the symbols for exporting. This fixes the issues.
@dand-oss |
* additionally: added note, how to compile plugins with GCC (-fno-gnu-unique)
renamed json_serialization to json_example
This adds the functionality to load libraries in a simple cross platform way.
The native OS calls are wrapped behind a class interface.
Highlights:
additional: