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 <benoit.canet.contrib@gmail. > com> 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. > > 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. > > > >> >> 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.
