On Mon, Jan 13, 2020 at 3:48 AM Nadav Har'El <[email protected]> wrote:
> Please see comments below. You forgot to change one part of the patch. > > On Mon, Jan 13, 2020 at 4:40 AM zhiting zhu <[email protected]> > wrote: > >> This version only contains one iteration over the module list. >> >> On Sun, Jan 12, 2020 at 8:39 PM Zhiting Zhu <[email protected]> >> wrote: >> >>> Signed-off-by: Zhiting Zhu <[email protected]> >>> --- >>> core/elf.cc | 31 +++++++++++++++++++++++++++++++ >>> include/osv/elf.hh | 1 + >>> libc/dlfcn.cc | 5 +++-- >>> tests/tst-dlfcn.cc | 2 +- >>> 4 files changed, 36 insertions(+), 3 deletions(-) >>> >>> diff --git a/core/elf.cc b/core/elf.cc >>> index 882b4d49..eaf9d0c8 100644 >>> --- a/core/elf.cc >>> +++ b/core/elf.cc >>> @@ -41,6 +41,7 @@ >>> TRACEPOINT(trace_elf_load, "%s", const char *); >>> TRACEPOINT(trace_elf_unload, "%s", const char *); >>> TRACEPOINT(trace_elf_lookup, "%s", const char *); >>> +TRACEPOINT(trace_elf_lookup_next, "%s", const char *); >>> TRACEPOINT(trace_elf_lookup_addr, "%p", const void *); >>> >>> extern void* elf_start; >>> @@ -1496,6 +1497,36 @@ symbol_module program::lookup(const char* name) >>> return ret; >>> } >>> >>> +symbol_module program::lookup_next(const char* name, const void* >>> retaddr) >>> +{ >>> + trace_elf_lookup_next(name); >>> + symbol_module ret(nullptr,nullptr); >>> + if (retaddr == nullptr) { >>> + return ret; >>> + } >>> + with_modules([&](const elf::program::modules_list &ml) >>> + { >>> + auto start = ml.objects.begin(); >>> >> + for (auto it = ml.objects.begin(), end = ml.objects.end(); it != >>> end; ++it) { >>> + auto module = *it; >>> + if (module->contains_addr(retaddr)) { >>> + start = it; >>> + break; >>> + } >>> + } >>> + assert(start != ml.objects.end()); >>> >> > Here start cannot be ml.objects.end()... > Maybe you meant to check that it != end? > Or maybe you meant to check that start != begin - since this is what you > initialized start to? > (if you don't check start != begin, you also don't need to initialize > start at all, by the way). > I meat to check start != end after ++start but that seems to not necessary. > > >> + start = ++start; >>> + for (auto it = start, end = ml.objects.end(); it != end; ++it) { >>> + auto module = *it; >>> + if (auto sym = module->lookup_symbol(name)) { >>> + ret = symbol_module(sym, module); >>> + break; >>> + } >>> + } >>> + }); >>> + return ret; >>> +} >>> + >>> void* program::do_lookup_function(const char* name) >>> { >>> auto sym = lookup(name); >>> diff --git a/include/osv/elf.hh b/include/osv/elf.hh >>> index ededb75a..247d9c58 100644 >>> --- a/include/osv/elf.hh >>> +++ b/include/osv/elf.hh >>> @@ -576,6 +576,7 @@ public: >>> void set_search_path(std::initializer_list<std::string> path); >>> >>> symbol_module lookup(const char* symbol); >>> + symbol_module lookup_next(const char* name, const void* retaddr); >>> template <typename T> >>> T* lookup_function(const char* symbol); >>> >>> diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc >>> index 1cad7ffd..91bdfba1 100644 >>> --- a/libc/dlfcn.cc >>> +++ b/libc/dlfcn.cc >>> @@ -70,8 +70,9 @@ void* dlsym(void* handle, const char* name) >>> if ((program == handle) || (handle == RTLD_DEFAULT)) { >>> sym = program->lookup(name); >>> } else if (handle == RTLD_NEXT) { >>> - // FIXME: implement >>> - abort(); >>> + auto retaddr = >>> __builtin_extract_return_addr(__builtin_return_address(0)); >>> + auto call_obj = program->object_containing_addr(retaddr); >>> >> + sym = program->lookup_next(name, call_obj); >>> >> > You forgot to fix this part... > The new look_next() implementation should take retaddr directly. You don't > need to call program->object_containing_addr(retaddr) at all. > > Unless I'm missing something, this version of the patch should not have > even worked. If the tests still did pass, maybe the test isn't good enough? > > >> } else { >>> auto obj = >>> *reinterpret_cast<std::shared_ptr<elf::object>*>(handle); >>> sym = obj->lookup_symbol_deep(name); >>> diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc >>> index 6d91fb93..6188236f 100644 >>> --- a/tests/tst-dlfcn.cc >>> +++ b/tests/tst-dlfcn.cc >>> @@ -12,7 +12,7 @@ >>> #include <boost/test/unit_test.hpp> >>> namespace utf = boost::unit_test; >>> >>> -const bool rtld_next = false; >>> +const bool rtld_next = true; >>> const bool deep_lookup = true; >>> >>> static bool called = false; >>> -- >>> 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/CA%2B3q14z2HB4%3DVRm5Z%2B01PfgjnkGs7ecHkwwFKwus1kjCSvjH8w%40mail.gmail.com >> <https://groups.google.com/d/msgid/osv-dev/CA%2B3q14z2HB4%3DVRm5Z%2B01PfgjnkGs7ecHkwwFKwus1kjCSvjH8w%40mail.gmail.com?utm_medium=email&utm_source=footer> >> . >> > -- > 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/CANEVyjuFJRtDgNANMmS29gARrcvNJBku6d57JDy%2BfuD8iWrQXw%40mail.gmail.com > <https://groups.google.com/d/msgid/osv-dev/CANEVyjuFJRtDgNANMmS29gARrcvNJBku6d57JDy%2BfuD8iWrQXw%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > -- 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/CA%2B3q14ymF1xpH9ZUBSOyJ-bYkcNA6jLbT6QTbWSyEm6Hy--bLA%40mail.gmail.com.
