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