RFR: 8261710: SA DSO objects have sizes that are too large [v4]

[Chris Plummer]

On Thu, 18 Feb 2021 02:58:55 GMT, Yasumasa Suenaga <ysuenaga at openjdk.org> wrote:

>> Hi Yasumasa,
>> 
>> Just some comments.
>> 
>> src/jdk.hotspot.agent/linux/native/libsaproc/libproc_impl.c
>> +static inline uintptr_t align_down(uintptr_t ptr, size_t size) {
>> +  return (ptr & ~(size - 1));
>> +}
>> +
>> +static inline uintptr_t align_up(uintptr_t ptr, size_t size) {
>> +  return ((ptr + size - 1) & ~(size - 1));
>> +}
>> 
>> The name of 'size' parameter is confusing. Should it be renamed to something like page_size of psize?
>> 
>> +  lib->end = (uintptr_t)-1L;
>> . . .
>> +      if ((lib->end == (uintptr_t)-1L) || (lib->end < aligned_end)) {
>> +        lib->end = aligned_end;
>>        }
>>  
>> The condition  `(lib->end == (uintptr_t)-1L)` is a subset of `(lib->end < aligned_end)`, and so, can be removed. The same is true for the condition:
>> +        if ((lib->exec_end == (uintptr_t)-1L) || (lib->exec_end < aligned_end)) {
>> +          lib->exec_end = aligned_end;
>> +        }
>> 
>> `+      print_debug("%s [%d] 0x%lx-0x%lx: base = 0x%lx, vaddr = 0x%lx, memsz = 0x%lx, filesz = 0x%lx\n", lib->name, cnt, aligned_start, aligned_end, lib->base, ph->p_vaddr, ph->p_memsz, ph->p_filesz);`
>> 
>> It is better to split this long line.
>> 
>> Thanks,
>> Serguei
>
> Thanks @sspitsyn for the comment!
> I fixed almost of your comment in new commit.
> 
>> The condition `(lib->end == (uintptr_t)-1L)` is a subset of `(lib->end < aligned_end)`, and so, can be removed. The same is true for the condition:
> 
> `lib->end` is declared as unsigned (`uintptr_t`), so we can't use `(lib->end < aligned_end)` when `lib->end` is set to -1.

BTW, one way to test these changes might be to do a `clhsdb pmap` and then do a `findpc` on the last address of each library's map (should find a symbol) and the first address after the map (should not find a symbol unless that happens to be the start of the next mapped library). I'm not sure if this will always work. It's possible that no symbol covers the very end of the map, especially due to page alignment. Maybe try the first address of the last page then. I'm not necessarily suggesting you write a test that does this, but maybe just do a bit of experimenting as a sanity check.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2563