Review request for fix for 7200297

[BILL PITTORE]

Hi Serguei,

Thanks for the review and your excellent suggestion at some 
simplification in dll_build_name. Applied the changes, built, test and 
all is still working. New webrev at:
http://cr.openjdk.java.net/~bpittore/7200297/webrev.01/

Are you an official reviewer?

thanks,
bill


On 11/15/2012 4:00 AM, serguei.spitsyn at oracle.com wrote:
>
> ||Hi Bill,
>
> *src/share/back/debugInit.c*
>
> **271 JVMTI_FUNC_PTR(gdata->jvmti,GetSystemProperty)
>
>    Space is missed after comma.
>
> ||*src/share/back/error_messages.c
> *
>
>   74 /* May be called before NPT is initialized so don't fault */
>
>     The comment seems to belong to the else statement.
>
> ||*src/share/back/transport.c
> *||*src/share/demo/jvmti/hprof/hprof.h*||*
> src/share/demo/jvmti/hprof/hprof_init.c*
>
>     No comments.
>
> ||*src/solaris/back/linker_md.c
> *
>
> ***Lines 75-96:*
>
>    It seems the *if/else* is not needed as the while loop would serve 
> the else as well.
>
> *     73 int n;*
>
>   Line #73 can be removed as "n" is not used.
>
> *   81 if ((p - pathname) == 0) {*
>
>    Nit: Replace *(p - pathname) == 0*   with *p == pathname*
>
> *   119 char *p;*
>
>    Line #119 can be removed as "p" is not used.
>
>
> ||*src/solaris/demo/jvmti/hprof/hprof_md.c
> *
>
> ***Lines 395-418:*
>
>      The same comments as for the *linker_md.c*.
>
>
> ||*src/solaris/npt/npt_md.h
> *
>
>     No comments.
>
> ||*src/windows/back/linker_md.c
> *
>
>    The same comments as for the *linker_md.c*.
>
>
> ||*src/windows/demo/jvmti/hprof/hprof_md.c
> *
>
>
> ****Lines 381-407:*
> *
>
>      The same comments as for the *linker_md.c*.
>
> *
> *
>
> ||*src/windows/npt/npt_md.h
> *
>
>     No comments.
>
>
> Thanks,
> Serguei
>
>
> On 11/14/12 2:27 PM, bill.pittore at oracle.com wrote:
>> This bug has to do with the jdwp and hprof agents not parsing the 
>> sun.boot.library.path (dll_dir) correctly since the fix for 6819213 
>> went in some years ago. This bug popped up while working on a 
>> particular platform that does not have the ability to set 
>> LD_LIBRARY_PATH before running the VM. As documented in the bug, on 
>> most platforms even if the sun.boot.library.path consists of multiple 
>> paths and the jdwp or hprof code fails to load a dependent lib, the 
>> system falls back to using LD_LIBRARY_PATH so the failure is masked. 
>> On some other platforms, this failover doesn't exist so we get an 
>> error when trying to load jdwp and hprof dependent libs.
>>
>> Some notes on a couple of files.
>> *
>> debugInit.c, hprof_init.c*:
>> Rearranged the init order so that the jvmti pointer gets initialized 
>> before attempting to load the npt shared library.
>>
>> *linker_md.c, hprof_md.c*
>> Much of the platform code in hprof and jdwp is duplicated and this 
>> change is no different. Based on the code in hotspot 
>> os_solaris/windows.cpp it parses the boot library path and attempts 
>> to find the library.
>> *
>> error_messages.c*
>> Fixed a bug in the error message code that blindly dereferenced the 
>> npt pointer even if it wasn't set because of an error in loading.
>>
>> Webrev: http://cr.openjdk.java.net/~bpittore/7200297/webrev.00/
>>
>> Ran the ute nsk/jdwp nsk/jvmti nsk/hprof tests, the JCK jdwp/jvmti 
>> tests and some command line testing on windows and linux.
>>
>> thanks,
>> bill
>>
>>
>


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20121115/a5e66397/attachment-0001.html