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 >> <[email protected]> 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 >>>> <[email protected]> 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.
