Re: [osv-dev] Fail to read data from large file
On Tue, Jan 14, 2020 at 9:28 AM Lewis Tian wrote:
> When I run pagerank on Ubuntu, the code works fine. (the graph data is
> stored in web-NotreDame.txt, 21M)
>
21M isn't very large, it shouldn't present any special problems. We
probably have a bug that isn't just about file size:
> taseikyo@ubuntu:~/Desktop/osv/apps/my-pagerank-test$ ls
> Makefile module.py pagerank.c usr.manifest web-NotreDame.txt
>
> taseikyo@ubuntu:~/Desktop/osv/apps/my-pagerank-test$ make
> cc -pie -o pagerank pagerank.c
>
> taseikyo@ubuntu:~/Desktop/osv/apps/my-pagerank-test$ ll -h
> total 21M
> drwxrwxr-x 2 taseikyo taseikyo 4.0K Jan 14 11:59 ./
> drwxrwxr-x 128 taseikyo taseikyo 4.0K Jan 14 11:51 ../
> -rw-rw-r-- 1 taseikyo taseikyo 110 Jan 14 11:51 Makefile
> -rw-rw-r-- 1 taseikyo taseikyo 60 Jan 13 11:26 module.py
> -rwxrwxr-x 1 taseikyo taseikyo 17K Jan 14 11:59 pagerank*
> -rw-rw-r-- 1 taseikyo taseikyo 5.6K Jan 14 11:51 pagerank.c
> -rw-rw-r-- 1 taseikyo taseikyo 85 Jan 14 11:53 usr.manifest
> -rw-rw-r-- 1 taseikyo taseikyo 21M Jan 14 11:54 web-NotreDame.txt
>
> taseikyo@ubuntu:~/Desktop/osv/apps/my-pagerank-test$ ./pagerank
> Graph data:
>
> Nodes: 325729, Edges: 1497134
>
>
> Number of iteration to converge: 52
>
> Final Pagerank values:
> [0.002066 , 0.000181 , ...]
>
> Time spent: 0.896491 seconds.
>
> But when I build and run pagerank on osv, it fails to read the graph data
> (only read part of the graph).
>
> taseikyo@ubuntu:~/Desktop/osv$ ./scripts/build image=my-pagerank-test
> taseikyo@ubuntu:~/Desktop/osv$ ./scripts/run.py
> OSv v0.54.0-71-g69a0ce39
> eth0: 192.168.122.15
> Booted up in 338.86 ms
> Cmdline: /pagerank
>
> Graph data:
>
> Nodes: 325729, Edges: 1497134
>
> Fail to read data...
>
> From: 6 To: 119
>
> Here is part of the code:
>
> while (!feof(fp)) {
> fret = fscanf(fp, "%d%d", , );
> if (fret == 0) {
> printf("Fail to read data...\n");
> printf("\n From: %d To: %d\n",fromnode, tonode);
> return -1;
> }
> ...
> }
>
>
Maybe we have a bug in fscanf or in the stdio reading layer?
Things I'd like you to please check:
1. Print a failure already if fret < 2 - since 1 is also a failure.
2. On failure, please print ftell(fp) - our position in the file (is it the
end? something in the middle?). Please also do a fgets() or a short fgetc()
loop or fread() to read the next available bytes, to try to understand by
fscanf() failed. Is the reading from the file failing, or is the parsing
failing?
> When I use a small graph (4 nodes, 7 edges), it runs normally.
>
> taseikyo@ubuntu:~/Desktop/osv$ ./scripts/build image=my-pagerank-test
> taseikyo@ubuntu:~/Desktop/osv$ ./scripts/run.py
> OSv v0.54.0-71-g69a0ce39
> eth0: 192.168.122.15
> Booted up in 356.44 ms
> Cmdline: /pagerank
>
> Graph data:
>
> Nodes: 4, Edges: 7
>
>
> Number of iteration to converge: 41
>
> Final Pagerank values:
>
> [0.159913 , 0.144016 , 0.144016 , 0.082809 ]
>
> Time spent: 0.693802 seconds.
>
> Is osv unable to read large files (bug?) I'll appreciate your help very
> much! : )
>
> --
> 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.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/osv-dev/50f3a073-e364-484c-803e-570e0dd6530c%40googlegroups.com
>
> .
>
--
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.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/CANEVyjsW9R2vhi-LA276U8Y3-Mp3bG0nZTEOKAKb9gYfnj8t%2BQ%40mail.gmail.com.
[osv-dev] Fail to read data from large file
When I run pagerank on Ubuntu, the code works fine. (the graph data is
stored in web-NotreDame.txt, 21M)
taseikyo@ubuntu:~/Desktop/osv/apps/my-pagerank-test$ ls
Makefile module.py pagerank.c usr.manifest web-NotreDame.txt
taseikyo@ubuntu:~/Desktop/osv/apps/my-pagerank-test$ make
cc -pie -o pagerank pagerank.c
taseikyo@ubuntu:~/Desktop/osv/apps/my-pagerank-test$ ll -h
total 21M
drwxrwxr-x 2 taseikyo taseikyo 4.0K Jan 14 11:59 ./
drwxrwxr-x 128 taseikyo taseikyo 4.0K Jan 14 11:51 ../
-rw-rw-r-- 1 taseikyo taseikyo 110 Jan 14 11:51 Makefile
-rw-rw-r-- 1 taseikyo taseikyo 60 Jan 13 11:26 module.py
-rwxrwxr-x 1 taseikyo taseikyo 17K Jan 14 11:59 pagerank*
-rw-rw-r-- 1 taseikyo taseikyo 5.6K Jan 14 11:51 pagerank.c
-rw-rw-r-- 1 taseikyo taseikyo 85 Jan 14 11:53 usr.manifest
-rw-rw-r-- 1 taseikyo taseikyo 21M Jan 14 11:54 web-NotreDame.txt
taseikyo@ubuntu:~/Desktop/osv/apps/my-pagerank-test$ ./pagerank
Graph data:
Nodes: 325729, Edges: 1497134
Number of iteration to converge: 52
Final Pagerank values:
[0.002066 , 0.000181 , ...]
Time spent: 0.896491 seconds.
But when I build and run pagerank on osv, it fails to read the graph data
(only read part of the graph).
taseikyo@ubuntu:~/Desktop/osv$ ./scripts/build image=my-pagerank-test
taseikyo@ubuntu:~/Desktop/osv$ ./scripts/run.py
OSv v0.54.0-71-g69a0ce39
eth0: 192.168.122.15
Booted up in 338.86 ms
Cmdline: /pagerank
Graph data:
Nodes: 325729, Edges: 1497134
Fail to read data...
From: 6 To: 119
Here is part of the code:
while (!feof(fp)) {
fret = fscanf(fp, "%d%d", , );
if (fret == 0) {
printf("Fail to read data...\n");
printf("\n From: %d To: %d\n",fromnode, tonode);
return -1;
}
...
}
When I use a small graph (4 nodes, 7 edges), it runs normally.
taseikyo@ubuntu:~/Desktop/osv$ ./scripts/build image=my-pagerank-test
taseikyo@ubuntu:~/Desktop/osv$ ./scripts/run.py
OSv v0.54.0-71-g69a0ce39
eth0: 192.168.122.15
Booted up in 356.44 ms
Cmdline: /pagerank
Graph data:
Nodes: 4, Edges: 7
Number of iteration to converge: 41
Final Pagerank values:
[0.159913 , 0.144016 , 0.144016 , 0.082809 ]
Time spent: 0.693802 seconds.
Is osv unable to read large files (bug?) I'll appreciate your help very
much! : )
--
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.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/50f3a073-e364-484c-803e-570e0dd6530c%40googlegroups.com.
[osv-dev] [COMMIT osv master] elf: ignore versioning table if symbol is looked up by object itself
From: Waldemar Kozaczuk
Committer: Waldemar Kozaczuk
Branch: master
elf: ignore versioning table if symbol is looked up by object itself
The commit
https://github.com/cloudius-systems/osv/commit/ed1eed7a567ec17138c65f0a5628c2311603c712
enhanced dynamic linker to skip old symbols per versions table.
Unfortunately
the relevant code did not take into account whether the old symbol is being
looked up by the object itself during relocation.
This sentence from https://www.akkadia.org/drepper/symbol-versioning -
"If the highest bit (bit 15) is set this is a hidden symbol which cannot
be referenced from outside the object." - SEEMS to indicate the old
symbols should be visible to the object itself only.
This patch enhances lookup method to help track "who" is looking
and if it is the object itself, then the versions table is ignored.
Here is some background information:
The main (and so far the only one) motivation for this patch is to allow
upgrading libgcc_s.so to the newest version from host instead of relying
on the "antique" version from externals (see usr.manifest.skel). It turns
out that at some point (starting with version 6) GCC team moved the
__cpu_model
and __cpu_indicator_init (constructor function) symbols from the shared
library
libgcc_s.so to the static one - libgcc.a (please see this commit for some
details -
https://github.com/gcc-mirror/gcc/commit/4b5fb32aba30762e0d8c9e75d1b46b47bee5eeb4#diff-3592c95126806aad11bb0f09e182f2f4)
and marked them as "compat" in libgcc_s.so (shown with single @).
This makes those symbols invisible to OSv linker because per symbols version
table they are effectively "old" and should be ignored during lookup
but not to the object itself when it tries to relocate them.
This patch actually makes the change to the version symbol handling
logic to achieve exactly that.
The version symbol handling logic in OSv is based on musl, but musl does
not try to expose
such "old" symbols to the object itself like this patch does. It turns out
that Musl team complained
about original change to libgcc_s.so and GCC team changed how libgcc_s.so
for musl is built.
This commit -
https://github.com/gcc-mirror/gcc/commit/6e6c7fc1e15525a10f48d4f5ac2edd853e2f5cb7
and this discussion - https://patchwork.ozlabs.org/patch/693782/ - sheds
some light.
Signed-off-by: Waldemar Kozaczuk
---
diff --git a/core/elf.cc b/core/elf.cc
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -186,7 +186,7 @@ void object::set_visibility(elf::VisibilityLevel
visibilityLevel)
template <>
void* object::lookup(const char* symbol)
{
-symbol_module sm{lookup_symbol(symbol), this};
+symbol_module sm{lookup_symbol(symbol, false), this};
if (!sm.symbol || sm.symbol->st_shndx == SHN_UNDEF) {
return nullptr;
}
@@ -680,7 +680,7 @@ symbol_module object::symbol(unsigned idx, bool
ignore_missing)
}
auto nameidx = sym->st_name;
auto name = dynamic_ptr(DT_STRTAB) + nameidx;
-auto ret = _prog.lookup(name);
+auto ret = _prog.lookup(name, this);
if (!ret.symbol && binding == STB_WEAK) {
return symbol_module(sym, this);
}
@@ -708,7 +708,7 @@ symbol_module object::symbol_other(unsigned idx)
for (auto module : ml.objects) {
if (module == this)
continue; // do not match this module
-if (auto sym = module->lookup_symbol(name)) {
+if (auto sym = module->lookup_symbol(name, false)) {
ret = symbol_module(sym, module);
break;
}
@@ -885,7 +885,7 @@ dl_new_hash(const char *s)
return h & 0x;
}
-Elf64_Sym* object::lookup_symbol_gnu(const char* name)
+Elf64_Sym* object::lookup_symbol_gnu(const char* name, bool self_lookup)
{
auto symtab = dynamic_ptr(DT_SYMTAB);
auto strtab = dynamic_ptr(DT_STRTAB);
@@ -909,7 +909,7 @@ Elf64_Sym* object::lookup_symbol_gnu(const char* name)
if (idx == 0) {
return nullptr;
}
-auto version_symtab = dynamic_exists(DT_VERSYM) ?
dynamic_ptr(DT_VERSYM) : nullptr;
+auto version_symtab = (!self_lookup && dynamic_exists(DT_VERSYM)) ?
dynamic_ptr(DT_VERSYM) : nullptr;
do {
if ((chains[idx] & ~1) != (hashval & ~1)) {
continue;
@@ -924,14 +924,14 @@ Elf64_Sym* object::lookup_symbol_gnu(const char* name)
return nullptr;
}
-Elf64_Sym* object::lookup_symbol(const char* name)
+Elf64_Sym* object::lookup_symbol(const char* name, bool self_lookup)
{
if (!visible()) {
return nullptr;
}
Elf64_Sym* sym;
if (dynamic_exists(DT_GNU_HASH)) {
-sym = lookup_symbol_gnu(name);
+sym = lookup_symbol_gnu(name, self_lookup);
} else {
sym = lookup_symbol_old(name);
}
@@ -1491,14 +1491,14 @@ void program::del_debugger_obj(object* obj)
}
}
-symbol_module program::lookup(const char* name)
+symbol_module program::lookup(const char* name, object* seeker)
{
Re: [osv-dev] Re: [PATCH 3/3] Implement RTLD_NEXT
On Mon, Jan 13, 2020 at 3:48 AM Nadav Har'El 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
> wrote:
>
>> This version only contains one iteration over the module list.
>>
>> On Sun, Jan 12, 2020 at 8:39 PM Zhiting Zhu
>> wrote:
>>
>>> Signed-off-by: Zhiting Zhu
>>> ---
>>> 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 )
>>> +{
>>> +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 path);
>>>
>>> symbol_module lookup(const char* symbol);
>>> +symbol_module lookup_next(const char* name, const void* retaddr);
>>> template
>>> 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*>(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
>>> 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 osv-dev+unsubscr...@googlegroups.com.
>> To view this discussion on the web visit
>>
[osv-dev] [PATCH 3/3] Implement RTLD_NEXT
Signed-off-by: Zhiting Zhu
---
core/elf.cc| 33 +
include/osv/elf.hh | 1 +
libc/dlfcn.cc | 4 ++--
tests/tst-dlfcn.cc | 2 +-
4 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/core/elf.cc b/core/elf.cc
index 882b4d49..badbb7b0 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,38 @@ 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 )
+{
+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;
+}
+}
+if (start == ml.objects.begin()) {
+return;
+}
+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 path);
symbol_module lookup(const char* symbol);
+symbol_module lookup_next(const char* name, const void* retaddr);
template
T* lookup_function(const char* symbol);
diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc
index 1cad7ffd..3d228bbf 100644
--- a/libc/dlfcn.cc
+++ b/libc/dlfcn.cc
@@ -70,8 +70,8 @@ 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));
+sym = program->lookup_next(name, retaddr);
} else {
auto obj = *reinterpret_cast*>(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
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 osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/20200113181034.11368-1-zhitingz%40cs.utexas.edu.
Re: [osv-dev] [PATCH 1/3] Add dlsym tests
I just wanted to say that it was me that got Zhiting on the road of creating the separate module. I just wanted to avoid the licensing issues and incorporating bionic code as is. But if there is a way to avoid the git clone, dl_tests altogether and no issues with licensing the better. Btw I will create new issue to track all the problems Zhiting and I identified in dlopen/dlsym function. Waldek On Mon, Jan 13, 2020 at 04:59 Nadav Har'El wrote: > Hi, thank. Please see my answers and requests inline. > > On Mon, Jan 13, 2020 at 4:39 AM zhiting zhu > wrote: > >> >> >> On Sun, Jan 12, 2020 at 4:02 PM Nadav Har'El wrote: >> >>> Thanks for the answers. I have some followup questions below. >>> >>> On Sun, Jan 12, 2020 at 10:36 PM zhiting zhu >>> wrote: >>> You can look at this thread: https://groups.google.com/forum/#!topic/osv-dev/FLsWldGFUiY The tests are coming from bionic which is Android's libc. They have their own implementation of libdl with lots of tests for each functions. The tests that are added to tst-dlfcn.cc are derived from the bionic tests. >>> >>> So, I thought the stuff in dl_tests comes from Bionic, but you're >>> staying that the code in tst-tlfcn.cc was also copied from Bionic? >>> Is it an actual copy, or just "inspired" by their test and written by >>> you from scratch? >>> >>> If it's a copy, I have to ask: >>> >>> 1. What license does Bionic have? We will need to add Bionic's copyright >>> statement. Better create a separate test file just for the stuff you copied >>> from Bionic, so the entire source file has one copyright statement. No need >>> to stuff this code - if it's copied - into the existing tst-dlfcn.cc file. >>> We can create a new source file. >>> 2. If you copied test code, why do we need to "git clone" the Bionic >>> project for the dl_tests module? What does this module do? (oh, I see you >>> answered this below so I'll continue below). >>> >>> In the future, please try to make these issues understandable by using >>> commit messages and/or internal documentation (e.g., a file >>> modules/dl_tests/README would have been useful). >>> As I said, I missed the original discussion, but somebody may wonder >>> about these things two years from now (e.g., when a build suddenly breaks >>> because "dl_tests" no longer compiles), and would also like to be able to >>> figure out what these things do. >>> >> I copy their code and change them from gtest to boost. >> > > If you really did copy code and just applied some straightforward > translation (not a complete rewrite just "inspired" by their tests), you > need to include their copyright statement - and license - in the test file. > > > >> The reason I need to git clone them is they contain the test library. >> > > What is "the test library"? Is it just a trivial creation of a ".so" with > some function which is used by the test? > If so, I would very strongly prefer that we just create this trivial .so > in modules/tests/Makefile - and not use a separate modules/dl_tests and > definitely not clone an entire libc project! > > But if I completely missed what "the test library" is, please explain it. > > >> I can add README to that directory with explanations. The license of >> bionic is Apache 2.0. >> > > At this point I'm starting to think the modules/dl_tests/ directory > shouldn't even exist - not that it needs a README. > Feel free to convince me otherwise :-) > > >>> >>> I have run these tests on Linux. They can be run without any error. But there're some errors when running in osv. Some of the failed checks are commented out with TODO comments which is due to incorrect implementation of dlopen. >>> Here's the command if you want to build them and run on linux: g++ -std=gnu++11 -Wall -o tst-dlfcn tst-dlfcn.cc -rdynamic -ldl -Wl,-Bstatic -lboost_unit_test_framework -Wl,-Bdynamic g++ -std=gnu++11 -Wall -o libtest_simple.so -shared -fpic dlopen_testlib_simple.cpp g++ -std=gnu++11 -Wall -o libtest_empty.so -shared -fpic empty.cpp g++ -std=gnu++11 -Wall -o libtest_check_rtld_next_from_library.so -shared -fpic check_rtld_next_from_library.cpp g++ -std=gnu++11 -Wall -o libtest_dlsym_from_this_grandchild.so -shared -fPIC dlsym_from_this_symbol2.cpp g++ -std=gnu++11 -Wall -o libtest_dlsym_from_this_child.so -shared -fPIC dlsym_from_this_functions.cpp -Wl,--no-as-needed -ltest_dlsym_from_this_grandchild -L. -Wl,-rpath=. g++ -std=gnu++11 -Wall -o libtest_dlsym_from_this.so -shared -fPIC dlsym_from_this_symbol.cpp -Wl,--no-as-needed -ltest_dlsym_from_this_child -L. -Wl,-rpath=. g++ -std=gnu++11 -Wall -o libdlext_test.so -shared -fPIC dlext_test_library.cpp -Wl,-z,relro -Wl,--no-as-needed -ltest_simple -L. -Wl,-rpath=. g++ -std=gnu++11 -Wall -o libtest_with_dependency.so -shared -fPIC dlopen_testlib_simple.cpp -Wl,--no-as-needed -ldlext_test -L. -Wl,-rpath=. sudo
Re: [osv-dev] [PATCH V3] elf: ignore versioning table if symbol is looked up by object itself
I just wanted to say in general that I am not super happy we had to come up
with this patch. But it blocks us from getting rid of externals.
On Mon, Jan 13, 2020 at 06:24 Nadav Har'El wrote:
> On Mon, Jan 13, 2020 at 7:14 AM Waldemar Kozaczuk
> wrote:
>
>> The commit
>> https://github.com/cloudius-systems/osv/commit/ed1eed7a567ec17138c65f0a5628c2311603c712
>> enhanced dynamic linker to skip old symbols per versions table.
>> Unfortunately
>> the relevant code did not take into account whether the old symbol is
>> being
>> looked up by the object itself during relocation.
>>
>> This sentence from https://www.akkadia.org/drepper/symbol-versioning -
>> "If the highest bit (bit 15) is set this is a hidden symbol which cannot
>> be referenced from outside the object." - SEEMS to indicate the old
>> symbols should be visible to the object itself only.
>>
>> This patch enhances lookup method to help track "who" is looking
>> and if it is the object itself, then the versions table is ignored.
>>
>> Here is some background information:
>>
>> The main (and so far the only one) motivation for this patch is to allow
>> upgrading libgcc_s.so to the newest version from host instead of relying
>> on the "antique" version from externals (see usr.manifest.skel). It turns
>> out that at some point (starting with version 6) GCC team moved the
>> __cpu_model
>> and __cpu_indicator_init (constructor function) symbols from the shared
>> library
>> libgcc_s.so to the static one - libgcc.a (please see this commit for some
>> details -
>>
>> https://github.com/gcc-mirror/gcc/commit/4b5fb32aba30762e0d8c9e75d1b46b47bee5eeb4#diff-3592c95126806aad11bb0f09e182f2f4
>> )
>> and marked them as "compat" in libgcc_s.so (shown with single @).
>>
>> This makes those symbols invisible to OSv linker because per symbols
>> version
>> table they are effectively "old" and should be ignored during lookup
>> but not to the object itself when it tries to relocate them.
>> This patch actually makes the change to the version symbol handling
>> logic to achieve exactly that.
>>
>> The version symbol handling logic in OSv is based on musl, but musl does
>> not try to expose
>> such "old" symbols to the object itself like this patch does. It turns
>> out that Musl team complained
>> about original change to libgcc_s.so and GCC team changed how libgcc_s.so
>> for musl is built.
>> This commit -
>> https://github.com/gcc-mirror/gcc/commit/6e6c7fc1e15525a10f48d4f5ac2edd853e2f5cb7
>> and this discussion - https://patchwork.ozlabs.org/patch/693782/ - sheds
>> some light.
>>
>
> Unfortunately I have to admit that I didn't understand any of the details
> on these commits and discussions :-(
>
I am not surprised. I barely could follow them.
> It seems glibc deliberately wanted to hide these symbols. Can you please
> remind me what application or user uses them?
> Or does the C compiler generate their uses?
>
I honestly do not know. I think gnu FORTRAN might be using it.
>
> Other than this, I don't have any specific objections or comments about
> this patch.
> One thing I'm not sure about is your call in object::symbol() to
> _prog.lookup(name, this).
> This supposedly means object::symbol() is only used "from" the object
> itself. Is that the case?
>
Yeah the object::symbol() is only called by object relocation methods which
fail without this patch because symbol is not found.
I theory dlsym() could also be called by object itself but for now I am
deliberately ignoring this potential case.
>
>
>
>> Signed-off-by: Waldemar Kozaczuk
>> ---
>> core/elf.cc| 20 ++--
>> include/osv/elf.hh | 6 +++---
>> libc/dlfcn.cc | 4 ++--
>> 3 files changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/core/elf.cc b/core/elf.cc
>> index 24dfe914..fa63c9aa 100644
>> --- a/core/elf.cc
>> +++ b/core/elf.cc
>> @@ -154,7 +154,7 @@ void object::setprivate(bool priv)
>> template <>
>> void* object::lookup(const char* symbol)
>> {
>> -symbol_module sm{lookup_symbol(symbol), this};
>> +symbol_module sm{lookup_symbol(symbol, false), this};
>> if (!sm.symbol || sm.symbol->st_shndx == SHN_UNDEF) {
>> return nullptr;
>> }
>> @@ -648,7 +648,7 @@ symbol_module object::symbol(unsigned idx, bool
>> ignore_missing)
>> }
>> auto nameidx = sym->st_name;
>> auto name = dynamic_ptr(DT_STRTAB) + nameidx;
>> -auto ret = _prog.lookup(name);
>> +auto ret = _prog.lookup(name, this);
>> if (!ret.symbol && binding == STB_WEAK) {
>> return symbol_module(sym, this);
>> }
>> @@ -676,7 +676,7 @@ symbol_module object::symbol_other(unsigned idx)
>> for (auto module : ml.objects) {
>> if (module == this)
>> continue; // do not match this module
>> -if (auto sym = module->lookup_symbol(name)) {
>> +if (auto sym = module->lookup_symbol(name, false)) {
>> ret = symbol_module(sym, module);
>> break;
>>
Re: [osv-dev] [PATCH V3] elf: ignore versioning table if symbol is looked up by object itself
On Mon, Jan 13, 2020 at 7:14 AM Waldemar Kozaczuk
wrote:
> The commit
> https://github.com/cloudius-systems/osv/commit/ed1eed7a567ec17138c65f0a5628c2311603c712
> enhanced dynamic linker to skip old symbols per versions table.
> Unfortunately
> the relevant code did not take into account whether the old symbol is being
> looked up by the object itself during relocation.
>
> This sentence from https://www.akkadia.org/drepper/symbol-versioning -
> "If the highest bit (bit 15) is set this is a hidden symbol which cannot
> be referenced from outside the object." - SEEMS to indicate the old
> symbols should be visible to the object itself only.
>
> This patch enhances lookup method to help track "who" is looking
> and if it is the object itself, then the versions table is ignored.
>
> Here is some background information:
>
> The main (and so far the only one) motivation for this patch is to allow
> upgrading libgcc_s.so to the newest version from host instead of relying
> on the "antique" version from externals (see usr.manifest.skel). It turns
> out that at some point (starting with version 6) GCC team moved the
> __cpu_model
> and __cpu_indicator_init (constructor function) symbols from the shared
> library
> libgcc_s.so to the static one - libgcc.a (please see this commit for some
> details -
>
> https://github.com/gcc-mirror/gcc/commit/4b5fb32aba30762e0d8c9e75d1b46b47bee5eeb4#diff-3592c95126806aad11bb0f09e182f2f4
> )
> and marked them as "compat" in libgcc_s.so (shown with single @).
>
> This makes those symbols invisible to OSv linker because per symbols
> version
> table they are effectively "old" and should be ignored during lookup
> but not to the object itself when it tries to relocate them.
> This patch actually makes the change to the version symbol handling
> logic to achieve exactly that.
>
> The version symbol handling logic in OSv is based on musl, but musl does
> not try to expose
> such "old" symbols to the object itself like this patch does. It turns out
> that Musl team complained
> about original change to libgcc_s.so and GCC team changed how libgcc_s.so
> for musl is built.
> This commit -
> https://github.com/gcc-mirror/gcc/commit/6e6c7fc1e15525a10f48d4f5ac2edd853e2f5cb7
> and this discussion - https://patchwork.ozlabs.org/patch/693782/ - sheds
> some light.
>
Unfortunately I have to admit that I didn't understand any of the details
on these commits and discussions :-(
It seems glibc deliberately wanted to hide these symbols. Can you please
remind me what application or user uses them?
Or does the C compiler generate their uses?
Other than this, I don't have any specific objections or comments about
this patch.
One thing I'm not sure about is your call in object::symbol() to
_prog.lookup(name, this).
This supposedly means object::symbol() is only used "from" the object
itself. Is that the case?
> Signed-off-by: Waldemar Kozaczuk
> ---
> core/elf.cc| 20 ++--
> include/osv/elf.hh | 6 +++---
> libc/dlfcn.cc | 4 ++--
> 3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/core/elf.cc b/core/elf.cc
> index 24dfe914..fa63c9aa 100644
> --- a/core/elf.cc
> +++ b/core/elf.cc
> @@ -154,7 +154,7 @@ void object::setprivate(bool priv)
> template <>
> void* object::lookup(const char* symbol)
> {
> -symbol_module sm{lookup_symbol(symbol), this};
> +symbol_module sm{lookup_symbol(symbol, false), this};
> if (!sm.symbol || sm.symbol->st_shndx == SHN_UNDEF) {
> return nullptr;
> }
> @@ -648,7 +648,7 @@ symbol_module object::symbol(unsigned idx, bool
> ignore_missing)
> }
> auto nameidx = sym->st_name;
> auto name = dynamic_ptr(DT_STRTAB) + nameidx;
> -auto ret = _prog.lookup(name);
> +auto ret = _prog.lookup(name, this);
> if (!ret.symbol && binding == STB_WEAK) {
> return symbol_module(sym, this);
> }
> @@ -676,7 +676,7 @@ symbol_module object::symbol_other(unsigned idx)
> for (auto module : ml.objects) {
> if (module == this)
> continue; // do not match this module
> -if (auto sym = module->lookup_symbol(name)) {
> +if (auto sym = module->lookup_symbol(name, false)) {
> ret = symbol_module(sym, module);
> break;
> }
> @@ -852,7 +852,7 @@ dl_new_hash(const char *s)
> return h & 0x;
> }
>
> -Elf64_Sym* object::lookup_symbol_gnu(const char* name)
> +Elf64_Sym* object::lookup_symbol_gnu(const char* name, bool self_lookup)
> {
> auto symtab = dynamic_ptr(DT_SYMTAB);
> auto strtab = dynamic_ptr(DT_STRTAB);
> @@ -876,7 +876,7 @@ Elf64_Sym* object::lookup_symbol_gnu(const char* name)
> if (idx == 0) {
> return nullptr;
> }
> -auto version_symtab = dynamic_exists(DT_VERSYM) ?
> dynamic_ptr(DT_VERSYM) : nullptr;
> +auto version_symtab = (!self_lookup && dynamic_exists(DT_VERSYM)) ?
> dynamic_ptr(DT_VERSYM) : nullptr;
> do {
>
Re: [osv-dev] [PATCH 1/3] Add dlsym tests
Hi, thank. Please see my answers and requests inline. On Mon, Jan 13, 2020 at 4:39 AM zhiting zhu wrote: > > > On Sun, Jan 12, 2020 at 4:02 PM Nadav Har'El wrote: > >> Thanks for the answers. I have some followup questions below. >> >> On Sun, Jan 12, 2020 at 10:36 PM zhiting zhu >> wrote: >> >>> You can look at this thread: >>> https://groups.google.com/forum/#!topic/osv-dev/FLsWldGFUiY >>> >>> The tests are coming from bionic which is Android's libc. They have >>> their own implementation of libdl with lots of tests for each functions. >>> The tests that are added to tst-dlfcn.cc are derived from the bionic tests. >>> >> >> So, I thought the stuff in dl_tests comes from Bionic, but you're staying >> that the code in tst-tlfcn.cc was also copied from Bionic? >> Is it an actual copy, or just "inspired" by their test and written by you >> from scratch? >> >> If it's a copy, I have to ask: >> >> 1. What license does Bionic have? We will need to add Bionic's copyright >> statement. Better create a separate test file just for the stuff you copied >> from Bionic, so the entire source file has one copyright statement. No need >> to stuff this code - if it's copied - into the existing tst-dlfcn.cc file. >> We can create a new source file. >> 2. If you copied test code, why do we need to "git clone" the Bionic >> project for the dl_tests module? What does this module do? (oh, I see you >> answered this below so I'll continue below). >> >> In the future, please try to make these issues understandable by using >> commit messages and/or internal documentation (e.g., a file >> modules/dl_tests/README would have been useful). >> As I said, I missed the original discussion, but somebody may wonder >> about these things two years from now (e.g., when a build suddenly breaks >> because "dl_tests" no longer compiles), and would also like to be able to >> figure out what these things do. >> > I copy their code and change them from gtest to boost. > If you really did copy code and just applied some straightforward translation (not a complete rewrite just "inspired" by their tests), you need to include their copyright statement - and license - in the test file. > The reason I need to git clone them is they contain the test library. > What is "the test library"? Is it just a trivial creation of a ".so" with some function which is used by the test? If so, I would very strongly prefer that we just create this trivial .so in modules/tests/Makefile - and not use a separate modules/dl_tests and definitely not clone an entire libc project! But if I completely missed what "the test library" is, please explain it. > I can add README to that directory with explanations. The license of > bionic is Apache 2.0. > At this point I'm starting to think the modules/dl_tests/ directory shouldn't even exist - not that it needs a README. Feel free to convince me otherwise :-) >> >> >>> I have run these tests on Linux. They can be run without any error. But >>> there're some errors when running in osv. Some of the failed checks are >>> commented out with TODO comments which is due to incorrect implementation >>> of dlopen. >>> >> >>> Here's the command if you want to build them and run on linux: >>> g++ -std=gnu++11 -Wall -o tst-dlfcn tst-dlfcn.cc -rdynamic -ldl >>> -Wl,-Bstatic -lboost_unit_test_framework -Wl,-Bdynamic >>> g++ -std=gnu++11 -Wall -o libtest_simple.so -shared -fpic >>> dlopen_testlib_simple.cpp >>> g++ -std=gnu++11 -Wall -o libtest_empty.so -shared -fpic empty.cpp >>> g++ -std=gnu++11 -Wall -o libtest_check_rtld_next_from_library.so >>> -shared -fpic check_rtld_next_from_library.cpp >>> g++ -std=gnu++11 -Wall -o libtest_dlsym_from_this_grandchild.so -shared >>> -fPIC dlsym_from_this_symbol2.cpp >>> g++ -std=gnu++11 -Wall -o libtest_dlsym_from_this_child.so -shared -fPIC >>> dlsym_from_this_functions.cpp -Wl,--no-as-needed >>> -ltest_dlsym_from_this_grandchild -L. -Wl,-rpath=. >>> g++ -std=gnu++11 -Wall -o libtest_dlsym_from_this.so -shared -fPIC >>> dlsym_from_this_symbol.cpp -Wl,--no-as-needed -ltest_dlsym_from_this_child >>> -L. -Wl,-rpath=. >>> g++ -std=gnu++11 -Wall -o libdlext_test.so -shared -fPIC >>> dlext_test_library.cpp -Wl,-z,relro -Wl,--no-as-needed -ltest_simple -L. >>> -Wl,-rpath=. >>> g++ -std=gnu++11 -Wall -o libtest_with_dependency.so -shared -fPIC >>> dlopen_testlib_simple.cpp -Wl,--no-as-needed -ldlext_test -L. -Wl,-rpath=. >>> sudo mkdir -p /tests >>> sudo cp *.so /tests >>> >> So, I don't see anything here using modules/dl_tests. If we can run this test in Linux without modules/dl_tests, isn't the same thing possible in OSv? >>> Some of the comments in the code are coming from bionic. Those links >>> should be removed. >>> >> Please do. >>> dl_tests contains the library that's needed by tst-dlfcn.cc. Those >>> library are built to help testing dlsym and dlopen. There're no tests in >>> dl_tests. The reason for cloning the bionic repo is to avoid license >>> issues. In my original patch, I
Re: [osv-dev] Re: [PATCH 3/3] Implement RTLD_NEXT
Please see comments below. You forgot to change one part of the patch.
On Mon, Jan 13, 2020 at 4:40 AM zhiting zhu wrote:
> This version only contains one iteration over the module list.
>
> On Sun, Jan 12, 2020 at 8:39 PM Zhiting Zhu
> wrote:
>
>> Signed-off-by: Zhiting Zhu
>> ---
>> 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 )
>> +{
>> +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).
> +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 path);
>>
>> symbol_module lookup(const char* symbol);
>> +symbol_module lookup_next(const char* name, const void* retaddr);
>> template
>> 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*>(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
>> 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 osv-dev+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/osv-dev/CA%2B3q14z2HB4%3DVRm5Z%2B01PfgjnkGs7ecHkwwFKwus1kjCSvjH8w%40mail.gmail.com
>
> .
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from
