On Mon, Oct 31, 2016 at 9:30 AM, Nadav Har'El <[email protected]> wrote:
> > 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. > Go initialization function will touch the non statis tls area. So the thread creation need to allocate them first else the following call to the go runtime will just find a blank tls are and be very confused. > >> >> >>> >>> >>> >>>> >>>> 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.
