On Mon, Oct 31, 2016 at 9:48 AM, Benoît Canet <[email protected]> wrote:
> > > On Thu, Oct 27, 2016 at 3:04 PM, Nadav Har'El <[email protected]> wrote: > >> >> On Thu, Oct 27, 2016 at 3:06 PM, Benoît Canet < >> [email protected]> wrote: >> >>> We want the init of the elf object to use >>> the tls defined when the new thread is started. >>> >>> Delay this initialization. >>> >> >> So, program::get_library() runs some code (the library's initialization >> thread), and we want it to run a new thread. So wouldn't it make more sense >> to just create that thread *before* calling get_library(), instead of >> splitting the get_library() interface to two and complicating everything? >> > > init_static_tls must be called _before_ the new thread is created else it > cause random crashes. > So it looks like we have here a pre-existing bug in the way static TLS (the "initial-exec" model) works. The program::get_library() function does not, and should not, create a new thread (only the osv::application wrapper does). One can load a library and call a function from it in the current thread, without creating a new thread. The problem is that get_library() calls init_static_tls() which, as far as I remember only changes the way that new threads will be created (when they will call setup_tcb()) - and does not modify the current thread's TLS area. So in a program which uses static TLS (the initial-exec model), and runs code from the library without starting a new thread, it will not have access to its TLS variables. By the way, I think we have an even bigger mess there. It turns out that setup_tcb() knows which objects' static TLS to set up for the new thread by using this thread's thread::_app_runtime. Which means that this thing will not work correctly without using osv::application anyway - someone will not be able to run code with static TLS by using get_library() alone anyway. An additonal problem: I recently modified osv::run() to not create a new thread - so now that would not work on code with static TLS. One last complication: Even if get_library() did modify the current thread's tls, who would restore it when the library is unloaded? This is one big messy situation :-( > > >> >> By the way, I'm curious why Go cares on which thread the initialization >> happens.... Does the share library only work on one thread, the same thread >> it was initialized on? >> > > Because a new thread will allocate the TLS. > All this discussion explained why we need to call init_static_tls() on the parent thread - not it was important to delay the shared object's initialization until the child thread. > > > >> >> >> >>> >>> Signed-off-by: Benoît Canet <[email protected]> >>> --- >>> core/app.cc | 4 +++- >>> core/elf.cc | 49 ++++++++++++++++++++++++++++++ >>> ++++++++++++++----- >>> include/osv/elf.hh | 6 +++++- >>> 3 files changed, 52 insertions(+), 7 deletions(-) >>> >>> diff --git a/core/app.cc b/core/app.cc >>> index 773d80e..0bfcedd 100644 >>> --- a/core/app.cc >>> +++ b/core/app.cc >>> @@ -174,7 +174,8 @@ application::application(const std::string& command, >>> >>> merge_in_environ(new_program, env); >>> prepare_argv(); >>> - _lib = current_program->get_library(_command); >>> + std::vector<std::string> extra_path; >>> >> >> Nitpick: I think you could pass {} below for an empty vector instead of >> adding this extra variable. >> >> >>> + _lib = current_program->get_library(_command, extra_path, >>> true); >>> } catch(const std::exception &e) { >>> throw launch_error(e.what()); >>> } >>> @@ -278,6 +279,7 @@ void application::main() >>> { >>> __libc_stack_end = __builtin_frame_address(0); >>> >>> + elf::get_program()->init_library(); >>> sched::thread::current()->set_name(_command); >>> >>> if (_main) { >>> diff --git a/core/elf.cc b/core/elf.cc >>> index efa574e..bf2137c 100644 >>> --- a/core/elf.cc >>> +++ b/core/elf.cc >>> @@ -1164,25 +1164,64 @@ program::load_object(std::string name, >>> std::vector<std::string> extra_path, >>> } >>> >>> std::shared_ptr<object> >>> -program::get_library(std::string name, std::vector<std::string> >>> extra_path) >>> +program::get_library(std::string name, std::vector<std::string> >>> extra_path, bool no_init) >>> { >>> SCOPE_LOCK(_mutex); >>> std::vector<std::shared_ptr<object>> loaded_objects; >>> auto ret = load_object(name, extra_path, loaded_objects); >>> + >>> + // Push the loaded object on a stack >>> + // init_library is to be called later at an arbitraty time >>> + // and operate on the list of loaded_objects. Since a library >>> + // can load another one like java.so does in OSv we want a stack >>> + // structure so each init_library call get it's corresponding >>> + // list of objects to operate on. >>> + >>> + // Build a vector of weak pointer so it will not prevent anyone >>> + // from unloading a .so properly while giving init_library some >>> + // safety. >>> + std::vector<std::weak_ptr<object>> weak_objects; >>> + for (auto obj: loaded_objects) { >>> + weak_objects.push_back(obj); >>> + } >>> + // push the weak pointer on the stack >>> + _loaded_objects_stack.push(weak_objects); >>> >> >> Why did you need the temporary weak_objects vector? Why not push the >> objects directly on weak_objects? >> >> By the way, if you simply pass _loaded_objects_stack itself (not the >> local loaded_objects) variable to the load_object() function, wouldn't this >> do exactly what you want, without needing to copy things again? If it's >> weak pointers you wanted instead of normal shared pointers, we could modify >> load_object to work with a vector of weak pointers, I guess (I'm not sure >> exactly why you didn't want to work with normal shared pointers...). >> >> + >>> if (ret) { >>> ret->init_static_tls(); >>> } >>> + >>> + if (no_init) { >>> + return ret; >>> + } >>> + >>> + init_library(); >>> >> + >>> + return ret; >>> >> >> Wouldn't it be simpler to write this logic as? >> >> if (!no_init) { >> init_library(); >> } >> return ret; >> >> >> >>> +} >>> + >>> +void program::init_library() >>> +{ >>> + // get the list of weak pointers before iterating on them >>> + std::vector<std::weak_ptr<object>> weak_objects = >>> + _loaded_objects_stack.top(); >>> + >>> // After loading the object and all its needed objects, run these >>> objects' >>> // init functions in reverse order (so those of deepest needed >>> object runs >>> // first) and finally make the loaded objects visible in search >>> order. >>> - auto size = loaded_objects.size(); >>> + auto size = weak_objects.size(); >>> for (int i = size - 1; i >= 0; i--) { >>> - loaded_objects[i]->run_init_funcs(); >>> + if (auto obj = weak_objects[i].lock()) { >>> + obj->run_init_funcs(); >>> + } >>> } >>> for (unsigned i = 0; i < size; i++) { >>> - loaded_objects[i]->setprivate(false); >>> + if (auto obj = weak_objects[i].lock()) { >>> + obj->setprivate(false); >>> + } >>> } >>> - return ret; >>> + >>> + _loaded_objects_stack.pop(); >>> } >>> >>> void program::remove_object(object *ef) >>> diff --git a/include/osv/elf.hh b/include/osv/elf.hh >>> index fabc6e6..d26b22c 100644 >>> --- a/include/osv/elf.hh >>> +++ b/include/osv/elf.hh >>> @@ -11,6 +11,7 @@ >>> #include "fs/fs.hh" >>> #include <vector> >>> #include <map> >>> +#include <stack> >>> #include <memory> >>> #include <unordered_set> >>> #include <osv/types.h> >>> @@ -525,7 +526,9 @@ public: >>> * set_search_path(). >>> */ >>> std::shared_ptr<elf::object> >>> - get_library(std::string lib, std::vector<std::string> extra_path = >>> {}); >>> + get_library(std::string lib, std::vector<std::string> extra_path = >>> {}, bool no_init = false); >>> + >>> + void init_library(); >>> >>> /** >>> * Set the default search path for get_library(). >>> @@ -596,6 +599,7 @@ private: >>> >>> friend elf::file::~file(); >>> friend class object; >>> + std::stack<std::vector<std::weak_ptr<object>>> >>> _loaded_objects_stack; >>> }; >>> >>> void create_main_program(); >>> -- >>> 2.7.4 >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "OSv Development" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to [email protected]. >>> For more options, visit https://groups.google.com/d/optout. >>> >> >> > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
