Sure, I reviewed for this workaround (JDK-8248878). I'm still think we can use note section in the core for JDK-8248876 as I said, but I'm not sure.
Thanks, Yasumasa On 2020/07/14 3:12, Chris Plummer wrote:
Hi Yasumasa, If you have no further suggestions on how to fix JDK-8248876, I'd like to proceed with this work around. Can I considered it reviewed by you? thanks, Chris On 7/7/20 7:29 PM, Chris Plummer wrote:Hi Yasumasa, The executable is not opened with pathmap_open: if ((ph->core->exec_fd = open(exec_file, O_RDONLY)) < 0) { I think pathmap_open() is just used for libraries. thanks, Chris On 7/7/20 6:18 PM, Yasumasa Suenaga wrote:Hi Chris, SA would use `link_map` to decide to load address, but it does not seem to contain executable. I set breakpoint to pathmap_open() and I watched the argument of it, then I didn't see any executable (`java`) on it. Maybe current implementation is broken. I guess we can use note section in the core for deciding loading address. I can see valid address (includes executable) from `readelf -n`. Of course it might be big change for SA... Thanks, Yasumasa On 2020/07/07 15:38, Chris Plummer wrote:Hi Yasumasa, Thanks for the review. I tried the following for line 188: if ((phdr->p_type == PT_LOAD || phdr->p_type == PT_INTERP) && phdr->p_vaddr < baseaddr) { However, "base" still ended up being 0. I added some printfs. For the exec file there is both a PT_INTER with p_vaddr of 0x238 and a PT_LOAD with p_vaddr 0. I'm not sure which to use, but in either case that won't be the proper base when added to 0: if (add_lib_info_fd(ph, exec_file, ph->core->exec_fd, (uintptr_t)0 + find_base_address(ph->core->exec_fd, &exec_ehdr)) == NULL) { goto err; } So maybe it's the (uintptr_t)0 that is the problem here. For shared libs instead of 0 it computes the value to add: if (lib_base_diff == ZERO_LOAD_ADDRESS ) { lib_base_diff = calc_prelinked_load_address(ph, lib_fd, &elf_ehdr, link_map_addr); if (lib_base_diff == INVALID_LOAD_ADDRESS) { close(lib_fd); return false; } } lib_base = lib_base_diff + find_base_address(lib_fd, &elf_ehdr); So in this case we've actually computed lib_base_diff rather than just assumed 0. Chris On 7/6/20 10:46 PM, Yasumasa Suenaga wrote:Hi Chris, Your change looks good. BTW I saw JDK-8248876. I'm not sure, but I guess we can fix this issue if we allow PT_INTERP in L118: ``` 105 uintptr_t find_base_address(int fd, ELF_EHDR* ehdr) { : 115 // the base address of a shared object is the lowest vaddr of 116 // its loadable segments (PT_LOAD) 117 for (phdr = phbuf, cnt = 0; cnt < ehdr->e_phnum; cnt++, phdr++) { 118 if (phdr->p_type == PT_LOAD && phdr->p_vaddr < baseaddr) { 119 baseaddr = phdr->p_vaddr; 120 } 121 } ``` /proc/<PID>/maps shows top of `java` is 0x56543b9df000: 56543b9df000-56543b9e0000 r--p 00000000 08:10 55770 /usr/lib/jvm/java-11-openjdk-amd64/bin/java `i target` on GDB shows 0x56543b9df000 is .interp section: Local exec file: `/usr/lib/jvm/java-11-openjdk-amd64/bin/java', file type elf64-x86-64. Entry point: 0x56543b9e0330 0x000056543b9df318 - 0x000056543b9df334 is .interp Thanks, Yasumasa On 2020/07/07 13:18, Chris Plummer wrote:Hello, Please help review the following: http://cr.openjdk.java.net/~cjplummer/8248878/webrev.00/index.html https://bugs.openjdk.java.net/browse/JDK-8248878 The explanation of the fix is in the CR. The parent CR, JDK-8248876 [1], explains the issue being addressed. There's no test for this fix yet. It requires the changes I'm making for JDK-8247514 [2], which include changes to "findpc" support and the ClhsdbFindPC.java test that trigger this issue. [1] https://bugs.openjdk.java.net/browse/JDK-8248876 [2] https://bugs.openjdk.java.net/browse/JDK-8247514 thanks, Chris
