When you send a second version of a patch, please label it with "v2" and so on (if you use git format-patch, you can do "git format-patch -v2" to do this automatically) and write below the fold (the "--" line) some comments about what changed. Because it's hard for me to keep track what are new patches and what are modified versions and what was modified.
In the present case, this appears to be a second version of your earlier patch "[PATCH 2/3] look up symbol deeper". I had some review comments in that version, that I think you forgot to address. In that version, I was worried that the "DT_NEEDED" graph may have cycles, and cause your code to go into an infinite loop. I asked if you believe (and why) it can't have cycles. If it can, I suggested you can use object::collect_dependencies(), which builds a list of all modules you need to look at (dependencies and dependencies of dependencies and ...) - without a risk of cycles because the DFS walk uses a set to avoid walking the same node more than once. (by the way, the dlsym() manual says that it does a BFS walk over the dependencies. object::collect_dependencies() which I suggested above does DFS, not BFS. We need to consider if this makes any difference and if we need to do a BFS version of collect_dependencies()... I hope not, but I don't know). Thanks, Nadav. -- Nadav Har'El [email protected] On Wed, Jan 15, 2020 at 7:39 PM Zhiting Zhu <[email protected]> wrote: > dlsym function doesn't seem to lookup symbol deep enough. This > patch fixes test_dlsym_from_sofile_with_preload and > dlsym_with_dependencies > > Signed-off-by: Waldemar Kozaczuk <[email protected]> > Signed-off-by: Zhiting Zhu <[email protected]> > --- > core/elf.cc | 14 ++++++++++++++ > include/osv/elf.hh | 1 + > libc/dlfcn.cc | 2 +- > tests/tst-dlfcn.cc | 2 +- > 4 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/core/elf.cc b/core/elf.cc > index 73834b19..26a25c43 100644 > --- a/core/elf.cc > +++ b/core/elf.cc > @@ -950,6 +950,20 @@ Elf64_Sym* object::lookup_symbol(const char* name, > bool self_lookup) > return sym; > } > > +symbol_module object::lookup_symbol_deep(const char* name) > +{ > + symbol_module sym = { lookup_symbol(name, false), this }; > + if (!sym.symbol) { > + for (auto&& _obj : _needed) { > + sym = _obj->lookup_symbol_deep(name); > + if (sym.symbol) { > + break; > + } > + } > + } > + return sym; > +} > + > unsigned object::symtab_len() > { > if (dynamic_exists(DT_HASH)) { > diff --git a/include/osv/elf.hh b/include/osv/elf.hh > index 1dac6c08..8cd7b37a 100644 > --- a/include/osv/elf.hh > +++ b/include/osv/elf.hh > @@ -347,6 +347,7 @@ public: > void* base() const; > void* end() const; > Elf64_Sym* lookup_symbol(const char* name, bool self_lookup); > + symbol_module lookup_symbol_deep(const char* name); > void load_segments(); > void process_headers(); > void unload_segments(); > diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc > index 346cf195..e3a4a577 100644 > --- a/libc/dlfcn.cc > +++ b/libc/dlfcn.cc > @@ -74,7 +74,7 @@ void* dlsym(void* handle, const char* name) > abort(); > } else { > auto obj = > *reinterpret_cast<std::shared_ptr<elf::object>*>(handle); > - sym = { obj->lookup_symbol(name, false), obj.get() }; > + sym = obj->lookup_symbol_deep(name); > } > if (!sym.obj || !sym.symbol) { > dlerror_fmt("dlsym: symbol %s not found", name); > diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc > index 5258256a..6d91fb93 100644 > --- a/tests/tst-dlfcn.cc > +++ b/tests/tst-dlfcn.cc > @@ -13,7 +13,7 @@ > namespace utf = boost::unit_test; > > const bool rtld_next = false; > -const bool deep_lookup = false; > +const bool deep_lookup = true; > > static bool called = false; > extern "C" void DlSymTestFunction() > -- > 2.17.1 > > -- > 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]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/osv-dev/20200115173946.76109-1-zhitingz%40cs.utexas.edu > . > -- 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]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CANEVyjuF-6X%2BOi1pdkNzJupkVQunaH7-FuEZhr85JNUgjKFvrA%40mail.gmail.com.
