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

[Chris Plummer]

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
>>