Review request for fix for 7200297

[serguei.spitsyn at oracle.com]

Bill,

It looks good.

Minor:
   The line "int n;" is not needed and can be deleted in the files:
||*src/solaris/back/linker_md.c*
||*src/windows/demo/jvmti/hprof/hprof_md.c*
*    src/windows/back/linker_md.c*
*src/windows/demo/jvmti/hprof/hprof_md.c*

||*
*Similar simplification can be done:
*  src/windows/demo/jvmti/hprof/hprof_md.c*


I do not have a reviewer status, but you can list as a reviewer. :)
Thank you for fixing it!

Thanks,
Serguei



On 11/15/12 11:27 AM, BILL PITTORE wrote:
> 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/fa03c1ef/attachment.html