RFR(XS): 8248878: SA: Implement simple workaround for JDK-8248876

[serguei.spitsyn at oracle.com]

Hi Chris,

This workaround looks reasonable to me.

Thanks,
Serguei


On 7/14/20 12:04, Chris Plummer wrote:
> Hello,
>
> Can I get a second reviewer please. Yasumasa has done the first 
> review. The discussion below was all about how to possibly fix 
> JDK-8248876, but for now we're sticking with this workaround.
>
> 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
>
> On 7/13/20 4:48 PM, Yasumasa Suenaga wrote:
>> 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
>>>>>>
>>>>
>>>
>>>
>
>