On Thu, Oct 27, 2016 at 3:04 PM, Nadav Har'El <n...@scylladb.com> 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 <ben...@scylladb.com>
>> ---
>> 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 osv-dev+unsubscr...@googlegroups.com.
>> 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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
