Changes to fix #2648 #2665
Changes to fix #2648 #2665
Conversation
jason-chenyixi-chengdu
requested review from
4ast,
drzaeus77 and
yonghong-song
as code owners
|
[buildbot, test this please] |
|
[buildbot, ok to test] |
src/cc/bcc_elf.c
Outdated
|
|
||
| gelf_getphdr(e, 0, &phdr); | ||
|
|
||
| base_address = phdr.p_vaddr-phdr.p_offset; |
There was a problem hiding this comment.
The base_address is computed based on index 0 which is typically PHDR. Does this calculated base_address is always the same for the loaded segment?
There was a problem hiding this comment.
Do you mean we need to calculate the base address with Load segement rather than any type of segment in PHDR?
I have checked all the segements in PHDR of some elf files, no matter what type of the segment is, base address is always p_vaddr-p_offset
There was a problem hiding this comment.
This is what I mean:
-bash-4.4$ cat main.c
int main() { return 0; }
-bash-4.4$ gcc main.c
-bash-4.4$ readelf -l a.out
Elf file type is EXEC (Executable file)
Entry point 0x4003e0
There are 9 program headers, starting at offset 64
Program Headers:
Type Offset VirtAddr PhysAddr
FileSiz MemSiz Flags Align
PHDR 0x0000000000000040 0x0000000000400040 0x0000000000400040
0x00000000000001f8 0x00000000000001f8 R E 8
INTERP 0x0000000000000238 0x0000000000400238 0x0000000000400238
0x000000000000001c 0x000000000000001c R 1
[Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]
LOAD 0x0000000000000000 0x0000000000400000 0x0000000000400000
0x000000000000069c 0x000000000000069c R E 200000
LOAD 0x0000000000000e10 0x0000000000600e10 0x0000000000600e10
0x0000000000000214 0x0000000000000218 RW 200000
We probably care about the first LOAD segment, right? In this case, the virtual_address - offset is the same. I am just wondering whether this is true in general.
There was a problem hiding this comment.
Actually I am not sure if this is the same in all cases. So how about update the code with below part?
for (i = 0; i < phdr_num; i++) {
if (!gelf_getphdr(e, (int)i, &phdr))
continue;
if ((phdr.p_type == PT_LOAD) && (phdr.p_flags & PF_X)) {
base_address = phdr.p_vaddr-phdr.p_offset;
break;
}
}
| @@ -683,7 +683,7 @@ static int _find_sym(const char *symname, uint64_t addr, uint64_t, | |||
| void *payload) { | |||
| struct bcc_symbol *sym = (struct bcc_symbol *)payload; | |||
| if (!strcmp(sym->name, symname)) { | |||
| sym->offset = addr; | |||
| sym->offset = addr - sym->base_address; | |||
There was a problem hiding this comment.
@palmtenor could you double check? Does this always for non-so binaries?
@palmtenor Glad to know that, thanks! I will update the code and test it. |
|
@yonghong-song @palmtenor the code has been updated, bcc_elf_foreach_load_section is used as an interface to get the load segment and update base address in callback. Please help to review it. |
|
@jason-chenyixi-chengdu there are a few test failures, could you take a look? The change mostly look okay. @palmtenor please take a look as you are more familiar for bcc symbolization than me. In the past, we have the following bug fix for shared library symbol lookup, This also tries to deal with the case where file_offset is different from virtual address. |
|
@yonghong-song For the failed tests, i have no permission to access builbot, could you share with me here? |
|
In the pull request page (#2665), there is a |
|
But as i said, i have no permission to get the details, it was said " jason-chenyixi-chengdu is missing the Overall/Read permission" |
|
@yonghong-song We're all blocked from seeing the build logs since at least August (cf. #2501). |
|
@jason-chenyixi-chengdu @pchaigno I am not aware of the issue. Let me see how we can resolve this. cc @drzaeus77 |
|
The following tests failed for the current pull request, which may be worth to investigate: bashreadline.py also failed on ubuntu 17.04. |
|
@yonghong-song , the two cases failed are because offset address is used as target physical address for executable binaries(eg. python, bash). I have updated the code to get the correct target address. |
|
The following tests failed on all fedora hosts. For example, Some address adjustment might be needed for usdt as well? |
|
[buildbot, ok to test] |
|
[buildbot, test this please] |
|
|
|
It is strange... Could you help to get the dump file? There is no problem in my host |
|
I will take a look today or tomorrow. |
|
@palmtenor could you help check this patch as well? |
|
I did not have fc28 VM any more. I can reproduce the issue with fc31. please try fc31. |
|
I have tried. This fault has been raised too. But this fault is still exsisting even i revert my change. And I found that the fault is raised from bcc_func_load in bpf_module. I have print some messages around. |
|
After investgating on my host, the fault is caused by python initial file is not the latest version, so the interface bcc_func_load is not compatiable. After i update the python initial file, the test case passed. Could you help to check if this is the same reason on the testing host? |
|
@jason-chenyixi-chengdu The following is my setup. In fc31, x64 server iso image and installed as VM, I am using vmware fusion, but virtualbox should be the same, I guess. following the INSTALL.md instruction to install all packages, clone latest bcc and build it. with this patch, the |
|
The test appears passing now. There are conflicts with current master branch. Could you rebase, squash and resubmit? Thanks! |
Update from master
|
updated.pls help to check, thanks! |
|
@jason-chenyixi-chengdu you stilll have 15 commits and it still have conflicts with trunk. Maybe you can close this one and submit a new pull request with a single commit with making a reference to this pull request? |
Hi,
This changes is for fix issue #2648 . Please help to review if it OK.
Main changes are b93e4a3 and e6b9572. Others are some testing in my forked repo.
Thanks!