RFR (S): JDK-8056242: Add function to return structured information about loaded libraries.

[Dmitry Samersoff]

Frederik,

Unfortunately I'm not able to build FreeBSD hotspot with latest
configure, so I can't test.

Changes looks OK for me.

-Dmitry

On 2014-09-02 15:39, Fredrik Arvidsson wrote:
> Hi
> 
> Heres the latest version of my patch. I have learned so much these last
> couple of days. I'm very grateful...
> 
> Webrev: http://cr.openjdk.java.net/~farvidsson/8056242/webrev.05/index.html
> 
> Dmitry: I don't really know how to test the BSD changes. Do you have an
> environment for that?
> 
> Cheers
> 
> /F
> 
> On 2014-09-01 15:12, Staffan Larsen wrote:
>> Thanks for doing these changes. Here are comments on the new version:
>>
>> os_windows.cpp
>> - Why not rename enumerate_modules() to get_loaded_modules_info() and
>> update all caller to use the new unified method?
>> - At the same time, can we get rid of the pid parameter to
>> enumerate_modules()? It looks like all callers set it the same value.
>> - nit: looks like the <= and > used to be aligned in these if-statements:
>> 1380    if (base_addr     <= pmod->addr &&
>> 1381        top_address > pmod->addr) {
>> 1436    if (base_addr     <= (address)_locate_jvm_dll &&
>> 1437        top_address > (address)_locate_jvm_dll) {
>>
>> os_solaris.cpp
>> - I think print_dll_info() should be re-implemented in terms of
>> get_loaded_modules_info() so that there is only one implementation of
>> this logic.
>> - get_loaded_modules_info() references os::print_dll_info() which
>> looks like a mistake?
>>
>> os_linux.cpp
>> - get_loaded_modules_info() could use some blank lines to split it up
>> into logical chunks of code.
>> - I think “r” is enough for fopen() - ie drop the “t”
>> - Can the fgets+sscanf be replaced by a call to fscanf? That would
>> avoid the ‘line’ variable and any possible problems of lines not
>> fitting into the space allocated.
>>
>> os_bsd.cpp
>> - Maybe add a comment about module_top_addr being 0?
>>
>> Thanks,
>> /Staffan
>>
>>
>> On 1 sep 2014, at 14:05, Fredrik Arvidsson
>> <fredrik.arvidsson at oracle.com> wrote:
>>
>>> Hi
>>>
>>> Here is an updated version of the patch. Thanks for the comments.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~farvidsson/8056242/webrev.01/index.html
>>>
>>> /Fredrik
>>>
>>> On 2014-08-28 16:21, Staffan Larsen wrote:
>>>> Hi Fredrik,
>>>>
>>>> A couple of comments:
>>>> - I would prefer if the new callback was unified with the one that
>>>> exists on Windows so that we have only on callback-based API for
>>>> listing dynamic libraries.
>>>> - If you do that, then would you also clean up enumerate_modules()
>>>> on windows to get rid of the non-NT support?
>>>> - Smaller: I think “address” is a better type to use instead of “u8”.
>>>>
>>>> Thanks
>>>> /Staffan
>>>>
>>>>
>>>> On 28 aug 2014, at 15:54, Fredrik Arvidsson
>>>> <fredrik.arvidsson at oracle.com> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> Please help me review this small enhancement.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~farvidsson/8056242/webrev.00/index.html
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8056242
>>>>>
>>>>> Cheers
>>>>> /F
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.